diff --git a/go/sqlescape/ids_test.go b/go/sqlescape/ids_test.go index 230befe15d6..67ae233d7d6 100644 --- a/go/sqlescape/ids_test.go +++ b/go/sqlescape/ids_test.go @@ -30,6 +30,9 @@ func TestEscapeID(t *testing.T) { }, { in: "a`a", out: "`a``a`", + }, { + in: "a a", + out: "`a a`", }, { in: "`fo`o`", out: "```fo``o```", diff --git a/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go b/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go index f115e1041d6..d4517e67aff 100644 --- a/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go +++ b/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go @@ -357,8 +357,8 @@ func testRevertible(t *testing.T) { }, { name: "expanded: enum", - fromSchema: `id int primary key, e1 enum('a', 'b'), e2 enum('a', 'b'), e3 enum('a', 'b'), e4 enum('a', 'b'), e5 enum('a', 'b'), e6 enum('a', 'b'), e7 enum('a', 'b'), e8 enum('a', 'b')`, - toSchema: `id int primary key, e1 enum('a', 'b'), e2 enum('a'), e3 enum('a', 'b', 'c'), e4 enum('a', 'x'), e5 enum('a', 'x', 'b'), e6 enum('b'), e7 varchar(1), e8 tinyint`, + fromSchema: `id int primary key, e1 enum('a', 'b'), e2 enum('a', 'b'), e3 enum('a', 'b'), e4 enum('a', 'b'), e5 enum('a', 'b'), e6 enum('a', 'b'), e7 enum('a', 'b'), e8 enum('a', 'b')`, + toSchema: `id int primary key, e1 enum('a', 'b'), e2 enum('a'), e3 enum('a', 'b', 'c'), e4 enum('a', 'x'), e5 enum('a', 'x', 'b'), e6 enum('b'), e7 varchar(1), e8 tinyint`, expandedColumnNames: `e3,e4,e5,e6,e7,e8`, }, { diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-rename/alter b/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-rename/alter new file mode 100644 index 00000000000..b3e025d40b5 --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-rename/alter @@ -0,0 +1 @@ +modify e enum('red', 'green', 'cyan') not null diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-rename/create.sql b/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-rename/create.sql new file mode 100644 index 00000000000..cb4e6db0fcd --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-rename/create.sql @@ -0,0 +1,24 @@ +drop table if exists onlineddl_test; +create table onlineddl_test ( + id int auto_increment, + i int not null, + e enum('red', 'green', 'blue') not null, + primary key(id) +) auto_increment=1; + +insert into onlineddl_test values (null, 11, 'red'); +insert into onlineddl_test values (null, 13, 'green'); + +drop event if exists onlineddl_test; +delimiter ;; +create event onlineddl_test + on schedule every 1 second + starts current_timestamp + ends current_timestamp + interval 60 second + on completion not preserve + enable + do +begin + insert into onlineddl_test values (null, 11, 'red'); + insert into onlineddl_test values (null, 13, 'green'); +end ;; diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-truncate/alter b/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-truncate/alter new file mode 100644 index 00000000000..1d11c9cf5cb --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-truncate/alter @@ -0,0 +1 @@ +modify e enum('red', 'green') not null diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-truncate/create.sql b/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-truncate/create.sql new file mode 100644 index 00000000000..cb4e6db0fcd --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-truncate/create.sql @@ -0,0 +1,24 @@ +drop table if exists onlineddl_test; +create table onlineddl_test ( + id int auto_increment, + i int not null, + e enum('red', 'green', 'blue') not null, + primary key(id) +) auto_increment=1; + +insert into onlineddl_test values (null, 11, 'red'); +insert into onlineddl_test values (null, 13, 'green'); + +drop event if exists onlineddl_test; +delimiter ;; +create event onlineddl_test + on schedule every 1 second + starts current_timestamp + ends current_timestamp + interval 60 second + on completion not preserve + enable + do +begin + insert into onlineddl_test values (null, 11, 'red'); + insert into onlineddl_test values (null, 13, 'green'); +end ;; diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-changelog/alter b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-changelog/alter new file mode 100644 index 00000000000..b3e025d40b5 --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-changelog/alter @@ -0,0 +1 @@ +modify e enum('red', 'green', 'cyan') not null diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-changelog/create.sql b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-changelog/create.sql new file mode 100644 index 00000000000..27980b89072 --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-changelog/create.sql @@ -0,0 +1,22 @@ +drop table if exists onlineddl_test; +create table onlineddl_test ( + id int auto_increment, + i int not null, + e enum('red', 'green', 'blue') not null, + primary key(id) +) auto_increment=1; + +drop event if exists onlineddl_test; +delimiter ;; +create event onlineddl_test + on schedule every 1 second + starts current_timestamp + ends current_timestamp + interval 60 second + on completion not preserve + enable + do +begin + insert into onlineddl_test values (null, 11, 'red'); + insert into onlineddl_test values (null, 13, 'green'); + insert into onlineddl_test values (null, 17, 'blue'); +end ;; diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-changelog/expect_failure b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-changelog/expect_failure new file mode 100644 index 00000000000..dfa68652e4d --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-changelog/expect_failure @@ -0,0 +1 @@ +Data truncated for column diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-copy/alter b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-copy/alter new file mode 100644 index 00000000000..b3e025d40b5 --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-copy/alter @@ -0,0 +1 @@ +modify e enum('red', 'green', 'cyan') not null diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-copy/create.sql b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-copy/create.sql new file mode 100644 index 00000000000..248ab90f5fc --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-copy/create.sql @@ -0,0 +1,11 @@ +drop table if exists onlineddl_test; +create table onlineddl_test ( + id int auto_increment, + i int not null, + e enum('red', 'green', 'blue') not null, + primary key(id) +) auto_increment=1; + +insert into onlineddl_test values (null, 11, 'red'); +insert into onlineddl_test values (null, 13, 'green'); +insert into onlineddl_test values (null, 17, 'blue'); diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-copy/expect_failure b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-copy/expect_failure new file mode 100644 index 00000000000..dfa68652e4d --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-copy/expect_failure @@ -0,0 +1 @@ +Data truncated for column diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-changelog/alter b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-changelog/alter new file mode 100644 index 00000000000..1d11c9cf5cb --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-changelog/alter @@ -0,0 +1 @@ +modify e enum('red', 'green') not null diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-changelog/create.sql b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-changelog/create.sql new file mode 100644 index 00000000000..27980b89072 --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-changelog/create.sql @@ -0,0 +1,22 @@ +drop table if exists onlineddl_test; +create table onlineddl_test ( + id int auto_increment, + i int not null, + e enum('red', 'green', 'blue') not null, + primary key(id) +) auto_increment=1; + +drop event if exists onlineddl_test; +delimiter ;; +create event onlineddl_test + on schedule every 1 second + starts current_timestamp + ends current_timestamp + interval 60 second + on completion not preserve + enable + do +begin + insert into onlineddl_test values (null, 11, 'red'); + insert into onlineddl_test values (null, 13, 'green'); + insert into onlineddl_test values (null, 17, 'blue'); +end ;; diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-changelog/expect_failure b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-changelog/expect_failure new file mode 100644 index 00000000000..dfa68652e4d --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-changelog/expect_failure @@ -0,0 +1 @@ +Data truncated for column diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-copy/alter b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-copy/alter new file mode 100644 index 00000000000..1d11c9cf5cb --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-copy/alter @@ -0,0 +1 @@ +modify e enum('red', 'green') not null diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-copy/create.sql b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-copy/create.sql new file mode 100644 index 00000000000..248ab90f5fc --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-copy/create.sql @@ -0,0 +1,11 @@ +drop table if exists onlineddl_test; +create table onlineddl_test ( + id int auto_increment, + i int not null, + e enum('red', 'green', 'blue') not null, + primary key(id) +) auto_increment=1; + +insert into onlineddl_test values (null, 11, 'red'); +insert into onlineddl_test values (null, 13, 'green'); +insert into onlineddl_test values (null, 17, 'blue'); diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-copy/expect_failure b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-copy/expect_failure new file mode 100644 index 00000000000..dfa68652e4d --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-copy/expect_failure @@ -0,0 +1 @@ +Data truncated for column diff --git a/go/vt/schemadiff/column.go b/go/vt/schemadiff/column.go index d36d226294d..ae341776ccc 100644 --- a/go/vt/schemadiff/column.go +++ b/go/vt/schemadiff/column.go @@ -21,9 +21,6 @@ import ( "vitess.io/vitess/go/mysql/collations" "vitess.io/vitess/go/vt/sqlparser" - "vitess.io/vitess/go/vt/vterrors" - - vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" ) // columnDetails decorates a column with more details, used by diffing logic @@ -99,9 +96,11 @@ func NewColumnDefinitionEntity(c *sqlparser.ColumnDefinition) *ColumnDefinitionE // We need to denormalize the column's charset/collate properties, so that the comparison can be done. func (c *ColumnDefinitionEntity) ColumnDiff( env *Environment, + tableName string, other *ColumnDefinitionEntity, t1cc *charsetCollate, t2cc *charsetCollate, + hints *DiffHints, ) (*ModifyColumnDiff, error) { if c.IsTextual() || other.IsTextual() { // We will now denormalize the columns charset & collate as needed (if empty, populate from table.) @@ -111,7 +110,7 @@ func (c *ColumnDefinitionEntity) ColumnDiff( collationID := env.CollationEnv().LookupByName(c.columnDefinition.Type.Options.Collate) charset := env.CollationEnv().LookupCharsetName(collationID) if charset == "" { - return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "cannot match charset to collation %v", c.columnDefinition.Type.Options.Collate) + return nil, &UnknownColumnCollationCharsetError{Column: c.columnDefinition.Name.String(), Collation: c.columnDefinition.Type.Options.Collate} } defer func() { c.columnDefinition.Type.Charset.Name = "" @@ -133,7 +132,7 @@ func (c *ColumnDefinitionEntity) ColumnDiff( if c.columnDefinition.Type.Options.Collate = t1cc.collate; c.columnDefinition.Type.Options.Collate == "" { collation := env.CollationEnv().DefaultCollationForCharset(t1cc.charset) if collation == collations.Unknown { - return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "cannot match collation to charset %v", t1cc.charset) + return nil, &UnknownColumnCharsetCollationError{Column: c.columnDefinition.Name.String(), Charset: t1cc.charset} } c.columnDefinition.Type.Options.Collate = env.CollationEnv().LookupName(collation) } @@ -143,7 +142,7 @@ func (c *ColumnDefinitionEntity) ColumnDiff( collationID := env.CollationEnv().LookupByName(other.columnDefinition.Type.Options.Collate) charset := env.CollationEnv().LookupCharsetName(collationID) if charset == "" { - return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "cannot match charset to collation %v", other.columnDefinition.Type.Options.Collate) + return nil, &UnknownColumnCollationCharsetError{Column: other.columnDefinition.Name.String(), Collation: other.columnDefinition.Type.Options.Collate} } defer func() { other.columnDefinition.Type.Charset.Name = "" @@ -160,7 +159,7 @@ func (c *ColumnDefinitionEntity) ColumnDiff( if other.columnDefinition.Type.Options.Collate = t2cc.collate; other.columnDefinition.Type.Options.Collate == "" { collation := env.CollationEnv().DefaultCollationForCharset(t2cc.charset) if collation == collations.Unknown { - return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "cannot match collation to charset %v", t2cc.charset) + return nil, &UnknownColumnCharsetCollationError{Column: other.columnDefinition.Name.String(), Charset: t2cc.charset} } other.columnDefinition.Type.Options.Collate = env.CollationEnv().LookupName(collation) } @@ -171,6 +170,24 @@ func (c *ColumnDefinitionEntity) ColumnDiff( return nil, nil } + getEnumValuesMap := func(enumValues []string) map[string]int { + m := make(map[string]int, len(enumValues)) + for i, enumValue := range enumValues { + m[enumValue] = i + } + return m + } + switch hints.EnumReorderStrategy { + case EnumReorderStrategyReject: + otherEnumValuesMap := getEnumValuesMap(other.columnDefinition.Type.EnumValues) + for ordinal, enumValue := range c.columnDefinition.Type.EnumValues { + if otherOrdinal, ok := otherEnumValuesMap[enumValue]; ok { + if ordinal != otherOrdinal { + return nil, &EnumValueOrdinalChangedError{Table: tableName, Column: c.columnDefinition.Name.String(), Value: enumValue, Ordinal: ordinal, NewOrdinal: otherOrdinal} + } + } + } + } return NewModifyColumnDiffByDefinition(other.columnDefinition), nil } diff --git a/go/vt/schemadiff/diff_test.go b/go/vt/schemadiff/diff_test.go index 7cf738c8326..fbe7238e3fd 100644 --- a/go/vt/schemadiff/diff_test.go +++ b/go/vt/schemadiff/diff_test.go @@ -32,17 +32,17 @@ func TestDiffTables(t *testing.T) { env57, err := vtenv.New(vtenv.Options{MySQLServerVersion: "5.7.9"}) require.NoError(t, err) tt := []struct { - name string - from string - to string - diff string - cdiff string - fromName string - toName string - action string - isError bool - hints *DiffHints - env *Environment + name string + from string + to string + diff string + cdiff string + fromName string + toName string + action string + expectError string + hints *DiffHints + env *Environment }{ { name: "identical", @@ -220,7 +220,16 @@ func TestDiffTables(t *testing.T) { AlterTableAlgorithmStrategy: AlterTableAlgorithmStrategyCopy, TableCharsetCollateStrategy: TableCharsetCollateIgnoreAlways, }, - isError: true, + expectError: (&UnknownColumnCollationCharsetError{Column: "a", Collation: "latin1_nonexisting"}).Error(), + }, + { + name: "error on unknown charset", + from: "create table t (a varchar(64)) default charset=latin_nonexisting collate=''", + to: "create table t (a varchar(64) CHARACTER SET latin1 COLLATE latin1_bin)", + hints: &DiffHints{ + AlterTableAlgorithmStrategy: AlterTableAlgorithmStrategyCopy, + }, + expectError: (&UnknownColumnCharsetCollationError{Column: "a", Charset: "latin_nonexisting"}).Error(), }, { name: "changing table level defaults with column specific settings", @@ -335,9 +344,9 @@ func TestDiffTables(t *testing.T) { dq, dqerr := DiffCreateTablesQueries(env, ts.from, ts.to, hints) d, err := DiffTables(env, fromCreateTable, toCreateTable, hints) switch { - case ts.isError: - assert.Error(t, err) - assert.Error(t, dqerr) + case ts.expectError != "": + assert.ErrorContains(t, err, ts.expectError) + assert.ErrorContains(t, dqerr, ts.expectError) case ts.diff == "": assert.NoError(t, err) assert.NoError(t, dqerr) diff --git a/go/vt/schemadiff/errors.go b/go/vt/schemadiff/errors.go index efe2bbdef78..5268db76ff3 100644 --- a/go/vt/schemadiff/errors.go +++ b/go/vt/schemadiff/errors.go @@ -446,3 +446,33 @@ type EntityNotFoundError struct { func (e *EntityNotFoundError) Error() string { return fmt.Sprintf("entity %s not found", sqlescape.EscapeID(e.Name)) } + +type EnumValueOrdinalChangedError struct { + Table string + Column string + Value string + Ordinal int + NewOrdinal int +} + +func (e *EnumValueOrdinalChangedError) Error() string { + return fmt.Sprintf("ordinal of %s changed in enum or set column %s.%s, from %d to %d", e.Value, sqlescape.EscapeID(e.Table), sqlescape.EscapeID(e.Column), e.Ordinal, e.NewOrdinal) +} + +type UnknownColumnCharsetCollationError struct { + Column string + Charset string +} + +func (e *UnknownColumnCharsetCollationError) Error() string { + return fmt.Sprintf("unable to determine collation for column %s with charset %q", sqlescape.EscapeID(e.Column), e.Charset) +} + +type UnknownColumnCollationCharsetError struct { + Column string + Collation string +} + +func (e *UnknownColumnCollationCharsetError) Error() string { + return fmt.Sprintf("unable to determine charset for column %s with collation %q", sqlescape.EscapeID(e.Column), e.Collation) +} diff --git a/go/vt/schemadiff/schema.go b/go/vt/schemadiff/schema.go index f07697e1ea7..f65e1e9ac55 100644 --- a/go/vt/schemadiff/schema.go +++ b/go/vt/schemadiff/schema.go @@ -22,9 +22,7 @@ import ( "strings" "vitess.io/vitess/go/mysql/capabilities" - "vitess.io/vitess/go/vt/proto/vtrpc" "vitess.io/vitess/go/vt/sqlparser" - "vitess.io/vitess/go/vt/vterrors" "vitess.io/vitess/go/vt/vtgate/semantics" ) @@ -1093,7 +1091,7 @@ func (s *Schema) getEntityColumnNames(entityName string, schemaInformation *decl case *CreateViewEntity: return s.getViewColumnNames(entity, schemaInformation) } - return nil, vterrors.Errorf(vtrpc.Code_INTERNAL, "unexpected entity type for %v", entityName) + return nil, &UnsupportedEntityError{Entity: entity.Name(), Statement: entity.Create().CanonicalStatementString()} } // getTableColumnNames returns the names of columns in given table. diff --git a/go/vt/schemadiff/table.go b/go/vt/schemadiff/table.go index 97468a124ac..79f9f535836 100644 --- a/go/vt/schemadiff/table.go +++ b/go/vt/schemadiff/table.go @@ -1599,7 +1599,7 @@ func (c *CreateTableEntity) diffColumns(alterTable *sqlparser.AlterTable, t2ColEntity := NewColumnDefinitionEntity(t2Col) // check diff between before/after columns: - modifyColumnDiff, err := t1ColEntity.ColumnDiff(c.Env, t2ColEntity, t1cc, t2cc) + modifyColumnDiff, err := t1ColEntity.ColumnDiff(c.Env, c.Name(), t2ColEntity, t1cc, t2cc, hints) if err != nil { return err } diff --git a/go/vt/schemadiff/table_test.go b/go/vt/schemadiff/table_test.go index 91608d8a65a..af72f15776b 100644 --- a/go/vt/schemadiff/table_test.go +++ b/go/vt/schemadiff/table_test.go @@ -28,24 +28,24 @@ import ( func TestCreateTableDiff(t *testing.T) { tt := []struct { - name string - from string - to string - fromName string - toName string - diff string - diffs []string - cdiff string - cdiffs []string - isError bool - errorMsg string - autoinc int - rotation int - fulltext int - colrename int - constraint int - charset int - algorithm int + name string + from string + to string + fromName string + toName string + diff string + diffs []string + cdiff string + cdiffs []string + errorMsg string + autoinc int + rotation int + fulltext int + colrename int + constraint int + charset int + algorithm int + enumreorder int }{ { name: "identical", @@ -300,6 +300,80 @@ func TestCreateTableDiff(t *testing.T) { diff: "alter table t1 modify column c int after a, add column x int after c, add column y int", cdiff: "ALTER TABLE `t1` MODIFY COLUMN `c` int AFTER `a`, ADD COLUMN `x` int AFTER `c`, ADD COLUMN `y` int", }, + // enum + { + name: "expand enum", + from: "create table t1 (id int primary key, e enum('a', 'b', 'c'))", + to: "create table t2 (id int primary key, e enum('a', 'b', 'c', 'd'))", + diff: "alter table t1 modify column e enum('a', 'b', 'c', 'd')", + cdiff: "ALTER TABLE `t1` MODIFY COLUMN `e` enum('a', 'b', 'c', 'd')", + }, + { + name: "truncate enum", + from: "create table t1 (id int primary key, e enum('a', 'b', 'c'))", + to: "create table t2 (id int primary key, e enum('a', 'b'))", + diff: "alter table t1 modify column e enum('a', 'b')", + cdiff: "ALTER TABLE `t1` MODIFY COLUMN `e` enum('a', 'b')", + }, + { + name: "rename enum value", + from: "create table t1 (id int primary key, e enum('a', 'b', 'c'))", + to: "create table t2 (id int primary key, e enum('a', 'b', 'd'))", + diff: "alter table t1 modify column e enum('a', 'b', 'd')", + cdiff: "ALTER TABLE `t1` MODIFY COLUMN `e` enum('a', 'b', 'd')", + }, + { + name: "reorder enum, fail", + from: "create table t1 (id int primary key, e enum('a', 'b', 'c'))", + to: "create table t2 (id int primary key, e enum('b', 'a', 'c'))", + enumreorder: EnumReorderStrategyReject, + errorMsg: (&EnumValueOrdinalChangedError{Table: "t1", Column: "e", Value: "'a'", Ordinal: 0, NewOrdinal: 1}).Error(), + }, + { + name: "reorder enum, allow", + from: "create table t1 (id int primary key, e enum('a', 'b', 'c'))", + to: "create table t2 (id int primary key, e enum('b', 'a', 'c'))", + diff: "alter table t1 modify column e enum('b', 'a', 'c')", + cdiff: "ALTER TABLE `t1` MODIFY COLUMN `e` enum('b', 'a', 'c')", + enumreorder: EnumReorderStrategyAllow, + }, + { + name: "expand set", + from: "create table t1 (id int primary key, e set('a', 'b', 'c'))", + to: "create table t2 (id int primary key, e set('a', 'b', 'c', 'd'))", + diff: "alter table t1 modify column e set('a', 'b', 'c', 'd')", + cdiff: "ALTER TABLE `t1` MODIFY COLUMN `e` set('a', 'b', 'c', 'd')", + }, + { + name: "truncate set", + from: "create table t1 (id int primary key, e set('a', 'b', 'c'))", + to: "create table t2 (id int primary key, e set('a', 'b'))", + diff: "alter table t1 modify column e set('a', 'b')", + cdiff: "ALTER TABLE `t1` MODIFY COLUMN `e` set('a', 'b')", + }, + { + name: "rename set value", + from: "create table t1 (id int primary key, e set('a', 'b', 'c'))", + to: "create table t2 (id int primary key, e set('a', 'b', 'd'))", + diff: "alter table t1 modify column e set('a', 'b', 'd')", + cdiff: "ALTER TABLE `t1` MODIFY COLUMN `e` set('a', 'b', 'd')", + }, + { + name: "reorder set, fail", + from: "create table t1 (id int primary key, e set('a', 'b', 'c'))", + to: "create table t2 (id int primary key, e set('b', 'a', 'c'))", + enumreorder: EnumReorderStrategyReject, + errorMsg: (&EnumValueOrdinalChangedError{Table: "t1", Column: "e", Value: "'a'", Ordinal: 0, NewOrdinal: 1}).Error(), + }, + { + name: "reorder set, allow", + from: "create table t1 (id int primary key, e set('a', 'b', 'c'))", + to: "create table t2 (id int primary key, e set('b', 'a', 'c'))", + diff: "alter table t1 modify column e set('b', 'a', 'c')", + cdiff: "ALTER TABLE `t1` MODIFY COLUMN `e` set('b', 'a', 'c')", + enumreorder: EnumReorderStrategyAllow, + }, + // keys { name: "added key", @@ -1312,6 +1386,7 @@ func TestCreateTableDiff(t *testing.T) { hints.FullTextKeyStrategy = ts.fulltext hints.TableCharsetCollateStrategy = ts.charset hints.AlterTableAlgorithmStrategy = ts.algorithm + hints.EnumReorderStrategy = ts.enumreorder alter, err := c.Diff(other, &hints) require.Equal(t, len(ts.diffs), len(ts.cdiffs)) @@ -1319,13 +1394,20 @@ func TestCreateTableDiff(t *testing.T) { ts.diff = ts.diffs[0] ts.cdiff = ts.cdiffs[0] } - switch { - case ts.isError: - require.Error(t, err) - if ts.errorMsg != "" { - assert.Contains(t, err.Error(), ts.errorMsg) - } - case ts.diff == "": + + if ts.diff != "" { + _, err := env.Parser().ParseStrictDDL(ts.diff) + require.NoError(t, err) + } + if ts.cdiff != "" { + _, err := env.Parser().ParseStrictDDL(ts.cdiff) + require.NoError(t, err) + } + if ts.errorMsg != "" { + require.ErrorContains(t, err, ts.errorMsg) + return + } + if ts.diff == "" { assert.NoError(t, err) assert.True(t, alter.IsEmpty(), "expected empty diff, found changes") if !alter.IsEmpty() { @@ -1334,60 +1416,61 @@ func TestCreateTableDiff(t *testing.T) { t.Logf("c: %v", sqlparser.CanonicalString(c.CreateTable)) t.Logf("other: %v", sqlparser.CanonicalString(other.CreateTable)) } - default: - assert.NoError(t, err) - require.NotNil(t, alter) - assert.False(t, alter.IsEmpty(), "expected changes, found empty diff") + return + } - { - diff := alter.StatementString() - assert.Equal(t, ts.diff, diff) + // Expecting diff + assert.NoError(t, err) + require.NotNil(t, alter) + assert.False(t, alter.IsEmpty(), "expected changes, found empty diff") - if len(ts.diffs) > 0 { + { + diff := alter.StatementString() + assert.Equal(t, ts.diff, diff) - allSubsequentDiffs := AllSubsequent(alter) - require.Equal(t, len(ts.diffs), len(allSubsequentDiffs)) - require.Equal(t, len(ts.cdiffs), len(allSubsequentDiffs)) - for i := range ts.diffs { - assert.Equal(t, ts.diffs[i], allSubsequentDiffs[i].StatementString()) - assert.Equal(t, ts.cdiffs[i], allSubsequentDiffs[i].CanonicalStatementString()) - } - } - // validate we can parse back the statement - _, err := env.Parser().ParseStrictDDL(diff) - assert.NoError(t, err) + if len(ts.diffs) > 0 { - // Validate "from/to" entities - eFrom, eTo := alter.Entities() - if ts.fromName != "" { - assert.Equal(t, ts.fromName, eFrom.Name()) - } - if ts.toName != "" { - assert.Equal(t, ts.toName, eTo.Name()) + allSubsequentDiffs := AllSubsequent(alter) + require.Equal(t, len(ts.diffs), len(allSubsequentDiffs)) + require.Equal(t, len(ts.cdiffs), len(allSubsequentDiffs)) + for i := range ts.diffs { + assert.Equal(t, ts.diffs[i], allSubsequentDiffs[i].StatementString()) + assert.Equal(t, ts.cdiffs[i], allSubsequentDiffs[i].CanonicalStatementString()) } + } + // validate we can parse back the statement + _, err := env.Parser().ParseStrictDDL(diff) + assert.NoError(t, err) - { // Validate "apply()" on "from" converges with "to" - applied, err := c.Apply(alter) - assert.NoError(t, err) - require.NotNil(t, applied) - appliedDiff, err := eTo.Diff(applied, &hints) - require.NoError(t, err) - assert.True(t, appliedDiff.IsEmpty(), "expected empty diff, found changes: %v.\nc=%v\n,alter=%v\n,eTo=%v\napplied=%v\n", - appliedDiff.CanonicalStatementString(), - c.Create().CanonicalStatementString(), - alter.CanonicalStatementString(), - eTo.Create().CanonicalStatementString(), - applied.Create().CanonicalStatementString(), - ) - } + // Validate "from/to" entities + eFrom, eTo := alter.Entities() + if ts.fromName != "" { + assert.Equal(t, ts.fromName, eFrom.Name()) } - { - cdiff := alter.CanonicalStatementString() - assert.Equal(t, ts.cdiff, cdiff) - _, err := env.Parser().ParseStrictDDL(cdiff) - assert.NoError(t, err) + if ts.toName != "" { + assert.Equal(t, ts.toName, eTo.Name()) } + { // Validate "apply()" on "from" converges with "to" + applied, err := c.Apply(alter) + assert.NoError(t, err) + require.NotNil(t, applied) + appliedDiff, err := eTo.Diff(applied, &hints) + require.NoError(t, err) + assert.True(t, appliedDiff.IsEmpty(), "expected empty diff, found changes: %v.\nc=%v\n,alter=%v\n,eTo=%v\napplied=%v\n", + appliedDiff.CanonicalStatementString(), + c.Create().CanonicalStatementString(), + alter.CanonicalStatementString(), + eTo.Create().CanonicalStatementString(), + applied.Create().CanonicalStatementString(), + ) + } + } + { + cdiff := alter.CanonicalStatementString() + assert.Equal(t, ts.cdiff, cdiff) + _, err := env.Parser().ParseStrictDDL(cdiff) + assert.NoError(t, err) } }) } diff --git a/go/vt/schemadiff/types.go b/go/vt/schemadiff/types.go index 76dded320d1..73ce096ef42 100644 --- a/go/vt/schemadiff/types.go +++ b/go/vt/schemadiff/types.go @@ -119,6 +119,11 @@ const ( AlterTableAlgorithmStrategyCopy ) +const ( + EnumReorderStrategyReject int = iota + EnumReorderStrategyAllow +) + // DiffHints is an assortment of rules for diffing entities type DiffHints struct { StrictIndexOrdering bool @@ -131,6 +136,7 @@ type DiffHints struct { TableCharsetCollateStrategy int TableQualifierHint int AlterTableAlgorithmStrategy int + EnumReorderStrategy int MySQLServerVersion string // Used to determine specific capabilities such as INSTANT DDL support } diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index e868f243be1..dcebc0b9bd4 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -2796,7 +2796,10 @@ func (e *Executor) evaluateDeclarativeDiff(ctx context.Context, onlineDDL *schem return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "unexpected: cannot find table or view even as it was just created: %v", onlineDDL.Table) } senv := schemadiff.NewEnv(e.env.Environment(), e.env.Environment().CollationEnv().DefaultConnectionCharset()) - hints := &schemadiff.DiffHints{AutoIncrementStrategy: schemadiff.AutoIncrementApplyHigher} + hints := &schemadiff.DiffHints{ + AutoIncrementStrategy: schemadiff.AutoIncrementApplyHigher, + EnumReorderStrategy: schemadiff.EnumReorderStrategyAllow, + } switch ddlStmt.(type) { case *sqlparser.CreateTable: diff, err = schemadiff.DiffCreateTablesQueries(senv, existingShowCreateTable, newShowCreateTable, hints) diff --git a/go/vt/vttablet/onlineddl/vrepl/foreign_key.go b/go/vt/vttablet/onlineddl/vrepl/foreign_key.go index 343df2a8a80..8671badadc0 100644 --- a/go/vt/vttablet/onlineddl/vrepl/foreign_key.go +++ b/go/vt/vttablet/onlineddl/vrepl/foreign_key.go @@ -36,7 +36,10 @@ func RemovedForeignKeyNames( return nil, nil } env := schemadiff.NewEnv(venv, venv.CollationEnv().DefaultConnectionCharset()) - diffHints := schemadiff.DiffHints{ConstraintNamesStrategy: schemadiff.ConstraintNamesIgnoreAll} + diffHints := schemadiff.DiffHints{ + ConstraintNamesStrategy: schemadiff.ConstraintNamesIgnoreAll, + EnumReorderStrategy: schemadiff.EnumReorderStrategyAllow, + } diff, err := schemadiff.DiffCreateTablesQueries(env, originalCreateTable, vreplCreateTable, &diffHints) if err != nil { return nil, err