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.

Sign up for GitHub

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

Jump to bottom

ddl: support altering the other charset to utf8 or utf8mb4 #8037

Merged
merged 28 commits into from
Dec 10, 2018
Merged

ddl: support altering the other charset to utf8 or utf8mb4 #8037

merged 28 commits into from
Dec 10, 2018

Conversation

winkyao
Copy link
Contributor

@winkyao winkyao commented Oct 24, 2018 •
edited by c4pt0r
Loading

What problem does this PR solve?

fix #7920.

Support changing the other charset to utf8 or utf8mb4.

Rules is:

  1. change any other charset to utf8mb4 is permitted, because TiDB treat all data as utf8mb4
  2. change any other charset to utf8 is permitted, except utf8mb4.

Before this PR, alter table charset take no effects, we just parse it and ignore, in this PR, we change the charset and collate of tableInfo in the infoSchema.

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change

Side effects

  • Increased code complexity

Related changes

  • Need to update the documentation
  • Need to be included in the release note

This change is Reviewable

@winkyao winkyao added component/DDL-need-LGT3 release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 24, 2018
@winkyao winkyao changed the title ddl: support alter the other charset to utf8 or utf8mb4 ddl: support altering the other charset to utf8 or utf8mb4 Oct 24, 2018
@shenli
Copy link
Member

shenli commented Oct 28, 2018

Please resolve the conflicts.

@winkyao winkyao mentioned this pull request Nov 5, 2018
Merged
@winkyao
Copy link
Contributor Author

winkyao commented Nov 5, 2018

Ci will be fixed, after the pr pingcap/parser#14 merged.

@tiancaiamao
Copy link
Contributor

Please make sure the change works
https://github.com/pingcap/parser#how-to-update-parser-for-tidb

@winkyao winkyao dismissed a stale review via bc7b5e3 December 3, 2018 02:22
@tiancaiamao
Copy link
Contributor

PTAL @ciscoxll @crazycs520 @zimulala

@winkyao
Copy link
Contributor Author

winkyao commented Dec 7, 2018

@zimulala PTAL

ciscoxll
ciscoxll reviewed Dec 7, 2018
View reviewed changes
Copy link
Contributor

@ciscoxll ciscoxll left a comment

Choose a reason for hiding this comment

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

Mostly LGTM

ciscoxll
ciscoxll reviewed Dec 7, 2018
View reviewed changes
Copy link
Contributor

@ciscoxll ciscoxll left a comment

Choose a reason for hiding this comment

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

LGTM

@ciscoxll ciscoxll added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 7, 2018
zimulala
zimulala reviewed Dec 7, 2018
View reviewed changes
ddl/ddl_api.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Show resolved Hide resolved
@winkyao
Copy link
Contributor Author

winkyao commented Dec 7, 2018

@zimulala PTAL

zimulala
zimulala reviewed Dec 10, 2018
View reviewed changes
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Dec 10, 2018
@zimulala
Copy link
Contributor

/run-all-tests

@winkyao
Copy link
Contributor Author

winkyao commented Dec 10, 2018

/run-common-test -tidb-test=pr/684

1 similar comment
@zhouqiang-cl
Copy link
Contributor

/run-common-test -tidb-test=pr/684

@winkyao
Copy link
Contributor Author

winkyao commented Dec 10, 2018

/run-integration-ddl-test -tidb-test=pr/684

@winkyao
Copy link
Contributor Author

winkyao commented Dec 10, 2018

/run-integration-common-test -tidb-test=pr/684

@winkyao
Copy link
Contributor Author

winkyao commented Dec 10, 2018

/run-integration-compatibility-test -tidb-test=pr/684

@winkyao winkyao merged commit 1d7d01c into pingcap:master Dec 10, 2018
@winkyao winkyao deleted the support_change_charset_utf8 branch December 10, 2018 04:58
@winkyao winkyao mentioned this pull request Dec 10, 2018
Merged
iamzhoug37 pushed a commit to iamzhoug37/tidb that referenced this pull request Dec 13, 2018
@winkyao @iamzhoug37
ddl: support altering the other charset to utf8 or utf8mb4 (pingcap#8037
d1749b6 
)
crazycs520 pushed a commit to crazycs520/tidb that referenced this pull request Jan 5, 2019
@winkyao @crazycs520
ddl: support altering the other charset to utf8 or utf8mb4 (pingcap#8037
cdfe76c 
)
@crazycs520 crazycs520 mentioned this pull request Jan 5, 2019
Merged
jackysp pushed a commit that referenced this pull request Jan 7, 2019
@crazycs520 @jackysp
ddl: support altering the other charset to utf8 or utf8mb4 (#8037) (#…
665db64 
…8951)
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiancaiamao tiancaiamao tiancaiamao left review comments

@jackysp jackysp jackysp left review comments

@ciscoxll ciscoxll ciscoxll left review comments

@crazycs520 crazycs520 crazycs520 left review comments

@zimulala zimulala zimulala approved these changes

Assignees
No one assigned
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change default charset and collation from utf8 to utf8mb4
9 participants
@winkyao @shenli @tiancaiamao @zimulala @zhouqiang-cl @jackysp @ciscoxll @crazycs520 @you06