-
Notifications
You must be signed in to change notification settings - Fork 601
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
Conversation
src/tokenizer.rs
Outdated
.dialect | ||
.nested_quote_start(quote_start, chars.peekable.clone()) | ||
{ | ||
chars.next(); // consume the opening quote |
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.
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.
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.
Thanks @7phs! Left some minor comments I think the approach looks reasonable!
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.
🚀
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! Thanks @7phs @bombsimon
cc @alamb
Quoted identifiers allow the use of any characters and can start with any character, including digits. For example:
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.