Skip to content

Redshift: Fix parsing for quoted numbered columns #1576

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

Merged
merged 12 commits into from
Dec 15, 2024

Conversation

7phs
Copy link
Contributor

@7phs 7phs commented Dec 3, 2024

Quoted identifiers allow the use of any characters and can start with any character, including digits. For example:

SELECT 1 AS "1" FROM a

However, the tokenizer cannot parse such types of quoted identifiers in the case of Redshift. This issue is related to the handling of potential identifiers in PartiQL.

This merge request (MR) clarifies the behavior of quoted identifiers in PartiQL and fixes the parsing of quoted identifiers for Redshift.

I added an additional case of a valid JSON path in a specific unit test to ensure that JSON path parsing is not broken.

aleksei.p added 2 commits December 3, 2024 15:07
@7phs 7phs requested a review from bombsimon December 4, 2024 09:22
src/tokenizer.rs Outdated
.dialect
.nested_quote_start(quote_start, chars.peekable.clone())
{
chars.next(); // consume the opening quote
Copy link
Contributor Author

@7phs 7phs Dec 5, 2024

Choose a reason for hiding this comment

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

This code is RedShift specific (nested quotation). The same time tokenisation of quoted identifier is a method of tokeniser and this is a reason of implemented it here.

Let me know if you have a good idea how makes this code more explicit.

@7phs 7phs requested a review from bombsimon December 5, 2024 00:46
aleksei.p added 2 commits December 5, 2024 20:56
@7phs 7phs requested a review from iffyio December 5, 2024 20:20
Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

Thanks @7phs! Left some minor comments I think the approach looks reasonable!

@7phs 7phs requested a review from iffyio December 6, 2024 14:02
Copy link
Contributor

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

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

🚀

@7phs 7phs requested a review from iffyio December 7, 2024 11:14
Copy link
Contributor

@iffyio iffyio 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 @7phs @bombsimon
cc @alamb

@iffyio iffyio merged commit 7867ba3 into apache:main Dec 15, 2024
8 checks passed
@iffyio iffyio deleted the redshift_numbered_columns branch December 15, 2024 09:56
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.

3 participants