Aleksandr Efimov 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 16: (1 comment) This time I read the whole patch rather than just the naming thread. The split between resolving the snapshot at validation time and having the planner read the spec off the helper reads nicely, and the non-Calcite path still behaves as before - SingleNodePlanner passes tblRef.getTimeTravelSpec() into ScanNodeHelperImpl and only IcebergScanPlanner consumes it. One question below, then I'm happy to +1. http://gerrit.cloudera.org:8080/#/c/24577/16/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/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/util/JavaCCParserUtils.java@67 PS16, Line 67: currentAbsoluteIndex += lineLength + 1; Small thing about the arithmetic here: JavaCC treats \r\n as a single line break (SimpleCharStream skips the line increment for the \n after a \r), but the string still holds two characters. So for a statement that arrives with CRLF, startIndex ends up one character short per preceding line and the extracted token shifts left. The reason I mention it is that it can pass silently rather than fail: in a multi-line "... FOR SYSTEM_VERSION AS OF 12345", a one-character shift gives " 1234", which parses happily and picks a different snapshot. Would normalizing the line endings first be enough, e.g. extracting from originalSource.replace("\r\n", "\n")? JavaCC's positions already treat CRLF as one break. And if something upstream normalizes the statement before SqlParser.create(), just ignore me. -- 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: 16 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: Tue, 18 Aug 2026 20:15:05 +0000 Gerrit-HasComments: Yes
