Skip to content

Commit

Permalink
Changing dolt_history table column conversion warning to only emit on…
Browse files Browse the repository at this point in the history
…e session warning per column
  • Loading branch information
fulghum committed Apr 30, 2024
1 parent eb0a3e4 commit 8feee63
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 23 deletions.
24 changes: 15 additions & 9 deletions go/libraries/doltcore/sqle/enginetest/dolt_queries.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}},
},
{
Expand All @@ -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 " +
Expand Down
33 changes: 19 additions & 14 deletions go/libraries/doltcore/sqle/history_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand All @@ -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{}{}
}
}
}
}
Expand Down

0 comments on commit 8feee63

Please sign in to comment.