schemadiff: fix diffing of textual columns with implicit charsets

go/vt/schemadiff/column.go

@@ -79,9 +79,90 @@
 
 // ColumnDiff compares this table statement with another table statement, and sees what it takes to
 // change this table to look like the other table.
-// It returns an AlterTable statement if changes are found, or nil if not.
-// the other table may be of different name; its name is ignored.
-func (c *ColumnDefinitionEntity) ColumnDiff(other *ColumnDefinitionEntity, _ *DiffHints) *ModifyColumnDiff {
+// It returns an ModifyColumnDiff statement if changes are found, or nil if not.
+// The function also requires the charset/collate on the source & target tables. This is because the column's
+// charset & collation, if undefined, are really defined by the table's charset & collation.
+//
+//	Anecdotally, in CreateTableEntity.normalize() we actually actively strip away the charset/collate properties
+//	from the column definition, to get a cleaner table definition.
+//
+// Things get complicated when we consider hints.TableCharsetCollateStrategy. Consider this test case:
+//
+//	from: "create table t (a varchar(64)) default charset=latin1",
+//	to:   "create table t (a varchar(64) CHARACTER SET latin1 COLLATE latin1_bin)",
+//
+// In both cases, the column is really a latin1. But the tables themselves have different collations.
+// In TableCharsetCollateStrict, the situation is handled correctly by converting the table charset as well as
+// pinning the column's charset.
+// But in TableCharsetCollateIgnoreEmpty or in TableCharsetCollateIgnoreAlways, schemadiff is not supposed to
+// change the table's charset. In which case it does need to identify the fact that the two columns are really of
+// the same charset, and not report a diff.
+func (c *ColumnDefinitionEntity) ColumnDiff(
+	other *ColumnDefinitionEntity,
+	hints *DiffHints,
+	t1cc *charsetCollate,
+	t2cc *charsetCollate,
+) *ModifyColumnDiff {
+	// We need to understand whether the table's charset is going to be ignored. If so, we need to denormalize
+	// the columns' charset/collate properties for the sake of this comparison.
+	// Denormalization means that if the column does not have charset/collate defined, then it must borrow those
+	// propeties from the table.
+	denormalizeCharsetCollate := false
+	if c.IsTextual() || other.IsTextual() {
+		switch hints.TableCharsetCollateStrategy {
+		case TableCharsetCollateStrict:
+			// No need to do anything
+		case TableCharsetCollateIgnoreEmpty:
+			if t1cc.charset != t2cc.charset {
+				if t1cc.charset == "" || t2cc.charset == "" {
+					denormalizeCharsetCollate = true
+				}
+			}
+			if t1cc.collate != t2cc.collate {
+				if t1cc.collate == "" || t2cc.collate == "" {
+					denormalizeCharsetCollate = true
+				}
+			}
+		case TableCharsetCollateIgnoreAlways:
+			if t1cc.charset != t2cc.charset {
+				denormalizeCharsetCollate = true
+			}
+			if t1cc.collate != t2cc.collate {
+				denormalizeCharsetCollate = true
+			}
+		}
+	}
+
+	if denormalizeCharsetCollate {
+		// We have concluded that the two tables have different charset/collate, and that the diff hints will cause
+		// that difference to be ignored.
+		// Now is the time to populate (denormalize) the column's charset/collate properties.
+		if c.columnDefinition.Type.Charset.Name == "" {
+			c.columnDefinition.Type.Charset.Name = t1cc.charset
+			defer func() {
+				c.columnDefinition.Type.Charset.Name = ""
+			}()
+		}
+		if c.columnDefinition.Type.Options.Collate == "" {
+			c.columnDefinition.Type.Options.Collate = t1cc.collate
+			defer func() {
+				c.columnDefinition.Type.Options.Collate = ""
+			}()
+		}
+		if other.columnDefinition.Type.Charset.Name == "" {
+			other.columnDefinition.Type.Charset.Name = t2cc.charset
+			defer func() {
+				other.columnDefinition.Type.Charset.Name = ""
+			}()
+		}
+		if other.columnDefinition.Type.Options.Collate == "" {
+			other.columnDefinition.Type.Options.Collate = t2cc.collate
+			defer func() {
+				other.columnDefinition.Type.Options.Collate = ""
+			}()
+		}
+	}
+
 	if sqlparser.Equals.RefOfColumnDefinition(c.columnDefinition, other.columnDefinition) {
 		return nil
 	}

go/vt/schemadiff/diff_test.go

@@ -189,6 +189,49 @@ func TestDiffTables(t *testing.T) {
 				TableQualifierHint: TableQualifierDeclared,
 			},
 		},
+		{
+			name: "changing table level defaults with column specific settings, ignore charset",
+			from: "create table t (a varchar(64) CHARACTER SET latin1 COLLATE latin1_bin) default charset=latin1",
+			to:   "create table t (a varchar(64) CHARACTER SET latin1 COLLATE latin1_bin)",
+			hints: &DiffHints{
+				AlterTableAlgorithmStrategy: AlterTableAlgorithmStrategyCopy,
+				TableCharsetCollateStrategy: TableCharsetCollateIgnoreAlways,
+			},
+		},
+		{
+			name:     "changing table level defaults with column specific settings",
+			from:     "create table t (a varchar(64) CHARACTER SET latin1 COLLATE latin1_bin) default charset=latin1",
+			to:       "create table t (a varchar(64) CHARACTER SET latin1 COLLATE latin1_bin)",
+			diff:     "alter table t modify column a varchar(64) character set latin1 collate latin1_bin, charset utf8mb4, algorithm = COPY",
+			cdiff:    "ALTER TABLE `t` MODIFY COLUMN `a` varchar(64) CHARACTER SET latin1 COLLATE latin1_bin, CHARSET utf8mb4, ALGORITHM = COPY",
+			action:   "alter",
+			fromName: "t",
+			hints: &DiffHints{
+				AlterTableAlgorithmStrategy: AlterTableAlgorithmStrategyCopy,
+				TableCharsetCollateStrategy: TableCharsetCollateStrict,
+			},
+		},
+		{
+			name:     "changing table level defaults with column specific settings, table already normalized",
+			from:     "create table t (a varchar(64)) default charset=latin1",
+			to:       "create table t (a varchar(64) CHARACTER SET latin1 COLLATE latin1_bin)",
+			diff:     "alter table t modify column a varchar(64) character set latin1 collate latin1_bin, charset utf8mb4, algorithm = COPY",
+			cdiff:    "ALTER TABLE `t` MODIFY COLUMN `a` varchar(64) CHARACTER SET latin1 COLLATE latin1_bin, CHARSET utf8mb4, ALGORITHM = COPY",
+			action:   "alter",
+			fromName: "t",
+			hints: &DiffHints{
+				AlterTableAlgorithmStrategy: AlterTableAlgorithmStrategyCopy,
+				TableCharsetCollateStrategy: TableCharsetCollateStrict,
+			},
+		},
+		{
+			name:   "changing table level charset to default",
+			from:   `create table t (i int) default charset=latin1`,
+			to:     `create table t (i int)`,
+			action: "alter",
+			diff:   "alter table t charset utf8mb4",
+			cdiff:  "ALTER TABLE `t` CHARSET utf8mb4",
+		},
 	}
 	parser := sqlparser.NewTestParser()
 	for _, ts := range tt {
@@ -228,8 +271,12 @@ func TestDiffTables(t *testing.T) {
 			case ts.diff == "":
 				assert.NoError(t, err)
 				assert.NoError(t, dqerr)
-				assert.Nil(t, d)
-				assert.Nil(t, dq)
+				if !assert.Nil(t, d) {
+					assert.Failf(t, "found unexpected diff", "%v", d.CanonicalStatementString())
+				}
+				if !assert.Nil(t, dq) {
+					assert.Failf(t, "found unexpected diff", "%v", dq.CanonicalStatementString())
+				}
 			default:
 				assert.NoError(t, err)
 				require.NotNil(t, d)
@@ -357,8 +404,12 @@ func TestDiffViews(t *testing.T) {
 			case ts.diff == "":
 				assert.NoError(t, err)
 				assert.NoError(t, dqerr)
-				assert.Nil(t, d)
-				assert.Nil(t, dq)
+				if !assert.Nil(t, d) {
+					assert.Failf(t, "found unexpected diff", "%v", d.CanonicalStatementString())
+				}
+				if !assert.Nil(t, dq) {
+					assert.Failf(t, "found unexpected diff", "%v", dq.CanonicalStatementString())
+				}
 			default:
 				assert.NoError(t, err)
 				require.NotNil(t, d)

go/vt/schemadiff/table.go

@@ -31,6 +31,15 @@
 	"vitess.io/vitess/go/vt/sqlparser"
 )
 
+type charsetCollate struct {
+	charset string
+	collate string
+}
+
+func (c *charsetCollate) defaultCollation() string {
+	return defaultCharsetCollation(c.charset)
+}
+
 type AlterTableEntityDiff struct {
 	from       *CreateTableEntity
 	to         *CreateTableEntity
@@ -436,21 +445,26 @@
 	return collationEnv.LookupName(collation)
 }
 
-func (c *CreateTableEntity) normalizeColumnOptions() {
-	tableCharset := defaultCharset()
-	tableCollation := ""
-	for _, option := range c.CreateTable.TableSpec.Options {
-		switch strings.ToUpper(option.Name) {
-		case "CHARSET":
-			tableCharset = option.String
-		case "COLLATE":
-			tableCollation = option.String
+func getTableCharsetCollate(tableOptions *sqlparser.TableOptions) *charsetCollate {
+	cc := &charsetCollate{
+		charset: defaultCharset(),
+	}
+	for _, option := range *tableOptions {
+		if strings.EqualFold(option.Name, "charset") {
+			cc.charset = option.String
+		}
+		if strings.EqualFold(option.Name, "collate") {
+			cc.collate = option.String
 		}
 	}
-	defaultCollation := defaultCharsetCollation(tableCharset)
-	if tableCollation == "" {
-		tableCollation = defaultCollation
+	if cc.collate == "" {
+		cc.collate = cc.defaultCollation()
 	}
+	return cc
+}
+
+func (c *CreateTableEntity) normalizeColumnOptions() {
+	cc := getTableCharsetCollate(&c.CreateTable.TableSpec.Options)
 
 	for _, col := range c.CreateTable.TableSpec.Columns {
 		if col.Type.Options == nil {
@@ -571,13 +585,13 @@
 		if _, ok := charsetTypes[col.Type.Type]; ok {
 			// If the charset is explicitly configured and it mismatches, we don't normalize
 			// anything for charsets or collations and move on.
-			if col.Type.Charset.Name != "" && col.Type.Charset.Name != tableCharset {
+			if col.Type.Charset.Name != "" && col.Type.Charset.Name != cc.charset {
 				continue
 			}
 
 			// Alright, first check if both charset and collation are the same as
 			// the table level options, in that case we can remove both since that's equivalent.
-			if col.Type.Charset.Name == tableCharset && col.Type.Options.Collate == tableCollation {
+			if col.Type.Charset.Name == cc.charset && col.Type.Options.Collate == cc.collate {
 				col.Type.Charset.Name = ""
 				col.Type.Options.Collate = ""
 			}
@@ -595,13 +609,13 @@
 			if col.Type.Charset.Name != "" {
 				col.Type.Charset.Name = ""
 				if col.Type.Options.Collate == "" {
-					col.Type.Options.Collate = defaultCollation
+					col.Type.Options.Collate = cc.defaultCollation()
 				}
 			}
 
 			// We now have one case left, which is when we have set a collation but it's the same
 			// as the table level. In that case, we can clear it since that is equivalent.
-			if col.Type.Options.Collate == tableCollation {
+			if col.Type.Options.Collate == cc.collate {
 				col.Type.Options.Collate = ""
 			}
 		}
@@ -826,21 +840,19 @@
 		alterTable.Table.Qualifier = other.Table.Qualifier
 	}
 
-	diffedTableCharset := ""
+	t1cc := getTableCharsetCollate(&c.CreateTable.TableSpec.Options)
+	t2cc := getTableCharsetCollate(&other.CreateTable.TableSpec.Options)
+
 	var parentAlterTableEntityDiff *AlterTableEntityDiff
 	var partitionSpecs []*sqlparser.PartitionSpec
 	var superfluousFulltextKeys []*sqlparser.AddIndexDefinition
-	{
-		t1Options := c.CreateTable.TableSpec.Options
-		t2Options := other.CreateTable.TableSpec.Options
-		diffedTableCharset = c.diffTableCharset(t1Options, t2Options)
-	}
 	{
 		// diff columns
 		// ordered columns for both tables:
+
 		t1Columns := c.CreateTable.TableSpec.Columns
 		t2Columns := other.CreateTable.TableSpec.Columns
-		c.diffColumns(alterTable, t1Columns, t2Columns, hints, diffedTableCharset != "")
+		c.diffColumns(alterTable, t1Columns, t2Columns, hints, t1cc, t2cc)
 	}
 	{
 		// diff keys
@@ -927,21 +939,11 @@
 }
 
 func (c *CreateTableEntity) diffTableCharset(
-	t1Options sqlparser.TableOptions,
-	t2Options sqlparser.TableOptions,
+	t1cc *charsetCollate,
+	t2cc *charsetCollate,
 ) string {
-	getcharset := func(options sqlparser.TableOptions) string {
-		for _, option := range options {
-			if strings.EqualFold(option.Name, "CHARSET") {
-				return option.String
-			}
-		}
-		return ""
-	}
-	t1Charset := getcharset(t1Options)
-	t2Charset := getcharset(t2Options)
-	if t1Charset != t2Charset {
-		return t2Charset
+	if t1cc.charset != t2cc.charset {
+		return t2cc.charset
 	}
 	return ""
 }
@@ -1019,7 +1021,7 @@
 			case "CHARSET":
 				switch hints.TableCharsetCollateStrategy {
 				case TableCharsetCollateStrict:
-					tableOption = &sqlparser.TableOption{String: ""}
+					tableOption = &sqlparser.TableOption{Name: "CHARSET", String: defaultCharset(), CaseSensitive: true}
 					// in all other strategies we ignore the charset
 				}
 			case "CHECKSUM":
@@ -1536,7 +1538,8 @@
 	t1Columns []*sqlparser.ColumnDefinition,
 	t2Columns []*sqlparser.ColumnDefinition,
 	hints *DiffHints,
-	tableCharsetChanged bool,
+	t1cc *charsetCollate,
+	t2cc *charsetCollate,
 ) {
 	getColumnsMap := func(cols []*sqlparser.ColumnDefinition) map[string]*columnDetails {
 		var prevCol *columnDetails
@@ -1599,13 +1602,13 @@
 		t2ColEntity := NewColumnDefinitionEntity(t2Col)
 
 		// check diff between before/after columns:
-		modifyColumnDiff := t1ColEntity.ColumnDiff(t2ColEntity, hints)
+		modifyColumnDiff := t1ColEntity.ColumnDiff(t2ColEntity, hints, t1cc, t2cc)
 		if modifyColumnDiff == nil {
 			// even if there's no apparent change, there can still be implicit changes
 			// it is possible that the table charset is changed. the column may be some col1 TEXT NOT NULL, possibly in both versions 1 and 2,
-			// but implicitly the column has changed its characters set. So we need to explicitly ass a MODIFY COLUMN statement, so that
+			// but implicitly the column has changed its character set. So we need to explicitly add a MODIFY COLUMN statement, so that
 			// MySQL rebuilds it.
-			if tableCharsetChanged && t2ColEntity.IsTextual() && t2Col.Type.Charset.Name == "" {
+			if t1cc.charset != t2cc.charset && t2ColEntity.IsTextual() && t2Col.Type.Charset.Name == "" {
 				modifyColumnDiff = NewModifyColumnDiffByDefinition(t2Col)
 			}
 		}