Update to sqlparser-rs v0.50.0
#12014
Conversation
| 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.
alamb
left a comment
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
| 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
| 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
| describe_alias: DescribeAlias::Describe, // only parse 'DESCRIBE table_name' and not 'EXPLAIN table_name' | ||
| hive_format: _, | ||
| table_name, | ||
| .. |
There was a problem hiding this comment.
per comment above, I recommend keeping the fields named explcitly
| 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
| 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.