From a9c3c841d065444d0413a94fe669fe6718b1f8f9 Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Wed, 2 Aug 2023 10:17:05 -0700 Subject: [PATCH 1/2] Auto-merge type changes that widen varchar columns. Fixes https://github.com/dolthub/dolt/issues/6261 --- .../doltcore/merge/type_compatibility.go | 9 ++ .../sqle/enginetest/dolt_queries_merge.go | 90 +++++++++++++++++-- 2 files changed, 94 insertions(+), 5 deletions(-) diff --git a/go/libraries/doltcore/merge/type_compatibility.go b/go/libraries/doltcore/merge/type_compatibility.go index ea12a6c32e..229acae2a1 100644 --- a/go/libraries/doltcore/merge/type_compatibility.go +++ b/go/libraries/doltcore/merge/type_compatibility.go @@ -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) diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go index 42f4848a35..f80475396c 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go @@ -4872,8 +4872,6 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{ }, }, }, - - // Constraints: Check Constraint Violations { Name: "check constraint violation - simple case, no schema changes", AncSetUpScript: []string{ @@ -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. @@ -4967,7 +4964,6 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{ }, }, }, - { Name: "check constraint violation - schema change", AncSetUpScript: []string{ @@ -5143,6 +5139,40 @@ 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{ @@ -5173,6 +5203,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 { @@ -5253,6 +5308,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 From 4ccec7d4edd89c7baa056707b28d9b36e43bf498 Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Wed, 2 Aug 2023 17:38:03 -0700 Subject: [PATCH 2/2] PR feedback --- go/libraries/doltcore/merge/type_compatibility.go | 2 +- .../doltcore/sqle/enginetest/dolt_queries_merge.go | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/go/libraries/doltcore/merge/type_compatibility.go b/go/libraries/doltcore/merge/type_compatibility.go index 229acae2a1..b285afa9e2 100644 --- a/go/libraries/doltcore/merge/type_compatibility.go +++ b/go/libraries/doltcore/merge/type_compatibility.go @@ -75,7 +75,7 @@ func (d doltTypeCompatibilityChecker) IsTypeChangeCompatible(from, to typeinfo.T // 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() && + return toStringType.Length() >= fromStringType.Length() && toStringType.Collation() == fromStringType.Collation() case types.IsEnum(fromSqlType) && types.IsEnum(toSqlType): diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go index f80475396c..6e28607dde 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go @@ -5164,8 +5164,13 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{ 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: "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!@#$%^&*()_+');",