-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Update sqlparser to 0.19 #2981
Update sqlparser to 0.19 #2981
Conversation
SQLExpr::UnaryOp { op, expr } => match (&op, expr.as_ref()) { | ||
// The AST for Exists does not support the NOT EXISTS case so it gets | ||
// wrapped in a unary NOT | ||
// https://github.com/sqlparser-rs/sqlparser-rs/issues/472 |
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.
apache/datafusion-sqlparser-rs#472 is indeed fixed 🎉 (thanks @togami2864)
datafusion/sql/src/planner.rs
Outdated
@@ -2278,6 +2269,17 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |||
let mut result_days: i64 = 0; | |||
let mut result_millis: i64 = 0; | |||
|
|||
// Only handle string exprs for now | |||
let value = match &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.
I am not sure about this one -- now the arguments to INTERVAL
can be an arbitrary expression rather than just a String
(after apache/datafusion-sqlparser-rs#517)
cc @ovr any thoughts?
@@ -2184,7 +2178,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |||
|
|||
fn sql_interval_to_literal( | |||
&self, | |||
value: String, | |||
value: SQLExpr, |
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.
Intervals now support arbitrary expressions, not just strings: apache/datafusion-sqlparser-rs#517
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! @alamb
SInce I don't think this has any particularly controversial changes, I am going to merge it right in |
Thanks for the review @xudong963 |
Benchmark runs are scheduled for baseline = 811bad4 and contender = 2d23860. 2d23860 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2983
Rationale for this change
keep up with dependencies
What changes are included in this PR?
Update sqlparser pin
Are there any user-facing changes?
no