Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/23042 )
Change subject: IMPALA-12337: Implement delete orphan files for Iceberg table ...................................................................... Patch Set 3: (7 comments) Thanks for working on this, Riza! http://gerrit.cloudera.org:8080/#/c/23042/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23042/3//COMMIT_MSG@9 PS3, Line 9: implement nit: implements http://gerrit.cloudera.org:8080/#/c/23042/3//COMMIT_MSG@14 PS3, Line 14: The bulk of implementation copies Hive's implementation of : org.apache.iceberg.actions.DeleteOrphanFiles interface (HIVE-27906). : The only difference is Could you please describe the implementation logic here besides/instead of the reference? http://gerrit.cloudera.org:8080/#/c/23042/3//COMMIT_MSG@20 PS3, Line 20: executed nit: is executed http://gerrit.cloudera.org:8080/#/c/23042/3/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRemoveOrphanFilesStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRemoveOrphanFilesStmt.java: http://gerrit.cloudera.org:8080/#/c/23042/3/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRemoveOrphanFilesStmt.java@36 PS3, Line 36: <parameter> Could you list the possible parameters in the comment, e.g. in bullet points? http://gerrit.cloudera.org:8080/#/c/23042/3/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRemoveOrphanFilesStmt.java@75 PS3, Line 75: timestampEpr nit: Did you mean timestampExpr? http://gerrit.cloudera.org:8080/#/c/23042/3/fe/src/main/java/org/apache/impala/catalog/iceberg/ImpalaIcebergDeleteOrphanFiles.java File fe/src/main/java/org/apache/impala/catalog/iceberg/ImpalaIcebergDeleteOrphanFiles.java: http://gerrit.cloudera.org:8080/#/c/23042/3/fe/src/main/java/org/apache/impala/catalog/iceberg/ImpalaIcebergDeleteOrphanFiles.java@61 PS3, Line 61: This is copied from HiveIcebergDeleteOrphanFiles.java with minimal changes : * (HIVE-27906). Could you please explain the logic here in more depth? The Hive implementation might change. Also, avoiding this indirection helps the reader understand the code. http://gerrit.cloudera.org:8080/#/c/23042/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/23042/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@4595 PS3, Line 4595: // Negative tests Please add a test case with no/empty parameter. -- To view, visit http://gerrit.cloudera.org:8080/23042 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5979cdf15048d5a2c4784918533f65f32e888de0 Gerrit-Change-Number: 23042 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 18 Jun 2025 11:07:15 +0000 Gerrit-HasComments: Yes
