ddl: Supports transitions between the same type (String types) #20032

Patrick0308 · 2020-09-16T06:10:07Z

What problem does this PR solve?

Issue Number: close #19571

Problem Summary:

What is changed and how it works?

What's Changed: Supports transitions between the same type (String types)

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

Unit test
Integration test

Release note

Supports transitions between the same type (String types)

djshow832

The procedure is basically right. Please be cautious about all kinds of transitions, because even a small bug may cause data loss.

djshow832 · 2020-09-17T10:46:13Z

ddl/ddl_api.go

-		if origin.Tp != to.Tp {
-			msg := fmt.Sprintf("cannot modify enum type column's to type %s", to.String())
-			return "", errUnsupportedModifyColumn.GenWithStackByArgs(msg)
-		}
-		if len(to.Elems) < len(origin.Elems) {
-			msg := fmt.Sprintf("the number of enum column's elements is less than the original: %d", len(origin.Elems))
-			return "", errUnsupportedModifyColumn.GenWithStackByArgs(msg)
-		}
-		for index, originElem := range origin.Elems {
-			toElem := to.Elems[index]
-			if originElem != toElem {
-				msg := fmt.Sprintf("cannot modify enum column value %s to %s", originElem, toElem)
-				return "", errUnsupportedModifyColumn.GenWithStackByArgs(msg)
-			}


Did you try that in MySQL? Is that permitted?

Yes, It's be allowed.

AilinKid · 2020-09-25T06:55:44Z

Sorry for such a long time, I will review it this weekend!!!

Patrick0308 · 2020-09-25T09:02:08Z

/run-all-tests

ddl/column.go

Patrick0308 · 2020-09-27T02:35:39Z

@AilinKid Do we need to support convertion between different charset?

AilinKid · 2020-09-27T03:41:30Z

@AilinKid Do we need to support conversion between different charset?

Should we take collate into consideration? @wjhuang2016

ddl/column.go

bb7133 · 2020-09-27T09:38:21Z

ddl/db_test.go

+	c.Assert(c2.FieldType.Tp, Equals, mysql.TypeBlob)
+
+	// text to set
+	tk.MustGetErrMsg("alter table tt change a a set('111', '2222');", "[types:1265]Data truncated for column 'a' at row 2")


If sql_mode is set to "", the job will be succeeded and warnings will be returned instead and 10000 will be converted to an empty value.

Since we don't have sql_mode here, you can choose to update your code if #20012 is merged before this PR(it's up to you). For now, let's skip the sql_mode.

ddl/column.go

bb7133 · 2020-09-27T09:54:32Z

@AilinKid Do we need to support conversion between different charset?

Should we take collate into consideration? @wjhuang2016

No, I don't think we need to consider the charset and collation in this PR. Let's keep the current constraint for modifying charset and constraint.

ddl/column.go

ti-srebot · 2020-10-14T13:46:02Z

@Patrick0308 merge failed.

djshow832 · 2020-10-14T14:28:00Z

mysql_test also needs to be modified to pass the CI. We'll adjust it.

djshow832 · 2020-10-15T03:30:02Z

mysql> CREATE TABLE t1(a INT AUTO_INCREMENT PRIMARY KEY, b ENUM('a', 'b', 'c') NOT NULL);
Query OK, 0 rows affected (0.01 sec)

mysql> INSERT INTO t1 (b) VALUES ('a'), ('c'), ('b'), ('b'), ('a');
Query OK, 5 rows affected (0.00 sec)
Records: 5  Duplicates: 0  Warnings: 0

mysql> ALTER TABLE t1 MODIFY b ENUM('a', 'z', 'b', 'c') NOT NULL;
Query OK, 0 rows affected (0.02 sec)

mysql> select @@tidb_enable_change_column_type;
+----------------------------------+
| @@tidb_enable_change_column_type |
+----------------------------------+
| 0                                |
+----------------------------------+
1 row in set (0.00 sec)

When tidb_enable_change_column_type is false, the ALTER should fail. PTAL @Patrick0308

Patrick0308 · 2020-10-15T11:41:44Z

Thanks @djshow832 , I fix this scene, but I found it's so complex to fully compatible with old version in a CheckModifyTypeCompatible function, may be we need a new CheckModifyTypeCompatible function when tidb_enable_change_column_type is true.

AilinKid · 2020-10-15T13:46:09Z

Thanks @djshow832 , I fix this scene, but I found it's so difficult to fully compatible with old version in a CheckModifyTypeCompatible function, may be we need a new CheckModifyTypeCompatible function when tidb_enable_change_column_type is true.

don't worry, I will resolve it tomorrow

AilinKid · 2020-10-16T02:54:57Z

/run-all-tests

AilinKid · 2020-10-16T03:21:32Z

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

AilinKid · 2020-10-16T03:23:45Z

/integration-common-test tidb-test=pr/1093

AilinKid · 2020-10-16T03:25:58Z

/run-all-tests tidb-test=pr/1093

AilinKid

LGTM

AilinKid · 2020-10-16T03:48:50Z

/merge

ti-srebot · 2020-10-16T03:49:11Z

/run-all-tests

AilinKid · 2020-10-16T03:59:52Z

thanks for your contribution so much~ (๑¯◡¯๑)

Patrick0308 added 2 commits September 16, 2020 11:33

Supports transitions between the same type (String types)

026364e

Supports transitions between the ENUM and Set and Strings type

a4b223c

ti-srebot added the contribution This PR is from a community contributor. label Sep 16, 2020

github-actions bot added the sig/sql-infra SIG: SQL Infra label Sep 16, 2020

Patrick0308 added 3 commits September 16, 2020 14:27

modify needChangeColumn

9b1a095

optimize needChangeColumnData

5a5b7f9

optimize needChangeColumnData use switch case

34f1edd

djshow832 reviewed Sep 17, 2020

View reviewed changes

Patrick0308 and others added 2 commits September 20, 2020 14:27

Merge remote-tracking branch 'upstream/master' into 19571

aac09e4

add some test

e1b96ee

Patrick0308 requested a review from a team as a code owner September 22, 2020 13:31

Patrick0308 requested review from wshwsh12 and removed request for a team September 22, 2020 13:31

github-actions bot added the component/expression label Sep 22, 2020

wshwsh12 requested review from AilinKid and removed request for wshwsh12 September 23, 2020 03:26

modify tests

4edf90e

AilinKid reviewed Sep 25, 2020

View reviewed changes

ddl/column.go Outdated Show resolved Hide resolved

ddl/column.go Outdated Show resolved Hide resolved

Merge branch 'master' into 19571

c360e0d

AilinKid added 2 commits September 27, 2020 11:41

Merge branch 'master' into 19571

2ba8aa3

Merge branch 'master' into 19571

8ab072f

bb7133 reviewed Sep 27, 2020

View reviewed changes

AilinKid reviewed Sep 27, 2020

View reviewed changes

ddl/column.go Outdated Show resolved Hide resolved

bb7133 reviewed Sep 27, 2020

View reviewed changes

ddl/column.go Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into 19571

38ceb01

AilinKid added 2 commits October 15, 2020 16:37

Merge branch 'master' into 19571

50ab573

Merge branch 'master' into 19571

994af5c

Patrick0308 added 2 commits October 15, 2020 19:56

fix TypeEnum convert compatible with old version

a4f1a02

Merge remote-tracking branch 'origin/19571' into 19571

b923e4d

Patrick0308 dismissed stale reviews from ti-srebot and djshow832 via b923e4d October 15, 2020 11:56

AilinKid and others added 3 commits October 15, 2020 20:19

Merge branch 'master' into 19571

8061ee8

fix convert compatible with old version

3e7a992

Merge remote-tracking branch 'origin/19571' into 19571

e80ff5c

Patrick0308 requested a review from djshow832 October 15, 2020 13:31

Merge branch 'master' into 19571

bde8729

AilinKid approved these changes Oct 16, 2020

View reviewed changes

ti-srebot removed the status/LGT2 Indicates that a PR has LGTM 2. label Oct 16, 2020

ti-srebot approved these changes Oct 16, 2020

View reviewed changes

ti-srebot added the status/LGT3 The PR has already had 3 LGTM. label Oct 16, 2020

Merge branch 'master' into 19571

176fece

ti-srebot merged commit da70410 into pingcap:master Oct 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: Supports transitions between the same type (String types) #20032

ddl: Supports transitions between the same type (String types) #20032

Patrick0308 commented Sep 16, 2020

djshow832 left a comment

djshow832 Sep 17, 2020

Patrick0308 Sep 20, 2020

AilinKid commented Sep 25, 2020

Patrick0308 commented Sep 25, 2020

Patrick0308 commented Sep 27, 2020

AilinKid commented Sep 27, 2020 •

edited

Loading

bb7133 Sep 27, 2020

bb7133 commented Sep 27, 2020

ti-srebot commented Oct 14, 2020

djshow832 commented Oct 14, 2020

djshow832 commented Oct 15, 2020

Patrick0308 commented Oct 15, 2020 •

edited

Loading

AilinKid commented Oct 15, 2020

AilinKid commented Oct 16, 2020

AilinKid commented Oct 16, 2020

AilinKid commented Oct 16, 2020

AilinKid commented Oct 16, 2020

AilinKid left a comment

AilinKid commented Oct 16, 2020

ti-srebot commented Oct 16, 2020

AilinKid commented Oct 16, 2020

ddl: Supports transitions between the same type (String types) #20032

ddl: Supports transitions between the same type (String types) #20032

Conversation

Patrick0308 commented Sep 16, 2020

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

djshow832 left a comment

Choose a reason for hiding this comment

djshow832 Sep 17, 2020

Choose a reason for hiding this comment

Patrick0308 Sep 20, 2020

Choose a reason for hiding this comment

AilinKid commented Sep 25, 2020

Patrick0308 commented Sep 25, 2020

Patrick0308 commented Sep 27, 2020

AilinKid commented Sep 27, 2020 • edited Loading

bb7133 Sep 27, 2020

Choose a reason for hiding this comment

bb7133 commented Sep 27, 2020

ti-srebot commented Oct 14, 2020

djshow832 commented Oct 14, 2020

djshow832 commented Oct 15, 2020

Patrick0308 commented Oct 15, 2020 • edited Loading

AilinKid commented Oct 15, 2020

AilinKid commented Oct 16, 2020

AilinKid commented Oct 16, 2020

AilinKid commented Oct 16, 2020

AilinKid commented Oct 16, 2020

AilinKid left a comment

Choose a reason for hiding this comment

AilinKid commented Oct 16, 2020

ti-srebot commented Oct 16, 2020

AilinKid commented Oct 16, 2020

AilinKid commented Sep 27, 2020 •

edited

Loading

Patrick0308 commented Oct 15, 2020 •

edited

Loading