Skip to content
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

Unreserved 8 keywords #851

Merged
merged 3 commits into from
May 12, 2020
Merged

Unreserved 8 keywords #851

merged 3 commits into from
May 12, 2020

Conversation

kennytm
Copy link
Contributor

@kennytm kennytm commented May 10, 2020

What problem does this PR solve?

Fix #846.

What is changed and how it works?

Unreserved the following 8 keywords:

  • chain
  • error
  • general
  • nvarchar
  • pack_keys
  • parser
  • shard_row_id_bits
  • pre_split_regions

And added a manually executed test case to ensure the list of reserved keywords mostly match that of MySQL. The only discrepancy currently are:

  • current_role — reserved in TiDB, not reserved in MySQL (introduced by parser, ast: add SET ROLE support #228, caused significant reduce/reduce conflict when unreserved, cc @imtbkcat)
  • function — reserved since MySQL 8.0.1, not reserved in TiDB
  • separator — reserved in MySQL (why?), not reserved in TiDB

Check List

Tests

  • Unit test

Code changes

Side effects

Related changes

  • Need to cherry-pick to the release branch
    • Affects all TiDB versions.

Release note

  • CHAIN, ERROR, GENERAL, NVARCHAR, PACK_KEYS, PARSER, SHARD_ROW_ID_BITS and PRE_SPLIT_REGIONS are no longer treated as reserved words.

@codecov
Copy link

codecov bot commented May 10, 2020

Codecov Report

Merging #851 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #851   +/-   ##
=======================================
  Coverage   78.28%   78.28%           
=======================================
  Files          40       40           
  Lines       14667    14667           
=======================================
  Hits        11482    11482           
  Misses       2502     2502           
  Partials      683      683           

Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

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

LGTM

@tangenta tangenta added the status/LGT1 LGT1 label May 11, 2020
@tiancaiamao
Copy link
Collaborator

shard_row_id_bits and pre_split_regions, should them be unreserved-keyword or non-keyword?

@kennytm
Copy link
Contributor Author

kennytm commented May 12, 2020

hmm what's the difference between UnreservedKeyword, NotKeyword and TiDBKeyword?

@tiancaiamao
Copy link
Collaborator

tiancaiamao commented May 12, 2020

hmm what's the difference between UnreservedKeyword, NotKeyword and TiDBKeyword?

I'm not sure there is any difference between NonKeyword and UnreservedKeyword.
NonKeyword is solely an identifier, whereas UnreservedKeyword can alse be used just like an identifier (no need to warp with `), so I can't tell apart them.

https://dev.mysql.com/doc/refman/5.7/en/keywords.html

If you are not sure as well, I think we can merge this change. Anyway, nobody knows or cares about it. @kennytm

@tiancaiamao
Copy link
Collaborator

LGTM

@tiancaiamao tiancaiamao added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels May 12, 2020
@kennytm kennytm merged commit fa89057 into master May 12, 2020
@kennytm kennytm deleted the kennytm/error-is-not-a-keyword branch May 12, 2020 06:06
@kennytm
Copy link
Contributor Author

kennytm commented May 12, 2020

/run-cherry-picker

@sre-bot
Copy link

sre-bot commented May 12, 2020

cherry pick to release-3.0 failed

1 similar comment
@sre-bot
Copy link

sre-bot commented May 12, 2020

cherry pick to release-3.0 failed

@sre-bot
Copy link

sre-bot commented May 12, 2020

cherry pick to release-3.1 failed

1 similar comment
@sre-bot
Copy link

sre-bot commented May 12, 2020

cherry pick to release-3.1 failed

@sre-bot
Copy link

sre-bot commented May 12, 2020

cherry pick to release-4.0 failed

@sre-bot
Copy link

sre-bot commented May 12, 2020

cherry pick to release-4.0 failed

@kennytm
Copy link
Contributor Author

kennytm commented May 12, 2020

okay.

tangenta pushed a commit to tangenta/parser that referenced this pull request Jun 5, 2020
* parser.y: unreserve 8 keywords

* consistent_test: added an optional test to check against MySQL keywords
tangenta pushed a commit to tangenta/parser that referenced this pull request Jun 5, 2020
* parser.y: unreserve 8 keywords

* consistent_test: added an optional test to check against MySQL keywords
tangenta added a commit that referenced this pull request Jun 9, 2020
* parser.y: unreserve 8 keywords

* consistent_test: added an optional test to check against MySQL keywords

Co-authored-by: kennytm <[email protected]>
@lysu
Copy link
Collaborator

lysu commented Jul 15, 2020

hi @kennytm could please help cherry-pick this to 3.0 and 3.1, some customers meet this problem too in 3.0, thanks~

kennytm added a commit that referenced this pull request Jul 15, 2020
* parser.y: unreserve 8 keywords

* consistent_test: added an optional test to check against MySQL keywords
kennytm added a commit that referenced this pull request Jul 15, 2020
* parser.y: unreserve 8 keywords

* consistent_test: added an optional test to check against MySQL keywords
@kennytm
Copy link
Contributor Author

kennytm commented Jul 15, 2020

@lysu #928, #929.

kennytm added a commit that referenced this pull request Jul 16, 2020
* parser.y: unreserve 8 keywords

* consistent_test: added an optional test to check against MySQL keywords
kennytm added a commit that referenced this pull request Jul 16, 2020
* parser.y: unreserve 8 keywords

* consistent_test: added an optional test to check against MySQL keywords
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
* parser.y: unreserve 8 keywords

* consistent_test: added an optional test to check against MySQL keywords
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.

SQL syntax parsing exception when the field name in the SQL statement is error
5 participants