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: (3 comments) The patch looks good. At first glance I found only minor issues. http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/main/java/org/apache/impala/util/IcebergUtil.java File fe/src/main/java/org/apache/impala/util/IcebergUtil.java: http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@515 PS1, Line 515: TableScan scan = createScanAsOf( nit: Since 'baseTable' is not used anywhere else, you could move L514 inside createScanAsOf(). This way createScanAsOf() would need only 2 params : table and timeTravelSpec. http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@4884 PS1, Line 4884: iceT, "FOR SYSTEM_VERSION AS OF <expression> must be an integer type but is"); The end of the error msg was left out intentionally? http://gerrit.cloudera.org:8080/#/c/17765/1/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/17765/1/tests/query_test/test_iceberg.py@134 PS1, Line 134: ts 'snapshot_id' ? -- 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: Tue, 24 Aug 2021 18:01:16 +0000 Gerrit-HasComments: Yes
