Peter Rozsa 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 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/20515/2/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/2/fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java@62 PS2, Line 62: // Stores files selected by Iceberg's partition filtering : private IcebergContentFileStore icebergContentFileStore_; : // > nit: single-line comment Done http://gerrit.cloudera.org:8080/#/c/20515/2/fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java@66 PS2, Line 66: : public AlterTableDropPartitionStmt(TableName tableName, : > nit: single-line comment Done http://gerrit.cloudera.org:8080/#/c/20515/2/fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java@114 PS2, Line 114: throw new AnalysisExcept > Do we need this for Alter statements? StmtBase does this but AlterTableStmt In L287 there's a usage for analyzer_, I think it's easier to keep the reference for it instead of pushing it all the way down http://gerrit.cloudera.org:8080/#/c/20515/2/fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java@131 PS2, Line 131: List<Expr> expressions = partitionSet_.getExprs(); : FeIcebergTable table = (FeIcebergTable) table_; : > I couldn't find a test for this code path. Done http://gerrit.cloudera.org:8080/#/c/20515/2/fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java@146 PS2, Line 146: catch (AnalysisException e) { : throw new AnalysisException( : "Invalid partition filtering expression: " + expr.toSql()); : } > I think it would be cleaner to propagate an exception instead of relying on Done http://gerrit.cloudera.org:8080/#/c/20515/2/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/2/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionExpr.java@41 PS2, Line 41: private final IcebergTable table_; > nit: empty line or missing comments Done 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: > I believe, this is a bit confusing, we should not throw an AnalysisExceptio It originates from IcebergUtil/getPartitionTransformType. Before this patch, it threw TableLoadException, which is improper for that method (there's no loading, just switching on the transformType). If it's unwanted here, maybe somewhere down in the call chain there should be a catch that wraps that AnalysisException. http://gerrit.cloudera.org:8080/#/c/20515/2/fe/src/main/java/org/apache/impala/common/IcebergPartitionPredicateConverter.java File fe/src/main/java/org/apache/impala/common/IcebergPartitionPredicateConverter.java: http://gerrit.cloudera.org:8080/#/c/20515/2/fe/src/main/java/org/apache/impala/common/IcebergPartitionPredicateConverter.java@114 PS2, Line 114: > nit: missing empty line Done http://gerrit.cloudera.org:8080/#/c/20515/2/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/2/fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java@152 PS2, Line 152: switch (literal.getType().getPrimi > nit: indentation Done http://gerrit.cloudera.org:8080/#/c/20515/2/fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java@246 PS2, Line 246: LOG.warn("Exception occurred during timestamp conversion: %s" : > nit: no need for line break. Done -- 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: 3 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: Wed, 11 Oct 2023 08:33:36 +0000 Gerrit-HasComments: Yes
