ddl: allow enum to append values #7767

mail2fish · 2018-09-21T12:51:41Z

What have you changed? (mandatory)

fix #6398

Allow to append new values to existing ENUM fields.
Removing values or changing existing values is still unsupported.

What are the type of the changes (mandatory)?

Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested (mandatory)?

added unit test cases in ddl/db_change_test.go

sre-bot · 2018-09-21T12:51:45Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

mail2fish · 2018-09-21T12:52:18Z

/ok-to-test

lysu · 2018-09-21T14:25:51Z

@mail2fish thanks for your PR.

/run-all-tests

mail2fish · 2018-09-23T03:57:33Z

/run-all-tests

crazycs520 · 2018-09-23T08:37:03Z

ddl/ddl_api.go

@@ -1430,6 +1430,7 @@ func modifiable(origin *types.FieldType, to *types.FieldType) error {
 	if to.Charset != origin.Charset {
 		msg := fmt.Sprintf("charset %s not match origin %s", to.Charset, origin.Charset)
 		return errUnsupportedModifyColumn.GenWithStackByArgs(msg)
+


remove this line.

crazycs520 · 2018-09-23T08:58:11Z

ddl/ddl_api.go

+			for index, originElem := range origin.Elems {
+				toElem := to.Elems[index]
+				if originElem != toElem {
+					msg := fmt.Sprintf("cannot modify value %s to %s", originElem, toElem)


How about:
msg := fmt.Sprintf("cannot modify enum column value %s to %s", originElem, toElem)

crazycs520 · 2018-09-23T08:58:30Z

ddl/ddl_api.go

-		return errUnsupportedModifyColumn.GenWithStackByArgs("modify enum column is not supported")
+		if origin.Tp == to.Tp {
+			if len(to.Elems) < len(origin.Elems) {
+				msg := fmt.Sprintf("elem length %d is less than origin %d", len(to.Elems), len(origin.Elems))


How about:
msg := fmt.Sprintf("enum column elements length %d is less than origin %d", len(to.Elems), len(origin.Elems))

mccxj · 2018-09-24T00:08:43Z

ddl/db_change_test.go

+	c.Assert(err, IsNil)
+
+	expected = []string{"8 N", "10 N", "11 A"}
+


so many blank lines

mail2fish · 2018-09-25T01:31:36Z

/run-all-tests

winoros

Allow to append new values to existing ENUM/SET fields

But you don't take SET into consideration?

mail2fish · 2018-09-25T06:13:31Z

@winoros This PR is needed by my job. Maybe I could post another PR for the set.

winoros · 2018-09-25T06:31:23Z

ddl/ddl_api.go

-		return errUnsupportedModifyColumn.GenWithStackByArgs("modify enum column is not supported")
+		if origin.Tp == to.Tp {
+			if len(to.Elems) < len(origin.Elems) {
+				msg := fmt.Sprintf("enum column elements length %d is less than origin %d", len(to.Elems), len(origin.Elems))


Is the number of enum column's elements is less than the original more clearer?

winoros · 2018-09-25T06:34:42Z

ddl/ddl_api.go

+			}
+			return nil
+		}
+		msg := fmt.Sprintf("cannot modify enum type column to type %s", to.String())


cannot modify enum column's type to type %s

mail2fish · 2018-09-25T07:16:08Z

/run-all-tests

zz-jason · 2018-09-25T07:33:01Z

ddl/db_change_test.go

@@ -349,6 +349,49 @@ type expectQuery struct {
 	rows []string
 }

+func (s *testStateChangeSuite) TestAppendEnum(c *C) {


I think we should test the ALTER TABLE ... MODIFY COLUMN statements which modify the enum column to another column type.

What's the test result you expected? An error message?

According to the modifiable() function, some changes are allowed, while others are not.

At this time, to.Tp is always be enum. If we alter enum's column to other type, we will got the error message: the number of enum column's elements is less than the original.

Is that all right?

Yes, for example:

CREATE TABLE ti1(a INT NOT NULL, b INT, c INT, e ENUM('a', 'b')) engine=InnoDB; ALTER TABLE ti1 MODIFY COLUMN e ENUM('a', 'b', 'c'); -- succeed ALTER TABLE ti1 MODIFY COLUMN e ENUM('a'); -- failed

Hi, I have add some new test, please check it. thank you.

mail2fish · 2018-09-25T08:25:27Z

/run-all-tests

mail2fish · 2018-09-26T01:58:39Z

/run-all-tests

imtbkcat · 2018-09-26T08:08:16Z

ddl/ddl_api.go

+			}
+			for index, originElem := range origin.Elems {
+				toElem := to.Elems[index]
+				if originElem != toElem {


For enum enum('N','Y'), alter table t change c2 c2 enum('N','Y','A','B') DEFAULT 'A'; is okay, but alter table t change c2 c2 enum('A','B','N','Y') DEFAULT 'A' does not work.

What do you mean? The enum order shouldn't be changed. The SQL "alter table t change c2 c2 enum('A','B','N','Y') DEFAULT 'A' " is also not working at MySQL.

imtbkcat

LGTM

zz-jason

LGTM

need one more reviewer.

zz-jason · 2018-09-26T09:12:44Z

@zimulala @winkyao PTAL

mail2fish · 2018-09-27T02:12:40Z

You are right. It worked in MySQL. But I am not sure whether we should support this feature. After I changed the enum order in MySQL 5.6, The enum's value in the rows has changed. mysql> create table t (c1 varchar(64),c2 enum('N','Y') not null default 'N',c3 timestamp on update current_timestamp,c4 int primary key,unique key idx2 (c2, c3)); Query OK, 0 rows affected (0.21 sec) mysql> insert into t values('a', 'N', '2017-07-01', 8); Query OK, 1 row affected (0.04 sec) mysql> select c2+0 from t; +------+ | c2+0 | +------+ | 1 | +------+ 1 row in set (0.00 sec) mysql> alter table t change c2 c2 enum('A','B','D','F','Y','N') DEFAULT 'D'; Query OK, 1 row affected (0.53 sec) Records: 1 Duplicates: 0 Warnings: 0 mysql> select c2+0 from t; +------+ | c2+0 | +------+ | 6 | +------+ 1 row in set (0.00 sec) Lingyu Song <notifications@github.com> 于2018年9月26日周三下午4:59写道：

…

***@***.**** commented on this pull request. ------------------------------ In ddl/ddl_api.go <#7767 (comment)>: > @@ -1455,7 +1455,22 @@ func modifiable(origin *types.FieldType, to *types.FieldType) error { return nil } case mysql.TypeEnum: - return errUnsupportedModifyColumn.GenWithStackByArgs("modify enum column is not supported") + if origin.Tp == to.Tp { + 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 { I use MySQL 5.6 to execute these case. in MySQL: mysql> create table t (c1 varchar(64),c2 enum('N','Y') not null default 'N',c3 timestamp on update current_timestamp,c4 int primary key,unique key idx2 (c2, c3)); Query OK, 0 rows affected (0.03 sec) mysql> insert into t values('a', 'N', '2017-07-01', 8); Query OK, 1 row affected (0.00 sec) mysql> alter table t change c2 c2 enum('A','B','N','Y') DEFAULT 'A'; Query OK, 1 row affected (0.05 sec) Records: 1 Duplicates: 0 Warnings: 0 in TiDB: mysql> insert into t values('a', 'N', '2017-07-01', 8); Query OK, 1 row affected (0.00 sec) mysql> alter table t change c2 c2 enum('A','B','N','Y') DEFAULT 'A'; ERROR 1105 (HY000): unsupported modify column cannot modify enum column value N to A mysql> alter table t change c2 c2 enum('N','Y','A','B') DEFAULT 'A'; Query OK, 0 rows affected (0.01 sec) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7767 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVWjEoUiXDd2khMrhG9SkMlOnfHtta3ks5ue0HpgaJpZM4W0FuT> .

mail2fish · 2018-09-27T02:17:36Z

/run-all-tests

coocood · 2018-09-27T09:32:54Z

LGTM

lysu added contribution This PR is from a community contributor. component/DDL-need-LGT3 labels Sep 21, 2018

lysu changed the title ~~ddl: allow enum/set to append values~~ ddl: allow enum to append values Sep 21, 2018

mail2fish force-pushed the enum branch from b97db2f to b557fee Compare September 23, 2018 03:20

crazycs520 reviewed Sep 23, 2018

View reviewed changes

mccxj reviewed Sep 24, 2018

View reviewed changes

ddl/db_change_test.go Outdated

c.Assert(err, IsNil)

expected = []string{"8 N", "10 N", "11 A"}

Copy link

Contributor

mccxj Sep 24, 2018

Choose a reason for hiding this comment

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

so many blank lines

mail2fish force-pushed the enum branch from b557fee to bd93a10 Compare September 25, 2018 01:31

winoros reviewed Sep 25, 2018

View reviewed changes

mail2fish force-pushed the enum branch from bd93a10 to efc48f9 Compare September 25, 2018 07:15

zz-jason reviewed Sep 25, 2018

View reviewed changes

mail2fish force-pushed the enum branch from efc48f9 to 0b89041 Compare September 25, 2018 08:18

mail2fish force-pushed the enum branch from 0b89041 to 1f22a27 Compare September 26, 2018 01:58

imtbkcat added the status/all tests passed label Sep 26, 2018

imtbkcat reviewed Sep 26, 2018

View reviewed changes

imtbkcat added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 26, 2018

zz-jason previously approved these changes Sep 26, 2018

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 26, 2018

mail2fish force-pushed the enum branch from 1f22a27 to fdcc991 Compare September 27, 2018 02:17

ddl: allow enum/set to append values

06f325d

mail2fish force-pushed the enum branch from 77076fd to 06f325d Compare September 27, 2018 08:47

Merge branch 'master' into enum

361d876

coocood approved these changes Sep 27, 2018

View reviewed changes

zz-jason merged commit 1771d67 into pingcap:master Sep 27, 2018

winkyao mentioned this pull request Feb 26, 2019

ddl: allow enum/set to append values #6941

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: allow enum to append values #7767

ddl: allow enum to append values #7767

mail2fish commented Sep 21, 2018 •

edited by winoros

Loading

sre-bot commented Sep 21, 2018

mail2fish commented Sep 21, 2018

lysu commented Sep 21, 2018

mail2fish commented Sep 23, 2018

crazycs520 Sep 23, 2018

crazycs520 Sep 23, 2018

crazycs520 Sep 23, 2018

mccxj Sep 24, 2018

mail2fish commented Sep 25, 2018

winoros left a comment •

edited

Loading

mail2fish commented Sep 25, 2018

winoros Sep 25, 2018 •

edited

Loading

winoros Sep 25, 2018

mail2fish commented Sep 25, 2018

zz-jason Sep 25, 2018

mail2fish Sep 25, 2018

zz-jason Sep 25, 2018

mail2fish Sep 25, 2018

zz-jason Sep 25, 2018

mail2fish Sep 26, 2018

mail2fish commented Sep 25, 2018

mail2fish commented Sep 26, 2018

imtbkcat Sep 26, 2018

mail2fish Sep 26, 2018

imtbkcat Sep 26, 2018 •

edited

Loading

imtbkcat left a comment

zz-jason left a comment

zz-jason commented Sep 26, 2018

mail2fish commented Sep 27, 2018 via email

mail2fish commented Sep 27, 2018

coocood commented Sep 27, 2018

		c.Assert(err, IsNil)

		expected = []string{"8 N", "10 N", "11 A"}

ddl: allow enum to append values #7767

ddl: allow enum to append values #7767

Conversation

mail2fish commented Sep 21, 2018 • edited by winoros Loading

What have you changed? (mandatory)

What are the type of the changes (mandatory)?

How has this PR been tested (mandatory)?

sre-bot commented Sep 21, 2018

mail2fish commented Sep 21, 2018

lysu commented Sep 21, 2018

mail2fish commented Sep 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mail2fish commented Sep 25, 2018

winoros left a comment • edited Loading

Choose a reason for hiding this comment

mail2fish commented Sep 25, 2018

winoros Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mail2fish commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mail2fish commented Sep 25, 2018

mail2fish commented Sep 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imtbkcat Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

imtbkcat left a comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Sep 26, 2018

mail2fish commented Sep 27, 2018 via email

mail2fish commented Sep 27, 2018

coocood commented Sep 27, 2018

mail2fish commented Sep 21, 2018 •

edited by winoros

Loading

winoros left a comment •

edited

Loading

winoros Sep 25, 2018 •

edited

Loading

imtbkcat Sep 26, 2018 •

edited

Loading