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

ddl: fix alter table charset bug that change blob column type to text #10477

Merged
merged 10 commits into from Jun 4, 2019

Conversation

Projects
None yet
5 participants
@crazycs520
Copy link
Contributor

commented May 15, 2019

What problem does this PR solve?

Alter table charset will change blob column to text column. just like below.

Before

tidb> create table t1 (a blob) character set utf8;
Query OK, 0 rows affected
Time: 0.005s
tidb> alter table t1 charset=utf8mb4 collate=utf8mb4_bin;
Query OK, 0 rows affected
Time: 0.005s
tidb> show create table t1
+-------+-------------------------------------------------------------+
| Table | Create Table                                                |
+-------+-------------------------------------------------------------+
| t1    | CREATE TABLE `t1` (                                         |
|       |   `a` text DEFAULT NULL                                     |
|       | ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |
+-------+-------------------------------------------------------------+

What is changed and how it works?

  • fix Alter table column bug.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

Related changes

  • Need to cherry-pick to the release branch
@zhouqiang-cl

This comment has been minimized.

Copy link
Member

commented May 15, 2019

/rebuild

@codecov

This comment has been minimized.

Copy link

commented May 15, 2019

Codecov Report

Merging #10477 into master will decrease coverage by 0.0136%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##            master     #10477        +/-   ##
===============================================
- Coverage   78.354%   78.3403%   -0.0137%     
===============================================
  Files          414        414                
  Lines        87730      87730                
===============================================
- Hits         68740      68728        -12     
- Misses       13849      13856         +7     
- Partials      5141       5146         +5
@@ -735,6 +735,9 @@ func onModifyTableCharsetAndCollate(t *meta.Meta, job *model.Job) (ver int64, _
tblInfo.Collate = toCollate
// update column charset.
for _, col := range tblInfo.Columns {
if col.Charset == charset.CharsetBin {

This comment has been minimized.

Copy link
@winkyao

winkyao May 15, 2019

Member

We can use HasCharset here.

@@ -735,7 +736,7 @@ func onModifyTableCharsetAndCollate(t *meta.Meta, job *model.Job) (ver int64, _
tblInfo.Collate = toCollate
// update column charset.
for _, col := range tblInfo.Columns {
if typesNeedCharset(col.Tp) {
if field_types.HasCharset(&col.FieldType) {

This comment has been minimized.

Copy link
@bb7133

bb7133 May 15, 2019

Contributor

I guess typesNeedCharset can be replaced by HasCharset. Another code that calls this function is:

if typesNeedCharset(tp.Tp) {

Can you confirm it here?

This comment has been minimized.

Copy link
@crazycs520

crazycs520 May 17, 2019

Author Contributor

No,

if typesNeedCharset(tp.Tp) {
here may be have not set the column Flag, use HasCharset here may be got some error.

This comment has been minimized.

Copy link
@winkyao

winkyao May 20, 2019

Member

so why you use HashCharset finally?

This comment has been minimized.

Copy link
@crazycs520

crazycs520 May 21, 2019

Author Contributor

HashCharset can be used here(ddl/table.go #739), but ddl/ddl_api.go #299 should use typesNeedCharset function.

This comment has been minimized.

Copy link
@zimulala

zimulala Jun 4, 2019

Member

Sorry, I still don't quite understand, why is it correct to change to HasCharset?

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

/run-all-tests

1 similar comment
@zhouqiang-cl

This comment has been minimized.

Copy link
Member

commented May 20, 2019

/run-all-tests

@zhouqiang-cl

This comment has been minimized.

Copy link
Member

commented May 20, 2019

/rebuild

@bb7133
Copy link
Contributor

left a comment

LGTM

However, obviously, we should not judge column type from their charsets. It should be corrected in some other PRs.

@winkyao
Copy link
Member

left a comment

LGTM

crazycs520 added some commits May 21, 2019

@zimulala
Copy link
Member

left a comment

LGTM

@zimulala

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

/run-all-tests

@zimulala zimulala added the status/LGT3 label Jun 4, 2019

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

/run-all-tests

@crazycs520 crazycs520 merged commit c80a79c into pingcap:master Jun 4, 2019

14 checks passed

ci/circleci Your tests passed on CircleCI!
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.