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 2: (6 comments) Added some more 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@131 PS2, Line 131: if (purgePartition_) { : throw new AnalysisException("Iceberg tables can't purge partitions"); : } I couldn't find a test for this code path. http://gerrit.cloudera.org:8080/#/c/20515/2/fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java@146 PS2, Line 146: if (icebergExpression == null) { : throw new AnalysisException( : "Invalid partition filtering expression: " + expr.toSql()); : } I think it would be cleaner to propagate an exception instead of relying on null results. 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: nit: empty line or missing comments 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 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: nit: indentation http://gerrit.cloudera.org:8080/#/c/20515/2/fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java@246 PS2, Line 246: + "backend handle it.", : ex); nit: no need for line break. -- 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: 2 Gerrit-Owner: Peter Rozsa <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 05 Oct 2023 12:37:23 +0000 Gerrit-HasComments: Yes
