feat: Upgrade to sqlparser-rs 0.62.0#22069
Conversation
Bump from crates.io 0.61.0 to git main rev 9833c033bc44c... to gain access to SparkSqlDialect ahead of the next sqlparser-rs release. Notable upstream changes absorbed: - Lambda::params is now OneOrManyWithParens<LambdaFunctionParameter>; reject typed lambda parameters (not_impl_err) since DataFusion does not model parameter data types. - Insert::columns is now Vec<ObjectName>; reject multi-part column names in INSERT (not_impl_err). - ExcludeSelectItem::Single/Multiple now carry ObjectName; reject multi-part EXCLUDE identifiers (plan_err). - New SelectItem::ExprWithAliases, BeginTransactionKind::Tran, TableConstraint::PrimaryKeyUsingIndex/UniqueUsingIndex, TableObject::TableQuery variants -> not_impl_err. - WildcardAdditionalOptions gained opt_alias -> not_impl_err. - VALUES rows are now Vec<Parens<Vec<Expr>>>; iterate via .content. - CREATE FUNCTION return_type is now Option<FunctionReturnType> (DataType vs SetOf); reject SETOF. - escape_char in Like/ILike/SimilarTo is now Option<ValueWithSpan>. - TableAlias gained `at: Option<Ident>`; CreateTable, CreateView, Insert, Update, Delete, Select, TableFactor::Function gained several new fields with `not_impl_err!` for unsupported ones. Update higher_order.slt golden error messages to match new LambdaFunctionParameter Debug formatting.
github-actions
Bot
added
sql
| // Declared outside the `if let` so `idents.extend(exclude_owned.iter())` | ||
| // below can borrow references that outlive the inner scope. | ||
| #[cfg(feature = "sql")] | ||
| let exclude_owned: Vec<sqlparser::ast::Ident>; | ||
| if let Some(exclude) = opt_exclude { | ||
| #[cfg(feature = "sql")] |
There was a problem hiding this comment.
I'm not too sure about this feature gating, tbh.
This is what Claude suggested
There was a problem hiding this comment.
Claude says:
Here's what's happening and why the cfg(feature = "sql") gates exist:
The setup. datafusion-expr has a sql feature (on by default) that depends on sqlparser. When sql is enabled, ExcludeSelectItem is re-exported from sqlparser::ast. When it's disabled, expr.rs provides its
own fallback enum with Ident payloads (datafusion/expr/src/expr.rs:1431-1436). The two paths in utils.rs already exist via the existing #[cfg(...)] imports at lines 41-45.
What sqlparser 0.62 changed. In 0.61, ExcludeSelectItem::Single carried Ident and Multiple carried Vec. In 0.62, both now carry ObjectName (a possibly multi-part name like schema.column). The
fallback enum in expr.rs still uses Ident because that crate-local copy hasn't changed.
There was a problem hiding this comment.
I suggest we should update the fallback copy in DataFusion to match the sql parser one.
For example
datafusion/datafusion/expr/src/expr.rs
Lines 1452 to 1457 in 658827d
That will avoid the need to have two copies
Looks like this copy came in via #17332 from @timsaucer
Maybe we can add some comments explaining the rationale for the second copy better (any isolate the cfg sql somewhere to make it clearer)
There was a problem hiding this comment.
I was trying to understand it. My understanding of this, please correct if I'm wrong is supporting select * exclude(col) using sqlparser(sql feature is on) and using Datafuiosn, specifically referring to ExceptSelectItem. Why we need to keep parts of parser in DF? Maybe I'm missing something
There was a problem hiding this comment.
Why can't we have unified approach, to use parser or use DataFusion, did they diverge at some point?
|
Thanks @andygrove I'll review the PR tomorrow |
| // Declared outside the `if let` so `idents.extend(exclude_owned.iter())` | ||
| // below can borrow references that outlive the inner scope. | ||
| #[cfg(feature = "sql")] | ||
| let exclude_owned: Vec<sqlparser::ast::Ident>; | ||
| if let Some(exclude) = opt_exclude { | ||
| #[cfg(feature = "sql")] |
There was a problem hiding this comment.
I suggest we should update the fallback copy in DataFusion to match the sql parser one.
For example
datafusion/datafusion/expr/src/expr.rs
Lines 1452 to 1457 in 658827d
That will avoid the need to have two copies
Looks like this copy came in via #17332 from @timsaucer
Maybe we can add some comments explaining the rationale for the second copy better (any isolate the cfg sql somewhere to make it clearer)
3 participants
Which issue does this PR close?
N/A
Rationale for this change
Dependency update
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?