Re: [PR] [API] Refactor api module [incubator-paimon-webui]

["SteNicholas (via GitHub)" <gi...@apache.org>]

SteNicholas commented on code in PR #45:
URL: https://github.com/apache/incubator-paimon-webui/pull/45#discussion_r1350262166


##########
paimon-web-api/src/main/java/org/apache/paimon/web/api/catalog/PaimonServiceFactory.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.web.api.catalog;
+
+import org.apache.paimon.catalog.CatalogContext;
+import org.apache.paimon.catalog.CatalogFactory;
+import org.apache.paimon.options.Options;
+import org.apache.paimon.web.api.common.CatalogProperties;
+import org.apache.paimon.web.api.common.MetastoreType;
+
+import org.apache.commons.lang3.StringUtils;
+
+/** Paimon service factory. */
+public class PaimonServiceFactory {
+
+    public static PaimonService createFileSystemCatalogService(String name, String warehouse) {
+        Options options = new Options();
+        options.set(CatalogProperties.WAREHOUSE, warehouse + "/" + name);
+
+        CatalogContext context = CatalogContext.create(options);
+
+        return new PaimonService(CatalogFactory.createCatalog(context), name);
+    }
+
+    public static PaimonService createHiveCatalogService(

Review Comment:
   Could this method merge with below method?



##########
paimon-web-api/src/main/java/org/apache/paimon/web/api/table/TableChange.java:
##########
@@ -0,0 +1,480 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.web.api.table;
+
+import org.apache.paimon.types.DataType;
+
+import javax.annotation.Nullable;
+
+import java.util.Objects;
+
+/**
+ * This change represents the modification of the table including adding, modifying and dropping
+ * column etc.
+ */
+public interface TableChange {
+    static AddColumn add(ColumnMetadata column) {
+        return new AddColumn(column, null);
+    }
+
+    static AddColumn add(ColumnMetadata column, @Nullable ColumnPosition position) {
+        return new AddColumn(column, position);
+    }
+
+    static ModifyColumn modify(
+            ColumnMetadata oldColumn,
+            ColumnMetadata newColumn,
+            @Nullable ColumnPosition columnPosition) {
+        return new ModifyColumn(oldColumn, newColumn, columnPosition);
+    }
+
+    static ModifyColumnType modifyColumnType(ColumnMetadata oldColumn, DataType newType) {
+        return new ModifyColumnType(oldColumn, newType);
+    }
+
+    static ModifyColumnName modifyColumnName(ColumnMetadata oldColumn, String newName) {
+        return new ModifyColumnName(oldColumn, newName);
+    }
+
+    static ModifyColumnComment modifyColumnComment(ColumnMetadata oldColumn, String newComment) {
+        return new ModifyColumnComment(oldColumn, newComment);
+    }
+
+    static ModifyColumnPosition modifyColumnPosition(
+            ColumnMetadata oldColumn, ColumnPosition columnPosition) {
+        return new ModifyColumnPosition(oldColumn, columnPosition);
+    }
+
+    static DropColumn dropColumn(String columnName) {
+        return new DropColumn(columnName);
+    }
+
+    static SetOption set(String key, String value) {
+        return new SetOption(key, value);
+    }
+
+    static RemoveOption remove(String key) {

Review Comment:
   Why not reuse `SetOption`? Any difference between `SetOption` and `RemoveOption`?



##########
paimon-web-api/src/main/java/org/apache/paimon/web/api/exception/DatabaseException.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.web.api.exception;
+
+/** database exception. */

Review Comment:
   ```suggestion
   /** Database exception. */
   ```



##########
paimon-web-api/src/main/java/org/apache/paimon/web/api/exception/ColumnException.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.web.api.exception;
+
+/** column exception. */

Review Comment:
   ```suggestion
   /** Column exception. */
   ```



##########
paimon-web-api/src/main/java/org/apache/paimon/web/api/exception/DatabaseException.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.web.api.exception;
+
+/** database exception. */
+public class DatabaseException extends RuntimeException {
+    public DatabaseException(String message) {
+        super(message);
+    }
+
+    /** database already exists exception. */
+    public static class DatabaseAlreadyExistsException extends DatabaseException {
+
+        public DatabaseAlreadyExistsException(String message) {
+            super(message);
+        }
+    }
+
+    /** database not empty exception. */
+    public static class DatabaseNotEmptyException extends DatabaseException {
+
+        public DatabaseNotEmptyException(String message) {
+            super(message);
+        }
+    }
+
+    /** database not exist exception. */

Review Comment:
   ```suggestion
       /** The exception that the database does not exist. */
   ```



##########
paimon-web-api/src/test/java/org/apache/paimon/web/api/catalog/PaimonServiceTest.java:
##########
@@ -0,0 +1,291 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.web.api.catalog;
+
+import org.apache.paimon.table.Table;
+import org.apache.paimon.types.DataType;
+import org.apache.paimon.types.DataTypes;
+import org.apache.paimon.web.api.exception.DatabaseException;
+import org.apache.paimon.web.api.exception.TableException;
+import org.apache.paimon.web.api.table.ColumnMetadata;
+import org.apache.paimon.web.api.table.TableChange;
+import org.apache.paimon.web.api.table.TableMetadata;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+
+/** The test class of catalog creator in {@link PaimonService}. */
+public class PaimonServiceTest {
+
+    private String warehouse;
+
+    private PaimonService catalog;
+
+    @TempDir java.nio.file.Path tempFile;

Review Comment:
   Why not import `java.nio.file.Path`?



##########
paimon-web-api/src/main/java/org/apache/paimon/web/api/exception/DatabaseException.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.web.api.exception;
+
+/** database exception. */
+public class DatabaseException extends RuntimeException {
+    public DatabaseException(String message) {
+        super(message);
+    }
+
+    /** database already exists exception. */
+    public static class DatabaseAlreadyExistsException extends DatabaseException {
+
+        public DatabaseAlreadyExistsException(String message) {
+            super(message);
+        }
+    }
+
+    /** database not empty exception. */

Review Comment:
   ```suggestion
       /** The exception that the database has already existed. */
   ```



##########
paimon-web-api/src/main/java/org/apache/paimon/web/api/table/ColumnMetadata.java:
##########
@@ -52,4 +56,12 @@ public DataType type() {
     public String description() {
         return this.description;
     }
+
+    public ColumnMetadata copy(DataType newDataType) {
+        return new ColumnMetadata(this.name, newDataType, this.description);
+    }
+
+    public ColumnMetadata withComment(String comment) {

Review Comment:
   Use getter/setter instead of with.



##########
paimon-web-api/src/main/java/org/apache/paimon/web/api/exception/ColumnException.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.web.api.exception;
+
+/** column exception. */
+public class ColumnException extends RuntimeException {
+
+    public ColumnException(String message) {
+        super(message);
+    }
+
+    /** column already exists exception. */
+    public static class ColumnAlreadyExistException extends ColumnException {
+
+        public ColumnAlreadyExistException(String message) {
+            super(message);
+        }
+    }
+
+    /** column not exist exception. */

Review Comment:
   ```suggestion
       /** The exception that the column does not exist. */
   ```



##########
paimon-web-api/src/test/java/org/apache/paimon/web/api/catalog/PaimonServiceTest.java:
##########
@@ -0,0 +1,291 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.web.api.catalog;
+
+import org.apache.paimon.table.Table;
+import org.apache.paimon.types.DataType;
+import org.apache.paimon.types.DataTypes;
+import org.apache.paimon.web.api.exception.DatabaseException;
+import org.apache.paimon.web.api.exception.TableException;
+import org.apache.paimon.web.api.table.ColumnMetadata;
+import org.apache.paimon.web.api.table.TableChange;
+import org.apache.paimon.web.api.table.TableMetadata;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+
+/** The test class of catalog creator in {@link PaimonService}. */
+public class PaimonServiceTest {
+
+    private String warehouse;
+
+    private PaimonService catalog;

Review Comment:
   ```suggestion
       private PaimonService service;
   ```



##########
paimon-web-api/src/main/java/org/apache/paimon/web/api/exception/ColumnException.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.web.api.exception;
+
+/** column exception. */
+public class ColumnException extends RuntimeException {
+
+    public ColumnException(String message) {
+        super(message);
+    }
+
+    /** column already exists exception. */

Review Comment:
   ```suggestion
       /** The exception that the column has already existed. */
   ```



##########
paimon-web-api/src/test/java/org/apache/paimon/web/api/catalog/PaimonServiceTest.java:
##########
@@ -0,0 +1,291 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.web.api.catalog;
+
+import org.apache.paimon.table.Table;
+import org.apache.paimon.types.DataType;
+import org.apache.paimon.types.DataTypes;
+import org.apache.paimon.web.api.exception.DatabaseException;
+import org.apache.paimon.web.api.exception.TableException;
+import org.apache.paimon.web.api.table.ColumnMetadata;
+import org.apache.paimon.web.api.table.TableChange;
+import org.apache.paimon.web.api.table.TableMetadata;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+
+/** The test class of catalog creator in {@link PaimonService}. */
+public class PaimonServiceTest {
+
+    private String warehouse;
+
+    private PaimonService catalog;
+
+    @TempDir java.nio.file.Path tempFile;
+
+    private final String db = "test_default_db";
+
+    @BeforeEach
+    public void before() {

Review Comment:
   Adds the after() method to dropDatabase to clean test resources.



##########
paimon-web-common/src/main/java/org/apache/paimon/web/common/utils/ParameterValidationUtil.java:
##########
@@ -32,4 +34,13 @@ public static void checkNotNull(Map.Entry<Object, Supplier<String>>... entries)
             }
         }
     }
+
+    @SafeVarargs
+    public static void checkNotNull(Pair<Object, String>... entries) {
+        for (Pair<Object, String> entry : entries) {
+            if (entry.getLeft() == null) {
+                throw new IllegalArgumentException(entry.getRight() + " cannot be null.");

Review Comment:
   Why use `entry.getRight()` in exception message?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org