Riza Suminto 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 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/23042/6/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/6/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRemoveOrphanFilesStmt.java@38 PS6, Line 38: as (NOW( > When we document this feature we should make it very clear not to use NOW() Agree. We should definitely warn that. I think the opposite scenario can be valid too. Let say big INSERT INTO crash, and the user knows that there is no other user / data pipeline going into the table, passing NOW() can be desirable as quick fix. http://gerrit.cloudera.org:8080/#/c/23042/6/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/6/fe/src/main/java/org/apache/impala/catalog/iceberg/ImpalaIcebergDeleteOrphanFiles.java@61 PS6, Line 61: This is copied from HiveIcebergDeleteOrphanFiles.java with minimal changes : * (HIVE-27906, > Please also include the git hash, so later we can quickly check for changes Done http://gerrit.cloudera.org:8080/#/c/23042/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/23042/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1687 PS6, Line 1687: Map<String, String> propert > I just wonder if we should return early after L1631, this way we might save Great idea! Done. http://gerrit.cloudera.org:8080/#/c/23042/6/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/23042/6/tests/query_test/test_iceberg.py@464 PS6, Line 464: def test_execut > Can we make this test work on S3 as well? Since we are removing files I thi test_load works with all FileSystem, so this should be OK to drop as well. -- 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: 8 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: Tue, 24 Jun 2025 17:50:28 +0000 Gerrit-HasComments: Yes
