-
Notifications
You must be signed in to change notification settings - Fork 2.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: add missing MySQL 8.4 reserved keywords #17525
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
83dcc4c
to
3d3f413
Compare
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.
I think we maybe also want VECTOR
as a keyword, from MySQL 9.0.
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.
go/vt/sqlparser/keywords.go needs updating, and probably some other stuff
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17525 +/- ##
==========================================
+ Coverage 67.69% 67.70% +0.01%
==========================================
Files 1584 1584
Lines 254541 254509 -32
==========================================
+ Hits 172315 172322 +7
+ Misses 82226 82187 -39 ☔ View full report in Codecov by Sentry. |
I have no idea how |
32fabe4
to
0057167
Compare
Signed-off-by: Leopold Jacquot <[email protected]>
0057167
to
40b6a05
Compare
It's not reserved, but it is a keyword, and it is missing from the generated test file. |
@L3o-pold So one main thing. We should not add the keywords here to the Vitess side of reserved keywords. The reason is the following. The original bug here you reported is that the keyword is not escaped in the SQL that Vitess generates again. That is indeed since it doesn't know these as keywords. But we don't need reserved keywords here, they can be non-reserved keywords. This is because these words won't cause any conflict in the grammar, so they don't need to be reserved. This way we don't break existing queries that use these keywords also for people who are not on MySQL 8.4, as we only have one grammar for all MySQL versions. In the future we might also need to make it reserved once we add the actual parsing rules that use these keywords, but that would be a better time to also make them reserved because that at point there's actual feature support for something where it's needed. |
Ah yes, that's because the current generated list here is from 8.0 (or 8.4 in this PR), where this isn't added yet. I think we're ok for now with that and can update it once we generate it with the first 9.x LTS version instead. |
@L3o-pold Would have pushed the fix here, but I don't think that's allowed for the PR you opened here? |
@dbussink Github do not allow this for organisation fork. Do you mind creating your own PR and I close this one? |
Description
MySQL 8.4 introduce new reserved keywords: https://dev.mysql.com/doc/mysqld-version-reference/en/keywords-8-4.html#keywords-new-in-8-4
Related Issue(s)
Fixes #17523
Checklist
Deployment Notes