Update to sqlparser-rs v0.50.0
#12014
Conversation
| @@ -268,6 +268,7 @@ pub(crate) fn value_to_string(value: &Value) -> Option<String> { | |||
| Value::SingleQuotedString(s) => Some(s.to_string()), | |||
| Value::DollarQuotedString(s) => Some(s.to_string()), | |||
| Value::Number(_, _) | Value::Boolean(_) => Some(value.to_string()), | |||
| Value::UnicodeStringLiteral(s) => Some(s.to_string()), | |||
There was a problem hiding this comment.
I don't really understand why value_to_string supports (returns Some(String)) for some types of string, but not others, so I'm not clear whether Value::UnicodeStringLiteral should return Some or None?
There was a problem hiding this comment.
I suspect this was not carefully designed and likely is not an intentional oversight
There was a problem hiding this comment.
I think what you have here is ok
|
Waiting for decision on apache/datafusion-sqlparser-rs#1382. |
|
BTW I double checked with this branch that you can still use $ git status
On branch sqlparser-main
nothing to commit, working tree clean
~/Software/datafusion/datafusion-cli$ cargo run
...> create table foo(id int) as values (1);
0 row(s) fetched.
Elapsed 0.030 seconds.
> select id from foo;
+----+
| id |
+----+
| 1 |
+----+
1 row(s) fetched.
Elapsed 0.007 seconds.
> |
Making this commit just to let tests progress.
There was a problem hiding this comment.
Thank you @samuelcolvin -- this looks great to me. I had some style suggestions, but they aren't required from my perspective to merge this PR
| @@ -178,7 +178,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |||
| SQLExpr::Value(value) => { | |||
| self.parse_value(value, planner_context.prepare_param_data_types()) | |||
| } | |||
| SQLExpr::Extract { field, expr } => { | |||
| SQLExpr::Extract { field, expr, .. } => { | |||
There was a problem hiding this comment.
I recommend explicitly listing out the other fields here and returning a NotImplemented error if there is some new syntax that DataFusion doesn't support.
Otherwise what we have seen a few times in the past is that the parser accepts some new syntax element but DataFusion just igores it silently
| @@ -438,7 +438,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |||
| } | |||
| SQLDataType::Bytea => Ok(DataType::Binary), | |||
| SQLDataType::Interval => Ok(DataType::Interval(IntervalUnit::MonthDayNano)), | |||
| SQLDataType::Struct(fields) => { | |||
| SQLDataType::Struct(fields, _) => { | |||
There was a problem hiding this comment.
Likewise here I think it would be good to name the field like
| SQLDataType::Struct(fields, _) => { | |
| SQLDataType::Struct(fields, _bracket_type) => { |
So it is easier to understand by reading just the code what is being ignored
| table_name, | ||
| .. |
There was a problem hiding this comment.
per comment above, I recommend keeping the fields named explcitly
| @@ -268,6 +268,7 @@ pub(crate) fn value_to_string(value: &Value) -> Option<String> { | |||
| Value::SingleQuotedString(s) => Some(s.to_string()), | |||
| Value::DollarQuotedString(s) => Some(s.to_string()), | |||
| Value::Number(_, _) | Value::Boolean(_) => Some(value.to_string()), | |||
| Value::UnicodeStringLiteral(s) => Some(s.to_string()), | |||
There was a problem hiding this comment.
I think what you have here is ok
| @@ -848,8 +848,10 @@ SELECT EXTRACT("year" FROM timestamp '2020-09-08T12:00:00+00:00') | |||
| ---- | |||
| 2020 | |||
|
|
|||
| query error | |||
| query R | |||
|
Thanks again @samuelcolvin |
Which issue does this PR close?
NA
Rationale for this change
Support latest features of
sqlparser-rs.What changes are included in this PR?
Uprev
sqlparser-rsto v0.50.0Are these changes tested?
No new tests needed AFAIK.
Are there any user-facing changes?
Some new SQL syntax supported.