Skip to content

Commit

Permalink
Merge pull request #6440 from dolthub/fulghum/dolt-6261
Browse files Browse the repository at this point in the history
  • Loading branch information
fulghum authored Aug 3, 2023
2 parents c97d90a + 4ccec7d commit 4a9778c
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 5 deletions.
9 changes: 9 additions & 0 deletions go/libraries/doltcore/merge/type_compatibility.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,15 @@ func (d doltTypeCompatibilityChecker) IsTypeChangeCompatible(from, to typeinfo.T
}

switch {
case fromSqlType.Type() == query.Type_VARCHAR && toSqlType.Type() == query.Type_VARCHAR:
fromStringType := fromSqlType.(types.StringType)
toStringType := toSqlType.(types.StringType)
// Varchar data is stored directly in the index, in a variable length field that includes
// the data's length, so widening the type doesn't require a rewrite and doesn't affect
// any existing data.
return toStringType.Length() >= fromStringType.Length() &&
toStringType.Collation() == fromStringType.Collation()

case types.IsEnum(fromSqlType) && types.IsEnum(toSqlType):
fromEnumType := fromSqlType.(sql.EnumType)
toEnumType := toSqlType.(sql.EnumType)
Expand Down
95 changes: 90 additions & 5 deletions go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -4872,8 +4872,6 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{
},
},
},

// Constraints: Check Constraint Violations
{
Name: "check constraint violation - simple case, no schema changes",
AncSetUpScript: []string{
Expand Down Expand Up @@ -4903,9 +4901,8 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{
},
},
},

// Constraints: Check Constraint Coercion
{
// Check Constraint Coercion:
// MySQL doesn't allow creating non-boolean check constraint
// expressions, but we currently allow it. Eventually we should
// close this gap and then we wouldn't need to coerce return values.
Expand Down Expand Up @@ -4967,7 +4964,6 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{
},
},
},

{
Name: "check constraint violation - schema change",
AncSetUpScript: []string{
Expand Down Expand Up @@ -5143,6 +5139,45 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{
},

// Resolvable type changes
{
Name: "varchar widening",
AncSetUpScript: []string{
"set autocommit = 0;",
"CREATE table t (pk int primary key, col1 varchar(10));",
"INSERT into t values (1, '123');",
"alter table t add index idx1 (col1);",
},
RightSetUpScript: []string{
"alter table t modify column col1 varchar(100);",
"INSERT into t values (2, '12345678901234567890');",
},
LeftSetUpScript: []string{
"INSERT into t values (3, '321');",
},
Assertions: []queries.ScriptTestAssertion{
{
Query: "call dolt_merge('right');",
Expected: []sql.Row{{doltCommit, 0, 0}},
},
{
Query: "select * from t;",
Expected: []sql.Row{{1, "123"}, {2, "12345678901234567890"}, {3, "321"}},
},
{
Query: "show create table t;",
Expected: []sql.Row{{"t", "CREATE TABLE `t` (\n" +
" `pk` int NOT NULL,\n" +
" `col1` varchar(100),\n" +
" PRIMARY KEY (`pk`),\n" +
" KEY `idx1` (`col1`)\n" +
") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}},
},
{
Query: "insert into t values (4, 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJJKLMNOPQRSTUVWXYZ!@#$%^&*()_+');",
Expected: []sql.Row{{types.NewOkResult(1)}},
},
},
},
{
Name: "type widening - enums and sets",
AncSetUpScript: []string{
Expand Down Expand Up @@ -5173,6 +5208,31 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{
},
},
},
{
Name: "varchar widening to TEXT",
AncSetUpScript: []string{
"set autocommit = 0;",
"CREATE table t (pk int primary key, col1 varchar(10));",
"INSERT into t values (1, '123');",
},
RightSetUpScript: []string{
"alter table t modify column col1 TEXT;",
"INSERT into t values (2, '12345678901234567890');",
},
LeftSetUpScript: []string{
"INSERT into t values (3, '321');",
},
Assertions: []queries.ScriptTestAssertion{
{
// TODO: We should be able to automatically widen a VARCHAR field to TEXT, but because TEXT data is
// encoded differently (stored separate from the index data), this merge will require rewriting
// any existing data in the table.
Skip: true,
Query: "call dolt_merge('right');",
Expected: []sql.Row{{doltCommit, 0, 0}},
},
},
},

// Schema conflicts
{
Expand Down Expand Up @@ -5253,6 +5313,31 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{
},
},
},
{
Name: "varchar shortening",
AncSetUpScript: []string{
"set autocommit = 0;",
"CREATE table t (pk int primary key, col1 varchar(10));",
"INSERT into t values (1, '123');",
},
RightSetUpScript: []string{
"alter table t modify column col1 varchar(9);",
"INSERT into t values (2, '12345');",
},
LeftSetUpScript: []string{
"INSERT into t values (3, '321');",
},
Assertions: []queries.ScriptTestAssertion{
{
Query: "call dolt_merge('right');",
Expected: []sql.Row{{"", 0, 1}},
},
{
Query: "select table_name, description like 'incompatible column types for column ''col1''%' from dolt_schema_conflicts;",
Expected: []sql.Row{{"t", true}},
},
},
},
{
// Dolt indexes currently use the set of columns covered by the index, as a unique identifier for matching
// indexes on either side of a merge. As Dolt's index support has grown, this isn't guaranteed to be a unique
Expand Down

0 comments on commit 4a9778c

Please sign in to comment.