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

Fix quoted identifier regression edge-case with "from" in SELECT #1346

Merged

Conversation

alexander-beedie
Copy link
Contributor

@alexander-beedie alexander-beedie commented Jul 18, 2024

#1212 introduced a regression in the recently released 0.48, disallowing the (valid) quoted identifier "from" in SELECT clauses, eg:

SELECT dt, rate, "to", "from" FROM fx_rates

Improved the check by additionally confirming that the potentially invalid from is not quoted, and added coverage.


FYI: this was caught by our (Polars) SQL unit tests while I was looking to upgrade us to 0.48 to take advantage of some recent PRs. Not sure if you usually make minor point releases to address regressions? If not, we can wait until 0.49 (or temporarily point to the fix on a branch), so no rush if that's the case 👍

@coveralls
Copy link

Pull Request Test Coverage Report for Build 9992710189

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 89.195%

Totals Coverage Status
Change from base Build 9944212602: 0.001%
Covered Lines: 26861
Relevant Lines: 30115

💛 - Coveralls

@alexander-beedie alexander-beedie changed the title Fix quoted identifier regression edge-case with "from" Fix quoted identifier regression edge-case with "from" in SELECT Jul 18, 2024
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you very much @alexander-beedie

cc @MohamedAbdeen21 as the author of #1212

@alexander-beedie I don't think we are setup to make a 0.48.1 patch release at this time, but I think I could make a small 0.49.0 release.

@alexander-beedie
Copy link
Contributor Author

@alexander-beedie I don't think we are setup to make a 0.48.1 patch release at this time, but I think I could make a small 0.49.0 release.

That would be great; whatever best fits your release model is fine by me 🙂

@MohamedAbdeen21
Copy link
Contributor

Nice catch. Thanks for fixing it

@alamb alamb mentioned this pull request Jul 21, 2024
4 tasks
@alamb alamb merged commit 71dc966 into apache:main Jul 21, 2024
10 checks passed
@alamb
Copy link
Contributor

alamb commented Jul 21, 2024

Thanks again -- I'll try and release the next version sometime this week: #1332

@alexander-beedie alexander-beedie deleted the fix-quoted-identifier-regression branch July 21, 2024 10:04
@alexander-beedie
Copy link
Contributor Author

alexander-beedie commented Jul 21, 2024

Thanks again -- I'll try and release the next version sometime this week: #1332

Fantastic; will finish integrating the new code on our side once it's out.

@alamb
Copy link
Contributor

alamb commented Jul 23, 2024

This fix has been published as part of 0.49.0 which is on crates.io: https://crates.io/crates/sqlparser/0.49.0

@alexander-beedie
Copy link
Contributor Author

This fix has been published as part of 0.49.0 which is on crates.io: https://crates.io/crates/sqlparser/0.49.0

Many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants