Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/20515 )
Change subject: IMPALA-12243: Add support for DROP PARTITION for Iceberg tables ...................................................................... Patch Set 8: (17 comments) Thank you for the changes Peter. Added some comments, mainly nits/formatting. http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java: http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java@96 PS8, Line 96: addPartParams This is not originating from your change, but looks like an copy&paste from AlterTableAddPartitionStmt. Could you rename it? http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionExpr.java File fe/src/main/java/org/apache/impala/analysis/IcebergPartitionExpr.java: http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionExpr.java@39 PS8, Line 39: nit: empty new line http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionExpr.java@129 PS8, Line 129: Column column = desc.getColumn(); : IcebergColumn icebergColumn = (IcebergColumn) column; nit: could be one line; then we don't need to import Column http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionExpr.java@133 PS8, Line 133: partitionSpec_.getFieldsBySourceId( : icebergColumn.getFieldId()); nit: should fit into one line http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionExpr.java@182 PS8, Line 182: /*Keeping it empty to avoid constant folding */ nit: missing space http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionExpressionRewriter.java File fe/src/main/java/org/apache/impala/analysis/IcebergPartitionExpressionRewriter.java: http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionExpressionRewriter.java@102 PS8, Line 102: if (!(literal instanceof LiteralExpr)) { : return binaryPredicate; : } nit: this could fit into one line, like L107 http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java File fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java: http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java@98 PS8, Line 98: if (table instanceof IcebergTable){ nit: space http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java@100 PS8, Line 100: } : else{ nit: one line http://gerrit.cloudera.org:8080/#/c/20515/2/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergTable.java: http://gerrit.cloudera.org:8080/#/c/20515/2/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@401 PS2, Line 401: > It originates from IcebergUtil/getPartitionTransformType. Before this patch Yeah, somehow we should prevent AnalysisException during table load. http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java File fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java: http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java@170 PS8, Line 170: Preconditions.checkState(false, : "Unsupported Iceberg type considered for predicate: %s", : literal.getType().toSql()); : } : } : throw new ImpalaRuntimeException( : String.format("Unable to parse Iceberg value: %s,", : literal.getStringValue())); This throw + precondition check looks redundant. http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java@253 PS8, Line 253: throw new ImpalaRuntimeException( nit: indentation http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java@296 PS8, Line 296: String.format("Unable to create term from expression: %s", : expr.toSql())); nit: one line http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java@302 PS8, Line 302: String.format("Invalid column type %s for column: %s", : column.getType(), column)); nit: one line http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java@306 PS8, Line 306: return new Term(Expressions.ref(column.getName()), : (IcebergColumn) column); nit: one line http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java@309 PS8, Line 309: } nit: missing empty line http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/util/IcebergUtil.java File fe/src/main/java/org/apache/impala/util/IcebergUtil.java: http://gerrit.cloudera.org:8080/#/c/20515/8/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@810 PS8, Line 810: parseDayToTransformMonth Do we need this method? Looks like this could fit into one line in getDateTimeTransformValue and not used anywhere else. http://gerrit.cloudera.org:8080/#/c/20515/8/testdata/workloads/functional-query/queries/QueryTest/iceberg-drop-partition.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-drop-partition.test: http://gerrit.cloudera.org:8080/#/c/20515/8/testdata/workloads/functional-query/queries/QueryTest/iceberg-drop-partition.test@288 PS8, Line 288: : nit: empty lines -- To view, visit http://gerrit.cloudera.org:8080/20515 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a768ba2966f570454687e02e4e6d67df46741f9 Gerrit-Change-Number: 20515 Gerrit-PatchSet: 8 Gerrit-Owner: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Mon, 13 Nov 2023 14:53:34 +0000 Gerrit-HasComments: Yes
