Merge pull request #7776 from dolthub/fulghum/patch-ddl

`dolt_history` tables: Issue a warning when types have changed

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

@@ -2049,26 +2049,32 @@ var HistorySystemTableScriptTests = []queries.ScriptTest{
 		Name: "primary key table: non-pk column type changes",
 		SetUpScript: []string{
 			"create table t (pk int primary key, c1 int, c2 varchar(20));",
-			"call dolt_add('.')",
+			"CALL DOLT_COMMIT('-Am', 'creating table t');",
+			"set @Commit1 = dolt_hashof('HEAD');",
+
 			"insert into t values (1, 2, '3'), (4, 5, '6');",
-			"set @Commit1 = '';",
-			"CALL DOLT_COMMIT_HASH_OUT(@Commit1, '-am', 'creating table t');",
+			"CALL DOLT_COMMIT('-Am', 'inserting two rows');",
+			"set @Commit2 = dolt_hashof('HEAD');",
+
+			"CALL DOLT_COMMIT('--allow-empty', '-m', 'empty commit');",
+			"set @Commit3 = dolt_hashof('HEAD');",
+
 			"alter table t modify column c2 int;",
-			"set @Commit2 = '';",
-			"CALL DOLT_COMMIT_HASH_OUT(@Commit2, '-am', 'changed type of c2');",
+			"CALL DOLT_COMMIT('-am', 'changed type of c2');",
+			"set @Commit4 = dolt_hashof('HEAD');",
 		},
 		Assertions: []queries.ScriptTestAssertion{
 			{
 				Query:    "select count(*) from dolt_history_t;",
-				Expected: []sql.Row{{4}},
+				Expected: []sql.Row{{6}},
 			},
 			// Can't represent the old schema in the current one, so it gets nil valued
 			{
-				Query:    "select pk, c2 from dolt_history_t where commit_hash=@Commit1 order by pk;",
+				Query:    "select pk, c2 from dolt_history_t where commit_hash=@Commit2 order by pk;",
 				Expected: []sql.Row{{1, nil}, {4, nil}},
 			},
 			{
-				Query:    "select pk, c2 from dolt_history_t where commit_hash=@Commit2 order by pk;",
+				Query:    "select pk, c2 from dolt_history_t where commit_hash=@Commit4 order by pk;",
 				Expected: []sql.Row{{1, 3}, {4, 6}},
 			},
 			{
@@ -2077,11 +2083,15 @@ var HistorySystemTableScriptTests = []queries.ScriptTest{
 				// 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}, {4, 5, nil}},
+				//       version's tuple, so it shows up as a NULL and a SQL warning in the session. In the future,
+				//       we could consider using a different tuple descriptor based on the version of the row and
+				//       pull the data out and try to convert it to the new type.
+				Query:                 "select pk, c1, c2 from dolt_history_t where pk=4;",
+				Expected:              []sql.Row{{4, 5, 6}, {4, 5, nil}, {4, 5, nil}},
+				ExpectedWarning:       1246,
+				ExpectedWarningsCount: 1,
+				ExpectedWarningMessageSubstring: "Unable to convert field c2 in historical rows because " +
+					"its type (int) doesn't match current schema's type (varchar(20))",
 			},
 		},
 	},

go/libraries/doltcore/sqle/history_table.go

@@ -59,12 +59,13 @@ var _ sql.PrimaryKeyTable = (*HistoryTable)(nil)
 
 // HistoryTable is a system table that shows the history of rows over time
 type HistoryTable struct {
-	doltTable     *DoltTable
-	commitFilters []sql.Expression
-	cmItr         doltdb.CommitItr
-	commitCheck   doltdb.CommitFilter
-	indexLookup   sql.IndexLookup
-	projectedCols []uint64
+	doltTable                  *DoltTable
+	commitFilters              []sql.Expression
+	cmItr                      doltdb.CommitItr
+	commitCheck                doltdb.CommitFilter
+	indexLookup                sql.IndexLookup
+	projectedCols              []uint64
+	conversionWarningsByColumn map[string]struct{}
 }
 
 func (ht *HistoryTable) PrimaryKeySchema() sql.PrimaryKeySchema {
@@ -179,8 +180,9 @@ func NewHistoryTable(table *DoltTable, ddb *doltdb.DoltDB, head *doltdb.Commit)
 	}
 
 	h := &HistoryTable{
-		doltTable: table,
-		cmItr:     cmItr,
+		doltTable:                  table,
+		cmItr:                      cmItr,
+		conversionWarningsByColumn: make(map[string]struct{}),
 	}
 	return h
 }
@@ -436,7 +438,7 @@ func (ht *HistoryTable) Partitions(ctx *sql.Context) (sql.PartitionIter, error)
 // PartitionRows takes a partition and returns a row iterator for that partition
 func (ht *HistoryTable) PartitionRows(ctx *sql.Context, part sql.Partition) (sql.RowIter, error) {
 	cp := part.(*commitPartition)
-	return newRowItrForTableAtCommit(ctx, ht.doltTable, cp.h, cp.cm, ht.indexLookup, ht.ProjectedTags())
+	return ht.newRowItrForTableAtCommit(ctx, ht.doltTable, cp.h, cp.cm, ht.indexLookup, ht.ProjectedTags())
 }
 
 // commitPartition is a single commit
@@ -483,7 +485,7 @@ type historyIter struct {
 	nonExistentTable bool
 }
 
-func newRowItrForTableAtCommit(ctx *sql.Context, table *DoltTable, h hash.Hash, cm *doltdb.Commit, lookup sql.IndexLookup, projections []uint64) (*historyIter, error) {
+func (ht *HistoryTable) newRowItrForTableAtCommit(ctx *sql.Context, table *DoltTable, h hash.Hash, cm *doltdb.Commit, lookup sql.IndexLookup, projections []uint64) (*historyIter, error) {
 	targetSchema := table.Schema()
 
 	root, err := cm.GetRootValue(ctx)
@@ -542,7 +544,7 @@ func newRowItrForTableAtCommit(ctx *sql.Context, table *DoltTable, h hash.Hash,
 		}
 	}
 
-	converter := rowConverter(lockedTable.Schema(), targetSchema, h, meta, projections)
+	converter := ht.rowConverter(ctx, lockedTable.Schema(), targetSchema, h, meta, projections)
 	return &historyIter{
 		table:           histTable,
 		tablePartitions: partIter,
@@ -587,7 +589,11 @@ func (i *historyIter) Close(ctx *sql.Context) error {
 	return nil
 }
 
-func rowConverter(srcSchema, targetSchema sql.Schema, h hash.Hash, meta *datas.CommitMeta, projections []uint64) func(row sql.Row) sql.Row {
+// rowConverter returns a function that converts a row to another schema for the dolt_history system tables. |srcSchema|
+// describes the incoming row, |targetSchema| describes the desired row schema, and |projections| controls which fields
+// are including the returned row. The hash |h| and commit metadata |meta| are used to augment the row with custom
+// fields for the dolt_history table to return commit metadata.
+func (ht *HistoryTable) rowConverter(ctx *sql.Context, srcSchema, targetSchema sql.Schema, h hash.Hash, meta *datas.CommitMeta, projections []uint64) func(row sql.Row) sql.Row {
 	srcToTarget := make(map[int]int)
 	for i, col := range targetSchema {
 		srcIdx := srcSchema.IndexOfColName(col.Name)
@@ -596,6 +602,12 @@ func rowConverter(srcSchema, targetSchema sql.Schema, h hash.Hash, meta *datas.C
 			// TODO: we could do a projection to convert between types in some cases
 			if srcSchema[srcIdx].Type.Equals(targetSchema[i].Type) {
 				srcToTarget[srcIdx] = i
+			} else {
+				if _, alreadyWarned := ht.conversionWarningsByColumn[col.Name]; !alreadyWarned {
+					ctx.Warn(1246, "Unable to convert field %s in historical rows because its type (%s) doesn't match "+
+						"current schema's type (%s)", col.Name, col.Type.String(), srcSchema[srcIdx].Type.String())
+					ht.conversionWarningsByColumn[col.Name] = struct{}{}
+				}
 			}
 		}
 	}