Preserving column tags on type changes

go/libraries/doltcore/sqle/dolt_patch_table_function.go

@@ -495,7 +495,6 @@ func getPatchNodes(ctx *sql.Context, dbData env.DbData, tableDeltas []diff.Table
 		// Get SCHEMA DIFF
 		var schemaStmts []string
 		if includeSchemaDiff {
-
 			schemaStmts, err = getSchemaSqlPatch(ctx, toRefDetails.root, td)
 			if err != nil {
 				return nil, err
@@ -661,6 +660,10 @@ func GetNonCreateNonDropTableSqlSchemaDiff(td diff.TableDelta, toSchemas map[str
 			if cd.Old.Name != cd.New.Name {
 				ddlStatements = append(ddlStatements, sqlfmt.AlterTableRenameColStmt(td.ToName, cd.Old.Name, cd.New.Name))
 			}
+			if cd.Old.TypeInfo != cd.New.TypeInfo {
+				ddlStatements = append(ddlStatements, sqlfmt.AlterTableModifyColStmt(td.ToName,
+					sqlfmt.GenerateCreateTableColumnDefinition(*cd.New, sql.CollationID(td.ToSch.GetCollation()))))
+			}
 		}
 	}
 

go/libraries/doltcore/sqle/enginetest/dolt_queries.go

@@ -2072,11 +2072,16 @@ var HistorySystemTableScriptTests = []queries.ScriptTest{
 				Expected: []sql.Row{{1, 3}, {4, 6}},
 			},
 			{
-				// When filtering on a column from the original table, we use the primary index here, but because
-				// column tags have changed in previous versions of the table, the index tags don't match up completely.
+				// When filtering on a column from the original table, we use the primary index here, but if column
+				// tags have changed in previous versions of the table, the index tags won't match up completely.
 				// https://github.com/dolthub/dolt/issues/6891
+				// NOTE: {4,5,nil} shows up as a row from the first commit, when c2 was a varchar type. The schema
+				//       for dolt_history_t uses the current table schema, and we can't extract an int from the older
+				//       version's tuple, so it shows up as a NULL. In the future, we could use a different tuple
+				//       descriptor based on the version of the row and pull the data out that way and try to convert
+				//       it to the new type, but it may not actually be worth it.
 				Query:    "select pk, c1, c2 from dolt_history_t where pk=4;",
-				Expected: []sql.Row{{4, 5, 6}},
+				Expected: []sql.Row{{4, 5, 6}, {4, 5, nil}},
 			},
 		},
 	},

go/libraries/doltcore/sqle/enginetest/dolt_queries_diff.go

@@ -3548,6 +3548,8 @@ var PatchTableFunctionScriptTests = []queries.ScriptTest{
 			"insert into t values (1, null, 1, 1), (2, 2, null, 2), (3, 3, 3, 3)",
 			"CALL dolt_commit('-Am', 'new table t')",
 			"CALL dolt_checkout('-b', 'other')",
+			"alter table t modify column a varchar(100) comment 'foo';",
+			"alter table t rename column c to z;",
 			"alter table t drop column b",
 			"alter table t add column d int",
 			"delete from t where pk = 3",
@@ -3559,11 +3561,18 @@ var PatchTableFunctionScriptTests = []queries.ScriptTest{
 			{
 				Query: "SELECT statement FROM dolt_patch('main', 'other', 't') ORDER BY statement_order",
 				Expected: []sql.Row{
+					{"ALTER TABLE `t` MODIFY COLUMN `a` varchar(100) COMMENT 'foo';"},
 					{"ALTER TABLE `t` DROP `b`;"},
+					{"ALTER TABLE `t` RENAME COLUMN `c` TO `z`;"},
 					{"ALTER TABLE `t` ADD `d` int;"},
-					{"UPDATE `t` SET `a`=9 WHERE `pk`=1;"},
+					// TODO: The two updates to z below aren't necessary, since the column
+					//       was renamed and those are the old values, but it shows as a diff
+					//       because of the column name change, so we output UPDATE statements
+					//       for them. This isn't a correctness issue, but it is inefficient.
+					{"UPDATE `t` SET `a`='9',`z`=1 WHERE `pk`=1;"},
+					{"UPDATE `t` SET `z`=2 WHERE `pk`=2;"},
 					{"DELETE FROM `t` WHERE `pk`=3;"},
-					{"INSERT INTO `t` (`pk`,`a`,`c`,`d`) VALUES (7,7,7,7);"},
+					{"INSERT INTO `t` (`pk`,`a`,`z`,`d`) VALUES (7,'7',7,7);"},
 				},
 			},
 		},

go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go

@@ -2347,6 +2347,57 @@ var MergeScripts = []queries.ScriptTest{
 			},
 		},
 	},
+	{
+		Name: "merge when schemas are equal, but column tags are different",
+		SetUpScript: []string{
+			// Create a branch where t doesn't exist yet
+			"call dolt_branch('branch1');",
+			// Create t on main, but change column types so that the tag won't match branch1
+			"CREATE TABLE t (pk INT PRIMARY KEY, col1 int);",
+			"call dolt_commit('-Am', 'creating table t on main');",
+			"ALTER TABLE t modify column col1 varchar(255);",
+			"call dolt_commit('-am', 'modifying table t on main');",
+			"INSERT INTO t values (1, 'one'), (2, 'two');",
+			"call dolt_commit('-am', 'inserting two rows into t on main');",
+
+			// Create t on branch1, without an intermediate type change, so that the tag doesn't match main
+			"call dolt_checkout('branch1');",
+			"CREATE TABLE t (pk INT PRIMARY KEY, col1 varchar(255));",
+			"call dolt_commit('-Am', 'creating table t on branch1');",
+			"INSERT INTO t values (3, 'three');",
+			"call dolt_commit('-am', 'inserting one row into t on branch1');",
+			"SET @PreMergeBranch1Commit = dolt_hashof('HEAD');",
+		},
+		Assertions: []queries.ScriptTestAssertion{
+			{
+				// We can merge from main -> branch1, even though the column tags are not identical
+				Query:    "call dolt_merge('main')",
+				Expected: []sql.Row{{doltCommit, 0, 0, "merge successful"}},
+			},
+			{
+				Query:    "SELECT * FROM t;",
+				Expected: []sql.Row{{1, "one"}, {2, "two"}, {3, "three"}},
+			},
+			{
+				// Reset branch1 to the pre-merge commit, so we can test merging branch1 -> main
+				Query:    "CALL dolt_reset('--hard', @PreMergeBranch1Commit);",
+				Expected: []sql.Row{{0}},
+			},
+			{
+				Query:    "CALL dolt_checkout('main');",
+				Expected: []sql.Row{{0, "Switched to branch 'main'"}},
+			},
+			{
+				// We can merge from branch1 -> main, even though the column tags are not identical
+				Query:    "call dolt_merge('branch1')",
+				Expected: []sql.Row{{doltCommit, 0, 0, "merge successful"}},
+			},
+			{
+				Query:    "SELECT * FROM t;",
+				Expected: []sql.Row{{1, "one"}, {2, "two"}, {3, "three"}},
+			},
+		},
+	},
 }
 
 var KeylessMergeCVsAndConflictsScripts = []queries.ScriptTest{

go/libraries/doltcore/sqle/tables.go

@@ -1591,7 +1591,7 @@ func (t *AlterableDoltTable) RewriteInserter(
 		return nil, err
 	}
 
-	newSch, err := t.getNewSch(ctx, oldColumn, newColumn, oldSch, newSchema, ws.WorkingRoot(), headRoot)
+	newSch, err := t.createSchemaForColumnChange(ctx, oldColumn, newColumn, oldSch, newSchema, ws.WorkingRoot(), headRoot)
 	if err != nil {
 		return nil, err
 	}
@@ -1925,9 +1925,13 @@ func validateFullTextColumnChange(ctx *sql.Context, idx schema.Index, oldColumn
 	return nil
 }
 
-func (t *AlterableDoltTable) getNewSch(ctx context.Context, oldColumn, newColumn *sql.Column, oldSch schema.Schema, newSchema sql.PrimaryKeySchema, root, headRoot *doltdb.RootValue) (schema.Schema, error) {
+// createSchemaForColumnChange creates a new Dolt schema based on the old Dolt schema (|oldSch|) changing to the new
+// SQL schema (|newSchema|) from the SQL column |oldColumn| changing to the SQL column |newColumn|. The working root
+// is provided in |root| and the branch head root is provided in |headRoot|. If any problems are encountered, a nil
+// Dolt schema is returned along with an error.
+func (t *AlterableDoltTable) createSchemaForColumnChange(ctx context.Context, oldColumn, newColumn *sql.Column, oldSch schema.Schema, newSchema sql.PrimaryKeySchema, root, headRoot *doltdb.RootValue) (schema.Schema, error) {
+	// Adding or dropping a column
 	if oldColumn == nil || newColumn == nil {
-		// Adding or dropping a column
 		newSch, err := sqlutil.ToDoltSchema(ctx, root, t.Name(), newSchema, headRoot, sql.CollationID(oldSch.GetCollation()))
 		if err != nil {
 			return nil, err
@@ -1935,80 +1939,35 @@ func (t *AlterableDoltTable) getNewSch(ctx context.Context, oldColumn, newColumn
 		return newSch, err
 	}
 
-	oldTi, err := typeinfo.FromSqlType(oldColumn.Type)
-	if err != nil {
-		return nil, err
-	}
-	newTi, err := typeinfo.FromSqlType(newColumn.Type)
-	if err != nil {
-		return nil, err
-	}
-
-	if oldTi.NomsKind() != newTi.NomsKind() {
-		oldCol, ok := oldSch.GetAllCols().GetByName(oldColumn.Name)
-		if !ok {
-			return nil, fmt.Errorf("expected column %s to exist in the old schema but did not find it", oldColumn.Name)
-		}
-		// Remove the old column from |root| so that its kind will not seed the
-		// new tag.
-		root, err = filterColumnFromRoot(ctx, root, oldCol.Tag)
-		if err != nil {
-			return nil, err
-		}
-	}
-
+	// Modifying a column
 	newSch, err := sqlutil.ToDoltSchema(ctx, root, t.Name(), newSchema, headRoot, sql.CollationID(oldSch.GetCollation()))
 	if err != nil {
 		return nil, err
 	}
 
-	return newSch, nil
+	oldDoltCol, ok := oldSch.GetAllCols().GetByName(oldColumn.Name)
+	if !ok {
+		return nil, fmt.Errorf("expected column %s to exist in the old schema but did not find it", oldColumn.Name)
+	}
+
+	newColCollection := replaceColumnTagInCollection(newSch.GetAllCols(), oldDoltCol.Name, oldDoltCol.Tag)
+	newPkColCollection := replaceColumnTagInCollection(newSch.GetPKCols(), oldDoltCol.Name, oldDoltCol.Tag)
+	return schema.NewSchema(newColCollection, newSch.GetPkOrdinals(), newSch.GetCollation(),
+		schema.NewIndexCollection(newColCollection, newPkColCollection), newSch.Checks())
 }
 
-// filterColumnFromRoot removes any columns matching |colTag| from a |root|. Returns the updated root.
-func filterColumnFromRoot(ctx context.Context, root *doltdb.RootValue, colTag uint64) (*doltdb.RootValue, error) {
-	newRoot := root
-	err := root.IterTables(ctx, func(name string, table *doltdb.Table, sch schema.Schema) (stop bool, err error) {
-		_, ok := sch.GetAllCols().GetByTag(colTag)
-		if !ok {
-			return false, nil
+// replaceColumnTagInCollection returns a new ColCollection, based on |cc|, with the column named |name| updated
+// to have the specified column tag |tag|. If the column is not found, no changes are made, no errors are returned,
+// and the returned ColCollection will be identical to |cc|.
+func replaceColumnTagInCollection(cc *schema.ColCollection, name string, tag uint64) *schema.ColCollection {
+	newColumns := cc.GetColumns()
+	for i := range newColumns {
+		if newColumns[i].Name == name {
+			newColumns[i].Tag = tag
+			break
 		}
-
-		newSch, err := filterColumnFromSch(sch, colTag)
-		if err != nil {
-			return true, err
-		}
-		t, err := table.UpdateSchema(ctx, newSch)
-		if err != nil {
-			return true, err
-		}
-		newRoot, err = newRoot.PutTable(ctx, name, t)
-		if err != nil {
-			return true, err
-		}
-		return false, nil
-	})
-	if err != nil {
-		return nil, err
 	}
-	return newRoot, nil
-}
-
-func filterColumnFromSch(sch schema.Schema, colTag uint64) (schema.Schema, error) {
-	var cols []schema.Column
-	_ = sch.GetAllCols().Iter(func(tag uint64, col schema.Column) (stop bool, err error) {
-		if tag == colTag {
-			return false, nil
-		}
-		cols = append(cols, col)
-		return false, nil
-	})
-	colCol := schema.NewColCollection(cols...)
-	newSch, err := schema.SchemaFromCols(colCol)
-	if err != nil {
-		return nil, err
-	}
-	return newSch, nil
+	return schema.NewColCollection(newColumns...)
 }
 
 // validateSchemaChange returns an error if the schema change given is not legal

integration-tests/bats/column_tags.bats

@@ -240,7 +240,7 @@ DELIM
     [[ "$output" =~ "ints_table,c5,12796" ]] || false
 }
 
-@test "column_tags: Round-tripping a column type through different NomsKinds restores original tag" {
+@test "column_tags: Round-tripping a column type through different NomsKinds preserves its tag" {
     dolt sql -q "CREATE TABLE t (pk INT PRIMARY KEY, col1 int);"
     run dolt schema tags
     [ $status -eq 0 ]
@@ -249,7 +249,7 @@ DELIM
     dolt sql -q "ALTER TABLE t MODIFY COLUMN col1 VARCHAR(100);"
     run dolt schema tags
     [ $status -eq 0 ]
-    [[ $output =~ "col1   | 16050" ]] || false
+    [[ $output =~ "col1   | 10878" ]] || false
 
     dolt sql -q "ALTER TABLE t MODIFY COLUMN col1 int;"
     run dolt schema tags
@@ -274,7 +274,7 @@ DELIM
     [[ $output =~ "col1   | 16050" ]] || false
 }
 
-@test "column_tags: Round-tripping a column type after some other column has been altered" {
+@test "column_tags: Round-tripping a column type after other column is altered preserves column tag" {
     dolt sql -q "CREATE TABLE t (pk INT PRIMARY KEY, col1 int);"
     run dolt schema tags
     [ $status -eq 0 ]
@@ -285,12 +285,12 @@ DELIM
     dolt sql -q "ALTER TABLE t MODIFY COLUMN col1 VARCHAR(100);"
     run dolt schema tags
     [ $status -eq 0 ]
-    [[ $output =~ "col1   | 11127" ]] || false
+    [[ $output =~ "col1   | 10878" ]] || false
 
     dolt sql -q "ALTER TABLE t MODIFY COLUMN col1 int;"
     run dolt schema tags
     [ $status -eq 0 ]
-    [[ $output =~ "col1   | 10186" ]] || false
+    [[ $output =~ "col1   | 10878" ]] || false
 }
 
 @test "column_tags: update-tag only available on __DOLT__" {