-
Notifications
You must be signed in to change notification settings - Fork 592
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
feat(batch): support as of now() - interval for time travel #17665
Conversation
c92904b
to
58b87a5
Compare
c7d9565
to
9210ca4
Compare
src/sqlparser/src/ast/mod.rs
Outdated
// used by time travel | ||
ProcessTimeWithInterval((String, Option<DateTimeField>)), |
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.
Should we evaluate ProcessTimeWithInterval
to TimestampNum
at the parser? In this way, we can unify the code.
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.
I've also considered that once.
The issue is the Interval parser is not in sqlparser crate. Not sure if it's a good idea to move this Interval parser to sqlparser crate, because it'll introduce additional dependencies to sqlparser crate.
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.
It's similar to why we don't evaludate AsOf::TimestampString to AsOf::TimestampNum.
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.
I see. ProcessTimeWithInterval((String, Option<DateTimeField>)),
this enum looks a bit special because it only can represent an expression like CURRENT() - xxx
but not another form. Can we use an expression instead, so that we can support a more general expression like CURRENT() + xxx - yyy
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.
For example, this sql select now() - interval '1' Day + interval '2' hour;
could be evaluated to a const value.
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.
As discussed offline, using expr as a better idea, can be done in further PR.
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.
Rest LGTM
src/sqlparser/src/parser.rs
Outdated
preceded( | ||
( | ||
Self::parse_identifier | ||
.verify(|ident| ident.real_value() == "current"), |
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.
Let's use now()
instead of current()
, since now()
could be used in the batch query directly and process time temporal join is no longer using now
as key word.
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 for the Cargo.lock.
(cherry picked from commit d06bb47)
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR supports the syntax
FOR SYSTEM TIME AS OF NOW() - INTERVAL
for time travel query.To differentiate from temporal join'sAS OF NOW/PROCTIME
, time travel usesAS OF CURRENT
.Interval parser
is being reused. Time travel query restricts the interval to be value + unit.It also adds e2e tests for time travel query.
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
Support time travel query to access historical data at a specific points in time.