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

Test upgrade to sqlparser-rs 0.54 #14198

Closed
wants to merge 2 commits into from

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jan 19, 2025

Which issue does this PR close?

Rationale for this change

I want to test the upgrade to the latest sqlparser before we release a new version of sqlparser as a sanity check / test downstream implications

What changes are included in this PR?

  1. pin to pre-release version of sqlparser
  2. Update Planning for new 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 labels Jan 19, 2025
@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jan 19, 2025
@alamb alamb force-pushed the alamb/sqlparser_upgrade_test branch from 98938a9 to 8632bdd Compare January 19, 2025 14:27
@github-actions github-actions bot added the common Related to common crate label Jan 19, 2025
SQLExpr::Subscript { expr, subscript } => {
self.sql_subscript_to_expr(*expr, subscript, schema, planner_context)
}
SQLExpr::CompoundFieldAccess { root, access_chain } => self
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code to plan CompoundFieldAccess is directly copy/pasted from @goldmedal 's PR

@@ -44,7 +44,7 @@ Interval(MonthDayNano) Interval(MonthDayNano)
query ?
select interval '5' years
----
5.000000000 secs
60 mons
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 certainly seems much more correct to me -- I also removed the now out of date comment.

@alamb alamb force-pushed the alamb/sqlparser_upgrade_test branch from 8632bdd to 5bb20ad Compare January 19, 2025 15:12
///
/// For example, `foo.bar` would be represented as a two element vector
/// `["foo", "bar"]`
pub fn from_idents(mut idents: Vec<String>) -> Option<Self> {
Copy link
Contributor Author

@alamb alamb Jan 19, 2025

Choose a reason for hiding this comment

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

i made this public and cleaned up the signature so I could reuse it when dealing with the changes to USING planning

However I am not quite sure what a USING(foo.bar) would actually mean 🤔 Maybe when joining across multiple schemas...

I could also revert this change and just make the planner error if it got a multi-part ObjectName in a USING clause ..

@alamb
Copy link
Contributor Author

alamb commented Jan 23, 2025

Superceded by #14255

@alamb alamb closed this Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate logical-expr Logical plan and expressions sql SQL Planner sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant