Skip to content
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

Upgrade to sqlparser 0.53.0 #13767

Merged
merged 7 commits into from
Dec 20, 2024
Merged

Upgrade to sqlparser 0.53.0 #13767

merged 7 commits into from
Dec 20, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Dec 13, 2024

Which issue does this PR close?

Rationale for this change

Keep up with dependencies (in this case get access to source spans)

What changes are included in this PR?

  1. Upgrade to sqlparser 0.53.0
  2. Update APIs

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) common Related to common crate labels Dec 13, 2024
@alamb alamb marked this pull request as draft December 13, 2024 19:57
@alamb alamb force-pushed the alamb/sqlparser_0.53.0 branch from 6dc5055 to 3e521f1 Compare December 14, 2024 01:28
@@ -200,7 +204,6 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
statement: Statement,
planner_context: &mut PlannerContext,
) -> Result<LogicalPlan> {
let sql = Some(statement.to_string());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was needed because otherwise tpcds_logical_q64 overflowed its stack

https://github.com/apache/datafusion/actions/runs/12322166828/job/34395679472?pr=13767

thread 'tpcds_logical_q64' has overflowed its stack
fatal runtime error: stack overflow
error: test failed, to rerun pass `-p datafusion --test tpcds_planning`

I traced it down locally and apparently trying to format the deeply nested query was actually overflowing at this spot.

@github-actions github-actions bot added optimizer Optimizer rules proto Related to proto crate labels Dec 14, 2024
@@ -514,6 +517,35 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
return not_impl_err!("To not supported")?;
}

// put the statement back together temporarily to get the SQL
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this formulation is more code, it does avoid serializing the entire input string on every query, even when it is not needed

@@ -313,7 +313,7 @@ pub enum Expr {
/// plan into physical plan.
Wildcard {
qualifier: Option<TableReference>,
options: WildcardOptions,
options: Box<WildcardOptions>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was needed to avoid a clippy lint about enum variants being too different. Specifically sqlparser added some new fields to WildCardOptions which made them substantially larger

@alamb alamb force-pushed the alamb/sqlparser_0.53.0 branch from 239c18e to 1e8d072 Compare December 14, 2024 11:08
@alamb alamb force-pushed the alamb/sqlparser_0.53.0 branch from 1e8d072 to bd101da Compare December 19, 2024 10:58
@alamb alamb marked this pull request as ready for review December 19, 2024 11:10
@alamb alamb force-pushed the alamb/sqlparser_0.53.0 branch from 504c04d to 3ae39a5 Compare December 19, 2024 11:12
} => self.show_columns_to_plan(extended, full, table_name, filter),
show_options,
} => {
let ShowStatementOptions {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this checks @alamb I'm wondering if it caused by sql parser API extension or it technical debt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was caused by changes in the SQL Parser AST -- specifically I think it was introduced in

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm thanks @alamb

@alamb
Copy link
Contributor Author

alamb commented Dec 20, 2024

Thanks for the reviews @comphead (as always). Let's keep this code flowing 🚀

@alamb alamb merged commit 75202b5 into apache:main Dec 20, 2024
27 checks passed
@alamb alamb deleted the alamb/sqlparser_0.53.0 branch December 22, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules proto Related to proto crate sql SQL Planner sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants