Adding tests for joins when schema is mapped; adding support for simple type conversion

2026-05-23 11:39:56 -05:00 · 2024-02-21 16:26:50 -08:00
parent 7db3825f0d
commit 6a4c99b91d
2 changed files with 172 additions and 108 deletions
@@ -121,15 +121,15 @@ func TestSingleQuery(t *testing.T) {
 func TestSchemaPinning(t *testing.T) {
 	var scripts = []queries.ScriptTest{

-		// TODO: Test doing a join on tables when the schema is pinned
 		// TODO: Add more tests with different projection changes (no values, PK not at front of row, etc)
-		// NOTE: If we do have to turn off indexes, will that affect other things, like foreign keys, right?
-		// TODO: Test unsetting the schema override var!
+		// TODO: Test unsetting the schema override var
+		// TODO: Test PK changes – they should error out, right?
+		// TODO: Test DDL operations – they should error out
+		//       How do we get into the right point of the analyzer to check for this?

 		// TODO:
 		//  - Deleting a column (in the middle of a schema – or perhaps deleting multiple columns, at start, middle, and end?
-		//  - Adding a new column – start, middle, and end of schema? (this is covered pretty well by a test case below!)
-		//  - Modifying columns ???
+		//  - Modifying columns – renaming and changing type

 		// BASIC OPERATIONS
 		{
@@ -304,102 +304,6 @@ func TestSchemaPinning(t *testing.T) {
 				},
 			},
 		},
-
-		// TABLE EXISTENCE EDGE CASES
-		{
-			Name: "Table exists in the pinned schema, but not in the data commit",
-			SetUpScript: []string{
-				"SET @commit1 = hashof('HEAD');",
-				"create table t (pk int primary key, c1 varchar(255));",
-				"insert into t values (1, 'one');",
-				"call dolt_commit('-Am', 'adding table t on main');",
-				"SET @commit2 = hashof('HEAD');",
-			},
-			Assertions: []queries.ScriptTestAssertion{
-				{
-					Query:    "SELECT * from t;",
-					Expected: []sql.Row{{1, "one"}},
-				},
-				{
-					Query:    "SET @@dolt_override_schema=@commit2;",
-					Expected: []sql.Row{{}},
-				},
-				{
-					Query:    "SELECT * from t;",
-					Expected: []sql.Row{{1, "one"}},
-				},
-				{
-					// TODO: This table exists in the pinned schema, but not in the data commit.
-					//       We need to update the database type to handle this and somehow return
-					//       an empty table with the pinned schema.
-					Query:    "SELECT * from t as of @commit1;",
-					Expected: []sql.Row{},
-					ExpectedColumns: sql.Schema{
-						{
-							Name: "pk",
-							Type: gmstypes.Int32,
-						}, {
-							Name: "c1",
-							Type: gmstypes.MustCreateStringWithDefaults(sqltypes.VarChar, 255),
-						},
-					},
-				},
-			},
-		},
-
-		// INDEX TEST CASES
-		{
-			Name: "Index exists in the pinned schema, but not in the data commit",
-			SetUpScript: []string{
-				"create table t (pk int primary key, c1 varchar(255), key c1_idx(c1));",
-				"insert into t values (1, 'one');",
-				"call dolt_commit('-Am', 'adding table t with index on main');",
-				"SET @commit1 = hashof('HEAD');",
-
-				"alter table t drop index c1_idx;",
-				"update t set c1='two';",
-				"call dolt_commit('-Am', 'adding table t with index on main');",
-				"SET @commit2 = hashof('HEAD');",
-			},
-			Assertions: []queries.ScriptTestAssertion{
-				{
-					// Going back to @commit1 with AS OF should use the available index
-					Query:           "SELECT c1 from t as of @commit1 where c1 > 'o';",
-					Expected:        []sql.Row{{"one"}},
-					ExpectedIndexes: []string{"c1_idx"},
-				},
-				{
-					// The tip of HEAD does not have an index
-					Query:           "SELECT c1 from t where c1 > 'o';",
-					Expected:        []sql.Row{{"two"}},
-					ExpectedIndexes: []string{},
-				},
-				{
-					// Set the overridden schema to the point where an index existed
-					Query:    "SET @@dolt_override_schema=@commit1;",
-					Expected: []sql.Row{{}},
-				},
-				{
-					// Using the overridden index, we should still get the latest data, but without using the index
-					Query:           "SELECT c1 from t where c1 > 'o';",
-					Expected:        []sql.Row{{"two"}},
-					ExpectedIndexes: []string{},
-				},
-				{
-					// Set the overridden schema to the point where an index existed
-					Query:    "SET @@dolt_override_schema=@commit2;",
-					Expected: []sql.Row{{}},
-				},
-				{
-					// Going back to @commit1 for data, but using @commit2 for schema
-					Query:           "SELECT c1 from t as of @commit1 where c1 > 'o';",
-					Expected:        []sql.Row{{"one"}},
-					ExpectedIndexes: []string{"c1_idx"},
-				},
-			},
-		},
-
-		// TODO: This should be moved up with the other basic operation tests
 		{
 			// TODO: What other type changes do we need to test/support?
 			// TODO: What happens when the data can't be converted into the mapped schema type? (e.g. -13 -> unsigned int)
@@ -505,7 +409,101 @@ func TestSchemaPinning(t *testing.T) {
 			},
 		},

-		// AS OF TESTS
+		// TABLE EXISTENCE EDGE CASES
+		{
+			Name: "Table exists in the pinned schema, but not in the data commit",
+			SetUpScript: []string{
+				"SET @commit1 = hashof('HEAD');",
+				"create table t (pk int primary key, c1 varchar(255));",
+				"insert into t values (1, 'one');",
+				"call dolt_commit('-Am', 'adding table t on main');",
+				"SET @commit2 = hashof('HEAD');",
+			},
+			Assertions: []queries.ScriptTestAssertion{
+				{
+					Query:    "SELECT * from t;",
+					Expected: []sql.Row{{1, "one"}},
+				},
+				{
+					Query:    "SET @@dolt_override_schema=@commit2;",
+					Expected: []sql.Row{{}},
+				},
+				{
+					Query:    "SELECT * from t;",
+					Expected: []sql.Row{{1, "one"}},
+				},
+				{
+					// TODO: This table exists in the pinned schema, but not in the data commit.
+					//       We need to update the database type to handle this and somehow return
+					//       an empty table with the pinned schema.
+					Query:    "SELECT * from t as of @commit1;",
+					Expected: []sql.Row{},
+					ExpectedColumns: sql.Schema{
+						{
+							Name: "pk",
+							Type: gmstypes.Int32,
+						}, {
+							Name: "c1",
+							Type: gmstypes.MustCreateStringWithDefaults(sqltypes.VarChar, 255),
+						},
+					},
+				},
+			},
+		},
+
+		// INDEX TEST CASES
+		{
+			Name: "Index exists in the pinned schema, but not in the data commit",
+			SetUpScript: []string{
+				"create table t (pk int primary key, c1 varchar(255), key c1_idx(c1));",
+				"insert into t values (1, 'one');",
+				"call dolt_commit('-Am', 'adding table t with index on main');",
+				"SET @commit1 = hashof('HEAD');",
+
+				"alter table t drop index c1_idx;",
+				"update t set c1='two';",
+				"call dolt_commit('-Am', 'adding table t with index on main');",
+				"SET @commit2 = hashof('HEAD');",
+			},
+			Assertions: []queries.ScriptTestAssertion{
+				{
+					// Going back to @commit1 with AS OF should use the available index
+					Query:           "SELECT c1 from t as of @commit1 where c1 > 'o';",
+					Expected:        []sql.Row{{"one"}},
+					ExpectedIndexes: []string{"c1_idx"},
+				},
+				{
+					// The tip of HEAD does not have an index
+					Query:           "SELECT c1 from t where c1 > 'o';",
+					Expected:        []sql.Row{{"two"}},
+					ExpectedIndexes: []string{},
+				},
+				{
+					// Set the overridden schema to the point where an index existed
+					Query:    "SET @@dolt_override_schema=@commit1;",
+					Expected: []sql.Row{{}},
+				},
+				{
+					// Using the overridden index, we should still get the latest data, but without using the index
+					Query:           "SELECT c1 from t where c1 > 'o';",
+					Expected:        []sql.Row{{"two"}},
+					ExpectedIndexes: []string{},
+				},
+				{
+					// Set the overridden schema to the point where an index existed
+					Query:    "SET @@dolt_override_schema=@commit2;",
+					Expected: []sql.Row{{}},
+				},
+				{
+					// Going back to @commit1 for data, but using @commit2 for schema
+					Query:           "SELECT c1 from t as of @commit1 where c1 > 'o';",
+					Expected:        []sql.Row{{"one"}},
+					ExpectedIndexes: []string{"c1_idx"},
+				},
+			},
+		},
+
+		// AS OF TEST CASES
 		{
 			Name: "AS OF with schema pinning",
 			SetUpScript: []string{
@@ -578,6 +576,53 @@ func TestSchemaPinning(t *testing.T) {
 				},
 			},
 		},
+
+		// JOIN TEST CASES
+		{
+			Name: "Joining two tables with changed schemas",
+			SetUpScript: []string{
+				"create table t1 (pk int primary key, c1 varchar(255));",
+				"create table t2 (pk int primary key, c1 int, c2 varchar(100));",
+				"insert into t1 values (1, 'one');",
+				"insert into t2 values (100, 1, 'blue');",
+				"call dolt_commit('-Am', 'adding tables t1 and t2 on main');",
+				"SET @commit1 = hashof('HEAD');",
+
+				"alter table t1 rename column c1 to c2;",
+				"alter table t2 modify column c1 varchar(100);",
+				"call dolt_commit('-am', 'modifying columns in t1 and t2 on main');",
+				"SET @commit2 = hashof('HEAD');",
+			},
+			Assertions: []queries.ScriptTestAssertion{
+				{
+					// use the tip of main for our response schema (pk, c2)
+					Query:    "SET @@dolt_override_schema=@commit1;",
+					Expected: []sql.Row{{}},
+				},
+				{
+					Query:    "SELECT * from t1 JOIN t2 on t1.pk = t2.c1;",
+					Expected: []sql.Row{{1, "one", 100, 1, "blue"}},
+					ExpectedColumns: sql.Schema{
+						{
+							Name: "pk",
+							Type: gmstypes.Int32,
+						}, {
+							Name: "c1",
+							Type: gmstypes.MustCreateStringWithDefaults(sqltypes.VarChar, 255),
+						}, {
+							Name: "pk",
+							Type: gmstypes.Int32,
+						}, {
+							Name: "c1",
+							Type: gmstypes.Int32,
+						}, {
+							Name: "c2",
+							Type: gmstypes.MustCreateStringWithDefaults(sqltypes.VarChar, 255),
+						},
+					},
+				},
+			},
+		},
 	}

 	tcc := &testCommitClock{}
@@ -28,6 +28,7 @@ import (

 	"github.com/dolthub/dolt/go/libraries/doltcore/doltdb"
 	"github.com/dolthub/dolt/go/libraries/doltcore/schema"
+	"github.com/dolthub/dolt/go/libraries/doltcore/schema/typeinfo"
 	"github.com/dolthub/dolt/go/libraries/doltcore/sqle/index"
 	"github.com/dolthub/dolt/go/libraries/utils/set"
 	"github.com/dolthub/dolt/go/store/datas"
@@ -577,10 +578,18 @@ func (i *historyIter) Close(ctx *sql.Context) error {
 	return nil
 }

-// NOTE: This was forked from rowConverter to handle the logic starting to separate
-// TODO: Add Godocs; move to another file?
+// rowConverterByColName returns a function that converts a row from |srcSchema| to |targetSchema| using the
+// specified |projectedTags| and |projectedColNames|. Projected tags and projected column names are both
+// provided so that if a tag changes (such as when a column's type is changed) the mapping can fall back to
+// matching by column name.
+//
+// NOTE: This was forked from the dolt_history system table's rowConverter function, to handle the logic
+// starting to diverge for schema mapping. It would be nice to deduplicate and clean this up.
+//
+// TODO: move to another file; shouldn't be part of the history system table
 func rowConverterByColName(srcSchema, targetSchema schema.Schema, projectedTags []uint64, projectedColNames []string) func(row sql.Row) sql.Row {
-	srcToTarget := make(map[int]int)
+	srcIndexToTargetIndex := make(map[int]int)
+	srcIndexToTargetType := make(map[int]typeinfo.TypeInfo)
 	for i, targetColumn := range targetSchema.GetAllCols().GetColumns() {
 		sourceColumn, found := srcSchema.GetAllCols().GetByTag(targetColumn.Tag)
 		if !found {
@@ -588,8 +597,9 @@ func rowConverterByColName(srcSchema, targetSchema schema.Schema, projectedTags
 		}

 		if found {
-			// TODO: Do we need to consider any type conversion here?
-			srcToTarget[srcSchema.GetAllCols().IndexOf(sourceColumn.Name)] = i
+			srcIndex := srcSchema.GetAllCols().IndexOf(sourceColumn.Name)
+			srcIndexToTargetIndex[srcIndex] = i
+			srcIndexToTargetType[srcIndex] = targetColumn.TypeInfo
 		}
 	}

@@ -604,7 +614,16 @@ func rowConverterByColName(srcSchema, targetSchema schema.Schema, projectedTags
 			}

 			if found {
-				r[i] = row[srcSchema.GetAllCols().IndexOf(srcColumn.Name)]
+				srcIndex := srcSchema.GetAllCols().IndexOf(srcColumn.Name)
+				temp := row[srcIndex]
+
+				temp, _, err := srcIndexToTargetType[srcIndex].ToSqlType().Convert(temp)
+				if err != nil {
+					// TODO: This function should return an error so we don't have to panic here
+					panic("unable to convert value: " + err.Error())
+				}
+
+				r[i] = temp
 			}
 		}
 		return r