Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24577 )
Change subject: IMPALA-15178: Calcite Planner: support Iceberg time travel feature. ...................................................................... Patch Set 14: (9 comments) Thanks for working on this! http://gerrit.cloudera.org:8080/#/c/24577/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24577/14//COMMIT_MSG@26 PS14, Line 26: ImpalaTimeTravelTable nit: IcebergTimeTravelTable http://gerrit.cloudera.org:8080/#/c/24577/14//COMMIT_MSG@47 PS14, Line 47: <TimeTravelSpec hash code> If the tt table is visible to users via plans/query profiles, then probably we should use the snapshot id (asOfVersion_) / epoch (asOfMicros_). http://gerrit.cloudera.org:8080/#/c/24577/14/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/24577/14/fe/src/main/cup/sql-parser.cup@385 PS14, Line 385: Object Can you add comment why it is an Object? http://gerrit.cloudera.org:8080/#/c/24577/14/fe/src/main/java/org/apache/impala/analysis/Parser.java File fe/src/main/java/org/apache/impala/analysis/Parser.java: http://gerrit.cloudera.org:8080/#/c/24577/14/fe/src/main/java/org/apache/impala/analysis/Parser.java@65 PS14, Line 65: timestamp nit: timestamp or snapshot id? http://gerrit.cloudera.org:8080/#/c/24577/14/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/RemoveSnapshotRule.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/RemoveSnapshotRule.java: http://gerrit.cloudera.org:8080/#/c/24577/14/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/RemoveSnapshotRule.java@41 PS14, Line 41: onMatch Should we check that Snapshot is created on top of an Iceberg table, and only remove it in this case? http://gerrit.cloudera.org:8080/#/c/24577/14/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/CalciteIcebergTable.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/CalciteIcebergTable.java: http://gerrit.cloudera.org:8080/#/c/24577/14/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/CalciteIcebergTable.java@135 PS14, Line 135: true Should it only return true when timeTravelName_ != null? http://gerrit.cloudera.org:8080/#/c/24577/14/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java: http://gerrit.cloudera.org:8080/#/c/24577/14/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java@255 PS14, Line 255: localTableNames unused http://gerrit.cloudera.org:8080/#/c/24577/14/java/calcite-planner/src/main/java/org/apache/impala/calcite/util/JavaCCParserUtils.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/util/JavaCCParserUtils.java: http://gerrit.cloudera.org:8080/#/c/24577/14/java/calcite-planner/src/main/java/org/apache/impala/calcite/util/JavaCCParserUtils.java@39 PS14, Line 39: or null if coordinates are invalid. nit: it throws exceptions but doesn't return null http://gerrit.cloudera.org:8080/#/c/24577/14/testdata/workloads/functional-query/queries/QueryTest/iceberg-negative.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-negative.test: http://gerrit.cloudera.org:8080/#/c/24577/14/testdata/workloads/functional-query/queries/QueryTest/iceberg-negative.test@651 PS14, Line 651: AS OF clause is only supported for Iceberg tables. Do we need CALCITE_PLANNER_CATCH here as well? -- To view, visit http://gerrit.cloudera.org:8080/24577 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ab3466d6c453f8e030763749dd64903b8c41264 Gerrit-Change-Number: 24577 Gerrit-PatchSet: 14 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 23 Jul 2026 14:05:11 +0000 Gerrit-HasComments: Yes
