ddl : fix column collate should use table's if it has when create table or alter table. by AilinKid · Pull Request #13119 · pingcap/tidb

AilinKid · 2019-11-04T09:16:36Z

What problem does this PR solve?

fix issue #13113
When creating a table, if we set a collate for this table, all columns in this table should use this collate instead of using default collates.
In MySQL:

mysql> show create table t;
+-------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                                                                                                                                                 |
+-------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| t     | CREATE TABLE `t` (
  `id` int(11) NOT NULL,
  `c1` varchar(10) COLLATE utf8_slovak_ci DEFAULT NULL,
  `c2` varchar(20) COLLATE utf8_slovak_ci DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_slovak_ci |
+-------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

In TiDB

mysql> create table t (id int(11) primary key ,c1 varchar(10) ,c2 varchar(20)) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_slovak_ci;
Query OK, 0 rows affected (0.01 sec)

mysql> show create table t;
+-------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                                                                                                                                     |
+-------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| t     | CREATE TABLE `t` (
  `id` int(11) NOT NULL,
  `c1` varchar(10) COLLATE utf8_bin DEFAULT NULL,
  `c2` varchar(20) COLLATE utf8_bin DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_slovak_ci |
+-------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

What is changed and how it works?

Table's collate should be passed into buildColumnsAndConstraints when in create table or alter table.

Check List

Tests

Unit test
Integration test

Code changes

add parameter for buildColumnsAndConstraints function.

Related changes

Need to cherry-pick to the release branch

Release note

fix column collate should use table's if it has when create table or alter table.

codecov · 2019-11-04T09:27:51Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a28fc71). Click here to learn what that means.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #13119   +/-   ##
===========================================
  Coverage          ?   80.7511%           
===========================================
  Files             ?        468           
  Lines             ?     114417           
  Branches          ?          0           
===========================================
  Hits              ?      92393           
  Misses            ?      15134           
  Partials          ?       6890

AilinKid · 2019-11-04T10:23:38Z

/rebuild

AilinKid · 2019-11-04T10:43:39Z

/run-unit-test

crazycs520 · 2019-11-05T01:31:49Z

ddl/db_change_test.go

 		if job.SchemaState != model.StatePublic {
-			result := tk.MustQuery("show create table t")
+			var result *testkit.Result
+			if job.Query[12:14] == "t2" {


How about use t2 := testGetTableByName(c, tk.Se, "test", "t2") then compare the job.TableID == t2.Meta().ID

crazycs520

REST LGTM

ddl/ddl_api.go

ddl/db_change_test.go

 		if job.SchemaState != model.StatePublic {
-			result := tk.MustQuery("show create table t")
+			var result *testkit.Result
+			if job.Query[12:14] == "t2" {


ddl/ddl_api.go

tangenta · 2019-11-05T03:59:05Z

ddl/ddl_api.go

 	}
-
-	return tableCharset
+	return tableCharset, tableCollate


I think using two loops is more expressive when there is little impact on the performance.

there is one loop with two if judgment.

wjhuang2016

LGTM

tangenta

LGTM

crazycs520

LGTM

crazycs520 · 2019-11-05T13:28:21Z

/run-all-tests

AilinKid · 2019-11-06T02:28:28Z

this pr need fix mysql_test as well

AilinKid · 2019-11-06T02:51:30Z

/run-common-test

AilinKid · 2019-11-06T02:52:02Z

/run-integration-common-test

sre-bot · 2019-11-06T03:17:23Z

cherry pick to release-3.0 in PR #13174

sre-bot · 2019-11-06T03:18:50Z

cherry pick to release-3.1 in PR #13175

sre-bot · 2019-11-06T03:19:39Z

cherry pick to release-2.1 failed

bb7133 · 2019-11-06T03:26:09Z

ddl/ddl_api.go


-func findTableOptionCharset(options []*ast.TableOption) string {
-	var tableCharset string
+func findTableOptionCharsetAndCollate(options []*ast.TableOption) (tableCharset, tableCollate string) {


Please use this function instead:

tidb/ddl/ddl_api.go

Line 1841 in aaaf254

func getCharsetAndCollateInTableOption(startIdx int, options []*ast.TableOption) (ca, co string, err error) {

I think we can remove findTableOptionCharsetAndCollate

…le or alter table. (pingcap#13119) fix column's collation should use table's collation

…le or alter table. (#13119 & #12327) (#13174)

…le or alter table. (#13119 & #12327) (#13175)

…le or alter table. (#13119 & #12327) (#13190)

…le or alter table. (pingcap#13119) fix column's collation should use table's collation

sre-bot · 2020-04-07T10:24:50Z

It seems that, not for sure, we failed to cherry-pick this commit to release-2.1 release-3.0. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @AilinKid PTAL.

fix column collate should use table's if it has

69eee73

AilinKid added type/bugfix This PR fixes a bug. needs-cherry-pick-2.1 labels Nov 4, 2019

Merge branch 'master' into fix_collate_same_with_table

b07371d

Merge branch 'master' into fix_collate_same_with_table

b01b7ea

crazycs520 reviewed Nov 5, 2019

View reviewed changes

use testGetTableByName to get table info

7bb3aa4

AilinKid added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 5, 2019

wjhuang2016 reviewed Nov 5, 2019

View reviewed changes

add dbCollate

2135ecb

tangenta reviewed Nov 5, 2019

View reviewed changes

AilinKid and others added 2 commits November 5, 2019 12:46

add dbCollateTest

15fef95

Merge branch 'master' into fix_collate_same_with_table

ef61e76

wjhuang2016 reviewed Nov 5, 2019

View reviewed changes

tangenta reviewed Nov 5, 2019

View reviewed changes

tangenta added status/LGT2 Indicates that a PR has LGTM 2. component/DDL-need-LGT3 and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 5, 2019

AilinKid requested a review from crazycs520 November 5, 2019 10:05

crazycs520 approved these changes Nov 5, 2019

View reviewed changes

Merge branch 'master' into fix_collate_same_with_table

5360e39

AilinKid added the status/DNM label Nov 6, 2019

Deardrops added sig/transaction SIG:Transaction status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. sig/transaction SIG:Transaction labels Nov 6, 2019

AilinKid added 2 commits November 6, 2019 10:54

Merge branch 'master' into fix_collate_same_with_table

6d1433c

Merge branch 'master' into fix_collate_same_with_table

3f62907

AilinKid removed the status/DNM label Nov 6, 2019

AilinKid merged commit 8d545a1 into pingcap:master Nov 6, 2019

sre-bot mentioned this pull request Nov 6, 2019

ddl : fix column collate should use table's if it has when create table or alter table. (#13119 & #12327) #13174

Merged

sre-bot mentioned this pull request Nov 6, 2019

ddl : fix column collate should use table's if it has when create table or alter table. (#13119 & #12327) #13175

Merged

bb7133 reviewed Nov 6, 2019

View reviewed changes

AilinKid added a commit to AilinKid/tidb that referenced this pull request Nov 6, 2019

ddl : fix column collate should use table's if it has when create tab…

704d07d

…le or alter table. (pingcap#13119) fix column's collation should use table's collation

AilinKid mentioned this pull request Nov 6, 2019

ddl : fix column collate should use table's if it has when create table or alter table. (#13119 & #12327) #13190

Merged

bb7133 pushed a commit that referenced this pull request Nov 6, 2019

ddl : fix column collate should use table's if it has when create tab…

5df9ff5

…le or alter table. (#13119 & #12327) (#13174)

bb7133 pushed a commit that referenced this pull request Nov 6, 2019

ddl : fix column collate should use table's if it has when create tab…

724963c

…le or alter table. (#13119 & #12327) (#13175)

bb7133 pushed a commit that referenced this pull request Nov 6, 2019

ddl : fix column collate should use table's if it has when create tab…

72c6073

…le or alter table. (#13119 & #12327) (#13190)

AilinKid mentioned this pull request Nov 14, 2019

ddl, test : fix CI with adding new tk to execute additional sql. #13447

Merged

XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019

ddl : fix column collate should use table's if it has when create tab…

7897842

…le or alter table. (pingcap#13119) fix column's collation should use table's collation

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

Conversation

AilinKid commented Nov 4, 2019

What problem does this PR solve?

What is changed and how it works?

Check List

Uh oh!

codecov bot commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AilinKid commented Nov 4, 2019

Uh oh!

AilinKid commented Nov 4, 2019

Uh oh!

crazycs520 Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

AilinKid Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

crazycs520 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

tangenta Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

AilinKid Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

wjhuang2016 left a comment

Choose a reason for hiding this comment

Uh oh!

tangenta left a comment

Choose a reason for hiding this comment

Uh oh!

crazycs520 left a comment

Choose a reason for hiding this comment

Uh oh!

crazycs520 commented Nov 5, 2019

Uh oh!

AilinKid commented Nov 6, 2019

Uh oh!

AilinKid commented Nov 6, 2019

Uh oh!

AilinKid commented Nov 6, 2019

Uh oh!

sre-bot commented Nov 6, 2019

Uh oh!

sre-bot commented Nov 6, 2019

Uh oh!

sre-bot commented Nov 6, 2019

Uh oh!

bb7133 Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

sre-bot commented Apr 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov bot commented Nov 4, 2019 •

edited

Loading