Skip to content

Don't lower case keywords#5897

Merged
harshit-gangal merged 1 commit intovitessio:masterfrom
systay:dont-lowercase-columns
Mar 9, 2020
Merged

Don't lower case keywords#5897
harshit-gangal merged 1 commit intovitessio:masterfrom
systay:dont-lowercase-columns

Conversation

@systay
Copy link
Copy Markdown
Collaborator

@systay systay commented Mar 6, 2020

Before this change, non-reserved keywords would get their case changed when parsed.

SELECT Status FROM Table

was parsed as:

SELECT `status` FROM Table

@systay systay requested a review from sougou as a code owner March 6, 2020 06:49
@systay
Copy link
Copy Markdown
Collaborator Author

systay commented Mar 6, 2020

This fixes #5821

Copy link
Copy Markdown
Member

@harshit-gangal harshit-gangal left a comment

Choose a reason for hiding this comment

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

LGTM

@sougou to confirm and merge.

Copy link
Copy Markdown
Contributor

@saifalharthi saifalharthi left a comment

Choose a reason for hiding this comment

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

Looks good

Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay systay force-pushed the dont-lowercase-columns branch from f11525b to 3f5196b Compare March 6, 2020 07:15
@morgo
Copy link
Copy Markdown
Contributor

morgo commented Mar 6, 2020

@systay I hate to suggest it - but you may need to add an option to temporarily revert the previous behavior. Apps that were built for Vitess may refer to columns in result sets in a case-sensitive way (they typically either do this or refer by ordinal position).

We can say the option is deprecated on-launch and will be removed in a future version. The default behavior should be as in this PR.

@harshit-gangal
Copy link
Copy Markdown
Member

@morgo The issue of lower casing was only with non reserved vitess keywords.
i.e for Other column names the case is maintained as-is.
If any application would have been case sensitive, then they either are using only lower case table column names or they are using case insensitive column name matching to retrieve the row information.

@morgo
Copy link
Copy Markdown
Contributor

morgo commented Mar 9, 2020

@harshit-gangal Sorry for my misunderstanding, that sounds better. Let's merge, and document as such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants