From 8cf16271921413d19f5460e84fbebe74a17cd450 Mon Sep 17 00:00:00 2001 From: Anthony Pounds-Cornish Date: Sun, 18 Aug 2024 01:32:34 +0100 Subject: [PATCH] [FLINK-30481][FLIP-277] Resolving nits and simplifying expression string parsing. Co-Authored-By: Samrat <40290566+Samrat002@users.noreply.github.com> --- .../flink-catalog-aws-glue/pom.xml | 2 +- .../flink/table/catalog/glue/GlueCatalog.java | 4 +-- .../glue/operator/GluePartitionOperator.java | 7 ++--- .../table/catalog/glue/util/GlueUtils.java | 31 +++++++++---------- .../catalog/glue/util/GlueUtilsTest.java | 10 +++--- 5 files changed, 24 insertions(+), 30 deletions(-) diff --git a/flink-catalog-aws/flink-catalog-aws-glue/pom.xml b/flink-catalog-aws/flink-catalog-aws-glue/pom.xml index e6064069..db6abb45 100644 --- 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. --> + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> 4.0.0 diff --git a/flink-catalog-aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalog.java b/flink-catalog-aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalog.java index 1eaf04e7..629dfdab 100644 --- a/flink-catalog-aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalog.java +++ b/flink-catalog-aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalog.java @@ -842,7 +842,7 @@ public List listPartitions( * *

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/flink-catalog-aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/operator/GluePartitionOperator.java b/flink-catalog-aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/operator/GluePartitionOperator.java index 9163a154..e612c3fa 100644 --- a/flink-catalog-aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/operator/GluePartitionOperator.java +++ b/flink-catalog-aws/flink-catalog-aws-glue/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 listGluePartitionsByFilter( ObjectPath tablePath, List filters) { String expression = filters.stream() - .map(x -> getExpressionString(x, new StringBuilder())) + .map(GlueUtils::getExpressionString) .collect( Collectors.joining( GlueCatalogConstants.SPACE diff --git a/flink-catalog-aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/GlueUtils.java b/flink-catalog-aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/GlueUtils.java index f265e4d0..073ba748 100644 --- a/flink-catalog-aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/GlueUtils.java +++ b/flink-catalog-aws/flink-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/flink-catalog-aws/flink-catalog-aws-glue/src/test/java/org/apache/flink/table/catalog/glue/util/GlueUtilsTest.java b/flink-catalog-aws/flink-catalog-aws-glue/src/test/java/org/apache/flink/table/catalog/glue/util/GlueUtilsTest.java index 671500a5..d667aad2 100644 --- a/flink-catalog-aws/flink-catalog-aws-glue/src/test/java/org/apache/flink/table/catalog/glue/util/GlueUtilsTest.java +++ b/flink-catalog-aws/flink-catalog-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 propertiesWithLocationUri = - new HashMap() { + HashMap propertiesWithLocationUri = + new HashMap() { { 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)); } }