Gabor Kaszab 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 9: Code-Review+1 (5 comments) Thanks for the latest PS, Peter! I had some minor comments, but apart from them the patch seems fine. Will +2 once they are covered. Also a rebase is needed. Please keep them in a separate PS from each other for easier reviewability. http://gerrit.cloudera.org:8080/#/c/20515/9/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/20515/9/common/thrift/CatalogObjects.thrift@634 PS9, Line 634: 2: optional bool is_truncate required? http://gerrit.cloudera.org:8080/#/c/20515/9/fe/src/main/java/org/apache/impala/analysis/PartitionSet.java File fe/src/main/java/org/apache/impala/analysis/PartitionSet.java: http://gerrit.cloudera.org:8080/#/c/20515/9/fe/src/main/java/org/apache/impala/analysis/PartitionSet.java@57 PS9, Line 57: return; nit: same line as 'if' http://gerrit.cloudera.org:8080/#/c/20515/9/fe/src/main/java/org/apache/impala/analysis/PartitionSet.java@93 PS9, Line 93: if (e instanceof AnalysisException) { throw(AnalysisException) e; } No brackets are needed. http://gerrit.cloudera.org:8080/#/c/20515/9/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/20515/9/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@241 PS9, Line 241: if (params.iceberg_drop_partition_request.is_truncate) { I think this is an optional member. Should check isset first to be on the safe side. Or alternatively should make is_truncate a requird filed, that would also simplify the code on the other end where we populate this: Just unconditionally set it. http://gerrit.cloudera.org:8080/#/c/20515/9/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@246 PS9, Line 246: deleteFiles.deleteFile(path); nit: merge with line above -- 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: 9 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: Wed, 22 Nov 2023 12:16:52 +0000 Gerrit-HasComments: Yes
