-
Notifications
You must be signed in to change notification settings - Fork 1.8k
chore(deps): Update sqlparser to 0.55.0 #15183
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
Conversation
|
Amazing! Thank you @PokIsemaine -- I'll try and review this sometime this week FYI @jonahgao |
jonahgao
left a comment
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.
Thank you @PokIsemaine. I reviewed part of the changes. I think some unwraps can be replaced with errors to prevent panic.
| } | ||
| } | ||
|
|
||
| pub(super) fn sql_case_identifier_to_expr( |
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.
Not related to this PR, why is it called case_identifier🤔
I'm not sure what the connection is between case and identifier.
jonahgao
left a comment
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. I think we can merge this as it is and improve it in follow-up PRs.
| .map(|object_name_part| { | ||
| object_name_part | ||
| .as_ident() | ||
| .map_or_else(String::new, ident_to_string) |
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 might be better to return an error than to silently use a default value.
* chore(deps): Update sqlparser to 0.55.0 * merge upstream * style: cargo fmt * fix: import * style: cargo fmt * fix: struct.slt, update.slt and lateral join * chore: cargo fmt & remove unwrap * fix: remove meaningless comment for rustfmt * refactor: plan_datafusion_err, remove clone * remove println --------- Co-authored-by: silezhou <zhousile2002@gmail.com> Co-authored-by: jonahgao <jonahgao@msn.com>
|
Thank you so much @jonahgao and @PokIsemaine |
* chore(deps): Update sqlparser to 0.55.0 * merge upstream * style: cargo fmt * fix: import * style: cargo fmt * fix: struct.slt, update.slt and lateral join * chore: cargo fmt & remove unwrap * fix: remove meaningless comment for rustfmt * refactor: plan_datafusion_err, remove clone * remove println --------- Co-authored-by: silezhou <zhousile2002@gmail.com> Co-authored-by: jonahgao <jonahgao@msn.com>
Which issue does this PR close?
Rationale for this change
Keep up with latest SQL parser dependencies
What changes are included in this PR?
Note
OrderBy: Add support for
ORDER BY ALLdatafusion-sqlparser-rs#1724I will create an issue for ORDER BY ALL and implement it later. Like
GROUP BY ALL, this requires obtaining select_exprs.struct.slt: user => user_infomation. I found that in the struct,useris parsed as aFunctionrather than anIdent.feat: Parse special keywords as functions (current_user, user, etc) datafusion-sqlparser-rs#561
update.slt: sqlparser already supports multiple tables in the UPDATE FROM clause, but DataFusion does not yet support it. Support multiple tables inUPDATE FROMclause datafusion-sqlparser-rs#1681ref
EXECUTE IMMEDIATEdatafusion-sqlparser-rs#1717BEGINdatafusion-sqlparser-rs#1718Are these changes tested?
Yes by CI
Are there any user-facing changes?