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 15: (1 comment) http://gerrit.cloudera.org:8080/#/c/24577/15/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/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java@152 PS15, Line 152: String timeTravelTableKey = lowerCaseTableName + "_tt_" + tts.hashCode(); > TimeTravelSpec doesn't override equals()/hashCode() (nor does StmtNode), so Yeah, I didn't love doing this. My problem came down to the fact that I needed to generate an Identifier name at SqlNode creation time in Parser.jj for this table. I thought i had put in a comment somewhere and if I didn't, I should. The ImpalaSnapshotSqlNode.getTimeTravelSpecTableRef() uses the same table name string. This is what allows the internals of Calcite to validate the name of the table produced here. And this SqlNode is created at Parser.jj time. The Analyzer object isn't available at Parser.jj time. Ensuring this gets analyzed may be possible, but it would have been jumping through hoops in an already complicated commit. So the asOfVersion and asOfMicros isn't available until after analysis. One thing I will change is to generate the "..._tt_..." in a static method that is called by both places to ensure the strings match. You do make some good points, of course. This is gonna be a headache at some point for the Digest reasons. And the optimization is a good point too. There are ways we can do this, of course. Off the top of my head, we can run a pre-validation step that changes the SqlNodes. Or maybe somehow have the Digest obtain the name elsewhere (not sure if that's possible)? We can potentially find some way to avoid the duplication as well. We'd probably want to do that after the snapshotId is determined, since SYSTEM_TIME and SYSTEM_VERSION can use different expressions and still point to the same table (as well as multiple different SYSTEM_TIMEs) My initial thought was to punt this and file a Jira and handle this later. We can live without the optimization for a first pass, but the Digest thing makes this a little sketchy to do. I'm open to thoughts on this as to what you think would be a good solution here if you have one. Otherwise, I think I'd still like to punt this into a Jira to be handled later on. -- 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: 15 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[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: Mon, 17 Aug 2026 15:20:38 +0000 Gerrit-HasComments: Yes
