-
Notifications
You must be signed in to change notification settings - Fork 1k
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: include lower-case identifiers among those that need quotes #3723
Conversation
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 - it'd be nice to have a description explaining the background of why we did this/how you discovered it (so that in the future if I git blame and go the PR I see the background)
0d31c22
to
3e0fac4
Compare
IdentifierUtils::needsQuotes is used by various code that formats SQL text to decide what identifiers to wrap in quotes. Before this change, this just included reserved identifiers. However, identifiers that include lower-case characters should also be quoted. Otherwise, when the parser parses them it will convert them to upper-case. So this patch changes needsQuotes to check for lower-case characters. For schema inference, we don't want to quote column names generated from lower-case avro schema fields. To handle this, this patch adds IdentifierUtils.isValid, that only returns true if the identifier is valid (parsable). Schema inference uses this to only quote invalid identifiers.
3e0fac4
to
113eedb
Compare
…fluentinc#3723) IdentifierUtils::needsQuotes is used by various code that formats SQL text to decide what identifiers to wrap in quotes. Before this change, this just included reserved identifiers. However, identifiers that include lower-case characters should also be quoted. Otherwise, when the parser parses them it will convert them to upper-case. So this patch changes needsQuotes to check for lower-case characters. For schema inference, we don't want to quote column names generated from lower-case avro schema fields. To handle this, this patch adds IdentifierUtils.isValid, that only returns true if the identifier is valid (parsable). Schema inference uses this to only quote invalid identifiers.
…) (#5139) IdentifierUtils::needsQuotes is used by various code that formats SQL text to decide what identifiers to wrap in quotes. Before this change, this just included reserved identifiers. However, identifiers that include lower-case characters should also be quoted. Otherwise, when the parser parses them it will convert them to upper-case. So this patch changes needsQuotes to check for lower-case characters. For schema inference, we don't want to quote column names generated from lower-case avro schema fields. To handle this, this patch adds IdentifierUtils.isValid, that only returns true if the identifier is valid (parsable). Schema inference uses this to only quote invalid identifiers. Co-authored-by: Rohan <desai.p.rohan@gmail.com>
IdentifierUtils::needsQuotes
is used by various code that formats SQL text to decide what identifiers to wrap in quotes. Before this change, this just included reserved identifiers. However, identifiers that include lower-case characters should also be quoted. Otherwise, when the parser parses them it will convert them to upper-case.