From 799357e059f57d3596e68de967e981dbee70e991 Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Mon, 22 Apr 2024 17:39:58 -0700 Subject: [PATCH 1/2] Adding a session warning when dolt_history tables can't create a rowConverter for a row due to a field's type changing. --- .../doltcore/sqle/enginetest/dolt_queries.go | 14 +++++++++----- go/libraries/doltcore/sqle/history_table.go | 11 +++++++++-- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries.go index bf5d61845e..02ba5fc0e2 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries.go @@ -2077,11 +2077,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}}, + 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))", }, }, }, diff --git a/go/libraries/doltcore/sqle/history_table.go b/go/libraries/doltcore/sqle/history_table.go index 291132de1c..db795c5a05 100644 --- a/go/libraries/doltcore/sqle/history_table.go +++ b/go/libraries/doltcore/sqle/history_table.go @@ -542,7 +542,7 @@ func newRowItrForTableAtCommit(ctx *sql.Context, table *DoltTable, h hash.Hash, } } - converter := rowConverter(lockedTable.Schema(), targetSchema, h, meta, projections) + converter := rowConverter(ctx, lockedTable.Schema(), targetSchema, h, meta, projections) return &historyIter{ table: histTable, tablePartitions: partIter, @@ -587,7 +587,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 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 +600,9 @@ 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 { + 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()) } } } From 8feee63d346e2ba4ced33d0220fb99e4ef655fc1 Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Tue, 30 Apr 2024 10:21:00 -0700 Subject: [PATCH 2/2] Changing dolt_history table column conversion warning to only emit one session warning per column --- .../doltcore/sqle/enginetest/dolt_queries.go | 24 +++++++++----- go/libraries/doltcore/sqle/history_table.go | 33 +++++++++++-------- 2 files changed, 34 insertions(+), 23 deletions(-) diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries.go index 02ba5fc0e2..5295d33f86 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries.go +++ b/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}}, }, { @@ -2081,7 +2087,7 @@ var HistorySystemTableScriptTests = []queries.ScriptTest{ // 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}}, + 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 " + diff --git a/go/libraries/doltcore/sqle/history_table.go b/go/libraries/doltcore/sqle/history_table.go index db795c5a05..9767e4f1de 100644 --- a/go/libraries/doltcore/sqle/history_table.go +++ b/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(ctx, lockedTable.Schema(), targetSchema, h, meta, projections) + converter := ht.rowConverter(ctx, lockedTable.Schema(), targetSchema, h, meta, projections) return &historyIter{ table: histTable, tablePartitions: partIter, @@ -591,7 +593,7 @@ func (i *historyIter) Close(ctx *sql.Context) error { // 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 rowConverter(ctx *sql.Context, srcSchema, targetSchema sql.Schema, h hash.Hash, meta *datas.CommitMeta, projections []uint64) func(row sql.Row) sql.Row { +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) @@ -601,8 +603,11 @@ func rowConverter(ctx *sql.Context, srcSchema, targetSchema sql.Schema, h hash.H if srcSchema[srcIdx].Type.Equals(targetSchema[i].Type) { srcToTarget[srcIdx] = i } else { - 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()) + 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{}{} + } } } }