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

fix alter table convert to charset default syntax #566

Merged
merged 2 commits into from
Nov 25, 2019

Conversation

sim41
Copy link
Contributor

@sim41 sim41 commented Sep 29, 2019

What problem does this PR solve?

issue:#498

MySQL Syntax:

alter_specification:
      table_options
    | [DEFAULT] CHARACTER SET [=] charset_name [COLLATE [=] collation_name]
    | CONVERT TO CHARACTER SET charset_name [COLLATE collation_name]

It used to parse the CONVERT TO CHARACTER SET chaset_name into [DEFAULT] CHARACTER SET syntax. I think we just want to change only the default character set for a table.
(https://dev.mysql.com/doc/refman/8.0/en/alter-table.html).
So I reserved it.

Bad SQL Case:

ALTER TABLE d_n.t_n CONVERT TO CHARSET DEFAULT
ALTER TABLE d_n.t_n CONVERT TO CHAR SET DEFAULT
ALTER TABLE d_n.t_n CONVERT TO CHARACTER SET DEFAULT
ALTER TABLE d_n.t_n CONVERT TO CHARACTER SET DEFAULT COLLATE utf8_bin

Check List

Tests

  • Unit test

@CLAassistant
Copy link

CLAassistant commented Sep 29, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Oct 8, 2019

Codecov Report

Merging #566 into master will not change coverage.
The diff coverage is 70%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #566   +/-   ##
======================================
  Coverage      80%     80%           
======================================
  Files          32      32           
  Lines       12681   12696   +15     
======================================
+ Hits        10145   10157   +12     
- Misses       1934    1935    +1     
- Partials      602     604    +2
Impacted Files Coverage Δ
parser.go 92.94% <ø> (+0.01%) ⬆️
ast/ddl.go 80.17% <70%> (-0.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5c49d1...6333e32. Read the comment docs.

@zier-one
Copy link
Contributor

zier-one commented Oct 9, 2019

@tiancaiamao PTAL

@zier-one
Copy link
Contributor

zier-one commented Oct 9, 2019

LGTM

@kennytm kennytm added the status/LGT1 LGT1 label Oct 9, 2019
@tiancaiamao
Copy link
Collaborator

PTAL @winkyao

@@ -1279,6 +1279,17 @@ AlterTableSpec:
}
$$ = op
}
| "CONVERT" "TO" CharsetKw "DEFAULT" OptCollate
Copy link
Member

Choose a reason for hiding this comment

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

It used to parse the CONVERT TO CHARACTER SET chaset_name into [DEFAULT] CHARACTER SET syntax. I think we just want to change only the default character set for a table...

So I reserved it.

I think we need to support all syntaxes rather than CONVERT TO CHARSET DEFAULT. Please update the codes here for specified character name.

BTW, the current implementation of ALTER TABLE ... DEFAULT CHARSET in TiDB is wrong, I think we need to fix it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sry, kind of busy these days. I will try to fix the TiDB to support the parser later. And the CONVERT TO CHARSET DEFAULT is another commits that has been merged. I will fix all the syntaxes as what MySQL do.

@bb7133
Copy link
Member

bb7133 commented Oct 11, 2019

hi @sim41 , thanks for the contribution, please check my comments.

@zier-one
Copy link
Contributor

hi @sim41 , could you check the comments and fix conflicts please?

@tiancaiamao
Copy link
Collaborator

Ping @sim41

@sim41 sim41 force-pushed the alter-convert-charset-default branch from 01c0cd0 to 9f9036c Compare October 25, 2019 08:04
@sim41 sim41 requested a review from a team October 25, 2019 08:04
@ghost ghost requested review from tangenta and removed request for a team October 25, 2019 08:04
@sim41
Copy link
Contributor Author

sim41 commented Oct 25, 2019

@tiancaiamao @tangenta sry, kind of busy these days. I will fix it this weekends.

@zier-one
Copy link
Contributor

zier-one commented Nov 8, 2019

@bb7133 PTAL

Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133 bb7133 added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels Nov 22, 2019
@bb7133
Copy link
Member

bb7133 commented Nov 22, 2019

hi @sim41 please resolve the conflict and we can approve your PR, thanks for the contribution!

@sim41 sim41 force-pushed the alter-convert-charset-default branch from 9f9036c to 6333e32 Compare November 22, 2019 08:42
@zier-one zier-one merged commit 5173e98 into pingcap:master Nov 25, 2019
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
lyonzhi pushed a commit to lyonzhi/parser that referenced this pull request Apr 25, 2024
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.

7 participants