Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/17765 )
Change subject: IMPALA-10840: Add support for "FOR SYSTEM_TIME AS OF" and "FOR SYSTEM_VERSION AS OF" for Iceberg tables ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/main/java/org/apache/impala/analysis/TableRef.java File fe/src/main/java/org/apache/impala/analysis/TableRef.java: http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/main/java/org/apache/impala/analysis/TableRef.java@152 PS1, Line 152: protected TimeTravelSpec timeTravelSpec_; Please add a comment. http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/main/java/org/apache/impala/analysis/TableRef.java@222 PS1, Line 222: timeTravelSpec_ = other.timeTravelSpec_; Maybe cloning the TimeTravelSpec object would be safer than just copying the reference. Perhaps it is not an issue, but if 2 TableRef instances share the same 'timeTravelSpec_' reference and one of them is reset(), that will affect the other instance as well, right? http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/main/java/org/apache/impala/analysis/TimeTravelSpec.java File fe/src/main/java/org/apache/impala/analysis/TimeTravelSpec.java: http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/main/java/org/apache/impala/analysis/TimeTravelSpec.java@113 PS1, Line 113: asOfVersion_ = asOfExpr_.evalToInteger(analyzer, "SYSTEM_VERSION AS OF"); you could also check that asOfVersion_ > 0 http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java File fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java: http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java@132 PS1, Line 132: } catch (IOException ex) { Does 'ex' contain dataFile.path() ? If not, please add dataFile.path() to the exception in L133 -- To view, visit http://gerrit.cloudera.org:8080/17765 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib523c5e47b8d9c377bea39a82fe20249177cf824 Gerrit-Change-Number: 17765 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 25 Aug 2021 16:08:15 +0000 Gerrit-HasComments: Yes
