Steve Carlin 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: (8 comments) http://gerrit.cloudera.org:8080/#/c/24577/14//COMMIT_MSG Commit Message: 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 This should not be visible to users. If it is, then it's a bug. We should have enough going information to have the real table name. It's only when Calcite has the thread of control where we need the ability to match the names up in this special way. 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? Done 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? Done 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 on Done 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? I'm debating on this one...the Iceberg table can always be temporal whether we are using time travel in the query or not. But putting "!= null" will also always be correct in our case. I guess I decided to make the change though :) 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 Done 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 Done 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? Removed this. I sometimes test this on top of other commits I have in my local branch. I have a commit that only falls back when the error is UnsupportedFeatureException. When that happens, I need to capture the Calcite specific message. In this case with the code currently committed, this will fall back to the original planner and produce the same message. -- 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: Wed, 29 Jul 2026 12:21:25 +0000 Gerrit-HasComments: Yes
