-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BEFORE syntax support for presto engine. #22631
Conversation
Codenotify: Notifying subscribers in CODENOTIFY files for diff 12a6e44...153bc1f.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include parser/formatter tests in TestSqlParser and TestStatementBuilder
presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java
Outdated
Show resolved
Hide resolved
presto-parser/src/main/java/com/facebook/presto/sql/ExpressionFormatter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me. Only some little nits.
presto-parser/src/main/java/com/facebook/presto/sql/tree/TableVersionExpression.java
Show resolved
Hide resolved
presto-parser/src/main/java/com/facebook/presto/sql/tree/TableVersionExpression.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Thanks!
Hey @gupteaj , this change has broken a bunch of tests.
Do we need to upgrade the antlr version? |
@zacw7 can you add a link to a test run? |
They are meta internal tests. Let me check the build tool. |
I suspect that the Meta build tool chain needs to recompile the parser module more aggressively. This is the second such instance I have seen in recent times |
Description
Presto issue #21971
Note - This PR has BEFORE syntax and engine side changes. For iceberg connector changes, test and doc, a new PR will be created.
Design description :
This feature will allow iceberg connector to query historical data using BEFORE syntax on a table.
Time travel version option will read bigint snapshot id value for the table. Time travel timestamp option will read
timestamp-with-time-zone value for the table.
examples
Parser, Semantic Analyzer, Metadata
In parser tree, update TableVersionExpression class to hold time travel operator ( EQUAL/LESS_THAN) for AS OF/BEFORE state
Motivation and Context
Snowflake provides a BEFORE clause for time travel queries. When used in a query, this clause is specified in the FROM clause immediately after the table name. It determines the point in the past from which historical data is requested for the object. The AT keyword includes changes made by a statement or transaction with a timestamp equal to the specified parameter, while the BEFORE keyword refers to a point immediately preceding the specified parameter.
IBM Netezza supports BEFORE clause similar to Snowflake.
References -
Snowflake - https://docs.snowflake.com/en/sql-reference/constructs/at-before
IBM Netezza - https://cloud.ibm.com/docs/netezza?topic=netezza-runningqueries_tt#before_tt
Impact
A new table level option to return specific snapshot for iceberg connector
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Note - Iceberg PR will have release notes and doc section