Zoltan Borok-Nagy 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 3: (12 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/17765/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17765/1//COMMIT_MSG@9 PS1, Line 9: This patch adds support "FOR SYSTEM_TIME AS OF" and > Please clarify the the timestamp specified with "FOR SYSTEM_TIME AS OF" is Done http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/main/cup/sql-parser.cup@3069 PS1, Line 3069: numeric_l > This could be integer literal. INTERGER_LITERAL has type of BigDecimal, so we can use numeric_literal here. 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: // Time travel spec of this table ref. It contains information specified in the > Please add a comment. Done http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/main/java/org/apache/impala/analysis/TableRef.java@222 PS1, Line 222: hasExplicitAlias_ = other.hasExplicitAlias_; > Maybe cloning the TimeTravelSpec object would be safer than just copying th Done http://gerrit.cloudera.org:8080/#/c/17765/1/fe/src/main/java/org/apache/impala/analysis/TableRef.java@222 PS1, Line 222: hasExplicitAlias_ = other.hasExplicitAlias_; > Alternatively, you could set timeTravelSpec_ to null in reset(), instead of I think we should clone it because we get the time travel spec in the constructor, i.e. it's not something that we can recreate during analyze(). Setting it to null in reset() would cause bugs when the query is rewritten and re-analyzed without a time travel spec. 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: asOfExpr_.analyze(analyzer); > you could also check that asOfVersion_ > 0 Added a check, but now that FOR SYSTEM_VERSION AS OF only accepts numeric_literals it's not possible to write FOR SYSTEM_TIME AS OF -123; because '-123' can only be parsed as an expr, not as a numeric_literal. Added tests to ParserTest.java. Also the above (asOfExpr_ instanceof LiteralExpr) is redundant for the same reason. But I probably keep these checks here in case we change the parser in the future. 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 t Done 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(table, timeTravelSpec); > nit: Since 'baseTable' is not used anywhere else, you could move L514 insid Good idea, done. 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: TblsAnalysisError("select * from $TBL for system_time as of now()", nonIceT, > The end of the error msg was left out intentionally? Since then this test has been removed due to SYSTEM_VERSION AS OF expects a numeric_literal now. 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: sn > 'snapshot_id' ? Done http://gerrit.cloudera.org:8080/#/c/17765/1/tests/query_test/test_iceberg.py@197 PS1, Line 197: # Query old snapshot > Maybe add another test to query with a timestamp in the future. Currently querying the future behaves the same as querying by now(). I'm not sure if we want a test for it to preserve this behavior. http://gerrit.cloudera.org:8080/#/c/17765/1/tests/query_test/test_iceberg.py@197 PS1, Line 197: # Query old snapshot > You could also test (if not too much work) that switching Impala to another We are using the local timezone of the machine that executes the test. I don't know what's the best way to change to a timezene relative to the current one. In some cases it might be not possible, e.g. if you are in GMT+12 then you cannot switch to GMT+13, but GMT-11 wouldn't work either. Or am I missing something? -- 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: 3 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: Thu, 26 Aug 2021 12:30:47 +0000 Gerrit-HasComments: Yes
