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 5: (4 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/17765/5/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/17765/5/fe/src/main/cup/sql-parser.cup@3066 PS5, Line 3066: opt_asof ::= > This is cool. Btw Delta also supports '@' syntax which can be convenient an Yeah, that is more concise. I discussed it with Hive/Spark contributors and the conclusion was to use the standard SQL2011 sytnax even if it's a bit more verbose. But I agree, if the '@' syntax gets popular in multiple engines in the future then we can consider adding support for it. http://gerrit.cloudera.org:8080/#/c/17765/5/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/17765/5/tests/query_test/test_iceberg.py@116 PS5, Line 116: def test_time_travel(self, vector, unique_database): > One generic question about Time Travel I had was w.r.t Compaction: Snapshots should be available until they are expired: https://iceberg.apache.org/maintenance/#recommended-maintenance It's not easy to test it currently as Impala/Hive don't support compaction yet. I created a Jira about testing: IMPALA-10892 http://gerrit.cloudera.org:8080/#/c/17765/5/tests/query_test/test_iceberg.py@171 PS5, Line 171: # Future queries return the current snapshot. > nice. Is it similar behaviour when we specify version greater than the curr No, you can only query concrete snapshots. Also, the snapshot IDs are not in an increasing order, i.e. newer snapshot might have a lower snapshot ID. http://gerrit.cloudera.org:8080/#/c/17765/5/tests/query_test/test_iceberg.py@205 PS5, Line 205: self.execute_query("SELECT * FROM {0} FOR SYSTEM_TIME AS OF {1}".format( > Does this cover the case when older snapshots are expired too Yes, but it's not easy to add a test for it currently. It will be covered by IMPALA-10892. -- 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: 5 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Amogh Margoor <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 27 Aug 2021 11:52:42 +0000 Gerrit-HasComments: Yes
