[FLINK-30481][FLIP-277] Resolving nits and simplifying expression str…

…ing parsing. Co-Authored-By: Samrat <[email protected]>
apache · Aug 18, 2024 · 8cf1627 · 8cf1627
1 parent 71f6374
commit 8cf1627
Show file tree

Hide file tree

Showing 5 changed files with 24 additions and 30 deletions.
diff --git a/flink-catalog-aws/flink-catalog-aws-glue/pom.xml b/flink-catalog-aws/flink-catalog-aws-glue/pom.xml
@@ -19,7 +19,7 @@ under the License.
 -->
 <project xmlns="http://maven.apache.org/POM/4.0.0"
          xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
-         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
 
     <modelVersion>4.0.0</modelVersion>
 

diff --git a/...flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalog.java b/...flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalog.java
@@ -842,7 +842,7 @@ public List<CatalogPartitionSpec> listPartitions(
      *
      * <p>If catalog does not support this interface at present, throw an {@link
      * UnsupportedOperationException} directly. If the catalog does not have a valid filter, throw
-     * the {@link UnsupportedOperationException} directly. Planner will fallback to get all
+     * the {@link UnsupportedOperationException} directly. Planner will fall back to get all
      * partitions and filter by itself.
      *
      * @param tablePath path of the table
@@ -938,7 +938,7 @@ public void dropPartition(
         checkNotNull(tablePath, "TablePath cannot be null.");
         checkNotNull(partitionSpec, "PartitionSpec cannot be null.");
         if (partitionExists(tablePath, partitionSpec)) {
-            Table glueTable = null;
+            Table glueTable;
             try {
                 glueTable = glueTableOperator.getGlueTable(tablePath);
             } catch (TableNotExistException e) {

diff --git a/...lue/src/main/java/org/apache/flink/table/catalog/glue/operator/GluePartitionOperator.java b/...lue/src/main/java/org/apache/flink/table/catalog/glue/operator/GluePartitionOperator.java
@@ -59,7 +59,6 @@
 import java.util.Optional;
 import java.util.stream.Collectors;
 
-import static org.apache.flink.table.catalog.glue.util.GlueUtils.getExpressionString;
 import static org.apache.flink.util.StringUtils.isNullOrWhitespaceOnly;
 
 /** Utilities for Glue catalog Partition related operations. */
@@ -78,8 +77,8 @@ public GluePartitionOperator(String catalogName, GlueClient glueClient, String g
      * @param glueTable glue table
      * @param partitionSpec partition spec
      * @param catalogPartition partition to add.
-     * @throws CatalogException
-     * @throws PartitionSpecInvalidException
+     * @throws CatalogException when partition is unable to be created.
+     * @throws PartitionSpecInvalidException when partition specification is invalid.
      */
     public void createGluePartition(
             final Table glueTable,
@@ -384,7 +383,7 @@ public List<CatalogPartitionSpec> listGluePartitionsByFilter(
             ObjectPath tablePath, List<Expression> filters) {
         String expression =
                 filters.stream()
-                        .map(x -> getExpressionString(x, new StringBuilder()))
+                        .map(GlueUtils::getExpressionString)
                         .collect(
                                 Collectors.joining(
                                         GlueCatalogConstants.SPACE

diff --git a/...nk-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/GlueUtils.java b/...nk-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/GlueUtils.java
@@ -84,6 +84,7 @@ public static String getGlueConventionalName(String name) {
      *
      * @param databaseProperties database properties.
      * @param databaseName fully qualified name for database.
+     * @param catalogPath catalog path.
      * @return location for database.
      */
     public static String extractDatabaseLocation(
@@ -104,6 +105,7 @@ public static String extractDatabaseLocation(
      *
      * @param tableProperties table properties.
      * @param tablePath fully qualified object for table.
+     * @param catalogPath catalog path.
      * @return location for table.
      */
     public static String extractTableLocation(
@@ -375,29 +377,24 @@ public static String getGlueFunctionClassName(CatalogFunction function) {
         }
     }
 
+    /**
+     * Derive the expression string from given {@link Expression}.
+     *
+     * @param expression Instance of {@link Expression}.
+     * @return Derived String from {@link Expression}.
+     */
+    public static String getExpressionString(Expression expression) {
+        return getExpressionString(expression, new StringBuilder());
+    }
+
     /**
      * Recursively derive the expression string from given {@link Expression}.
      *
      * @param expression Instance of {@link Expression}.
-     * @param sb StringBuilder.
+     * @param sb Used to build the derived expression string during recursion.
      * @return Derived String from {@link Expression}.
      */
-    //    public static String getExpressionString(Expression expression, StringBuilder sb) {
-    //
-    //        for (Expression childExpression : expression.getChildren()) {
-    //            if (childExpression.getChildren() != null &&
-    // !childExpression.getChildren().isEmpty()) {
-    //                getExpressionString(childExpression, sb);
-    //            }
-    //        }
-    //        return sb.insert(
-    //                        0,
-    //                        expression.asSummaryString()
-    //                                + GlueCatalogConstants.SPACE
-    //                                + GlueCatalogConstants.AND)
-    //                .toString();
-    //    }
-    public static String getExpressionString(Expression expression, StringBuilder sb) {
+    private static String getExpressionString(Expression expression, StringBuilder sb) {
         for (Expression childExpression : expression.getChildren()) {
             if (childExpression.getChildren() != null && !childExpression.getChildren().isEmpty()) {
                 getExpressionString(childExpression, sb);

diff --git a/...atalog-aws-glue/src/test/java/org/apache/flink/table/catalog/glue/util/GlueUtilsTest.java b/...atalog-aws-glue/src/test/java/org/apache/flink/table/catalog/glue/util/GlueUtilsTest.java
@@ -54,8 +54,8 @@ public void testGetGlueConventionalName() {
 
     @Test
     public void testExtractDatabaseLocation() {
-        Map<String, String> propertiesWithLocationUri =
-                new HashMap() {
+        HashMap<String, String> propertiesWithLocationUri =
+                new HashMap<String, String>() {
                     {
                         put(GlueCatalogConstants.LOCATION_URI, "s3://some-path/myDb/");
                         put("k1", "v1");
@@ -119,7 +119,7 @@ public void testGetCatalogDatabase() {
         String description = "Test description";
         Database database = Database.builder().parameters(params).description(description).build();
         CatalogDatabase catalogDatabase = GlueUtils.getCatalogDatabase(database);
-        Assertions.assertTrue(catalogDatabase instanceof CatalogDatabase);
+        Assertions.assertInstanceOf(CatalogDatabase.class, catalogDatabase);
         Assertions.assertEquals(catalogDatabase.getProperties(), params);
         Assertions.assertEquals(catalogDatabase.getDescription().orElse(null), description);
     }
@@ -169,9 +169,7 @@ public void testExtractTableOwner() {
 
     @Test
     public void testExpressionString() {
-        StringBuilder sb = new StringBuilder();
         Expression expression = ResolvedExpressionMock.of(DataTypes.INT(), "column1");
-        GlueUtils.getExpressionString(expression, sb);
-        Assertions.assertEquals("column1", sb.toString());
+        Assertions.assertEquals("column1", GlueUtils.getExpressionString(expression));
     }
 }