schemadiff: EnumReorderStrategy, checking if enum or set values c…

…hange ordinal (#15106)

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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```",

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`,
 		},
 		{

go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-rename/alter

@@ -0,0 +1 @@
+modify e enum('red', 'green', 'cyan') not null

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 ;;

go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-truncate/alter

@@ -0,0 +1 @@
+modify e enum('red', 'green') not null

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 ;;

go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-changelog/alter

@@ -0,0 +1 @@
+modify e enum('red', 'green', 'cyan') not null

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 ;;

go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-changelog/expect_failure

@@ -0,0 +1 @@
+Data truncated for column

go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-copy/alter

@@ -0,0 +1 @@
+modify e enum('red', 'green', 'cyan') not null

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');

go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-rename-copy/expect_failure

@@ -0,0 +1 @@
+Data truncated for column

go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-changelog/alter

@@ -0,0 +1 @@
+modify e enum('red', 'green') not null

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 ;;

go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-changelog/expect_failure

@@ -0,0 +1 @@
+Data truncated for column

go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-copy/alter

@@ -0,0 +1 @@
+modify e enum('red', 'green') not null

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');

go/test/endtoend/onlineddl/vrepl_suite/testdata/fail-enum-truncate-copy/expect_failure

@@ -0,0 +1 @@
+Data truncated for column

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
 }
 

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)

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)
+}

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.

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
 		}