From 070127bf9274cb48b8850d98264c8ded03a567f9 Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Thu, 20 Jul 2023 14:00:11 -0700 Subject: [PATCH] Adding support for: 1) data diff patch statements when there are schema changes, and 2) charset/collation change patch statements. --- go/libraries/doltcore/diff/diffsplitter.go | 15 +- .../sqle/dolt_patch_table_function.go | 18 +- .../sqle/enginetest/dolt_queries_diff.go | 166 ++++++++++++------ .../doltcore/sqle/sqlfmt/schema_fmt.go | 9 + 4 files changed, 145 insertions(+), 63 deletions(-) diff --git a/go/libraries/doltcore/diff/diffsplitter.go b/go/libraries/doltcore/diff/diffsplitter.go index 2bcecdb80f..e39e59f5da 100644 --- a/go/libraries/doltcore/diff/diffsplitter.go +++ b/go/libraries/doltcore/diff/diffsplitter.go @@ -118,9 +118,6 @@ func mapQuerySchemaToTargetSchema(query, target sql.Schema) (mapping []int, err } else { return nil, errors.New("expected column prefix of 'to_' or 'from_' (" + col.Name + ")") } - if mapping[i] < 0 { // sanity check - return nil, errors.New("failed to map diff column: " + col.Name) - } } return } @@ -163,6 +160,10 @@ func (ds DiffSplitter) SplitDiffResultRow(row sql.Row) (from, to RowDiff, err er from.RowDiff = Removed for i := 0; i < ds.splitIdx; i++ { j := ds.queryToTarget[i] + // skip any columns that aren't mapped + if j < 0 { + continue + } from.Row[j] = row[i] from.ColDiffs[j] = Removed } @@ -172,6 +173,10 @@ func (ds DiffSplitter) SplitDiffResultRow(row sql.Row) (from, to RowDiff, err er to.RowDiff = Added for i := ds.splitIdx; i < len(row); i++ { j := ds.queryToTarget[i] + // skip any columns that aren't mapped + if j < 0 { + continue + } to.Row[j] = row[i] to.ColDiffs[j] = Added } @@ -181,6 +186,10 @@ func (ds DiffSplitter) SplitDiffResultRow(row sql.Row) (from, to RowDiff, err er from.RowDiff = ModifiedOld for i := 0; i < ds.splitIdx; i++ { j := ds.queryToTarget[i] + // skip any columns that aren't mapped + if j < 0 { + continue + } from.Row[j] = row[i] } to.Row = make(sql.Row, len(ds.targetSch)) diff --git a/go/libraries/doltcore/sqle/dolt_patch_table_function.go b/go/libraries/doltcore/sqle/dolt_patch_table_function.go index cb41499d48..b099cc61b0 100644 --- a/go/libraries/doltcore/sqle/dolt_patch_table_function.go +++ b/go/libraries/doltcore/sqle/dolt_patch_table_function.go @@ -424,16 +424,7 @@ func canGetDataDiff(ctx *sql.Context, td diff.TableDelta) bool { }) return false } - // cannot sql diff - if td.ToSch == nil || (td.FromSch != nil && !schema.SchemasAreEqual(td.FromSch, td.ToSch)) { - // TODO(8/24/22 Zach): this is overly broad, we can absolutely do better - ctx.Session.Warn(&sql.Warning{ - Level: "Warning", - Code: mysql.ERNotSupportedYet, - Message: fmt.Sprintf("Incompatible schema change, skipping data diff for table '%s'", td.ToName), - }) - return false - } + return true } @@ -574,6 +565,13 @@ func GetNonCreateNonDropTableSqlSchemaDiff(td diff.TableDelta, toSchemas map[str } } + // Handle charset/collation changes + toCollation := toSch.GetCollation() + fromCollation := fromSch.GetCollation() + if toCollation != fromCollation { + ddlStatements = append(ddlStatements, sqlfmt.AlterTableCollateStmt(td.ToName, fromCollation, toCollation)) + } + return ddlStatements, nil } diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries_diff.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries_diff.go index 641105468d..35d7f66638 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries_diff.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries_diff.go @@ -3124,29 +3124,21 @@ var PatchTableFunctionScriptTests = []queries.ScriptTest{ SetUpScript: []string{ "set @Commit0 = HashOf('HEAD');", "create table t (pk int primary key, c1 int, c2 int, c3 int, c4 int, c5 int comment 'tag:5');", - "call dolt_add('.')", "insert into t values (0,1,2,3,4,5), (1,1,2,3,4,5);", - "set @Commit1 = '';", - "call dolt_commit_hash_out(@Commit1, '-am', 'inserting two rows into table t');", + "call dolt_commit('-Am', 'inserting two rows into table t');", "alter table t rename column c1 to c0", "alter table t drop column c4", "alter table t add c6 bigint", }, Assertions: []queries.ScriptTestAssertion{ { - Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch(@Commit1, 'WORKING', 't')", + Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('HEAD', 'WORKING', 't')", Expected: []sql.Row{ {1, "t", "schema", "ALTER TABLE `t` RENAME COLUMN `c1` TO `c0`;"}, {2, "t", "schema", "ALTER TABLE `t` DROP `c4`;"}, {3, "t", "schema", "ALTER TABLE `t` ADD `c6` bigint;"}, - }, - ExpectedWarning: 1235, - ExpectedWarningsCount: 1, - }, - { - Query: "SHOW WARNINGS;", - Expected: []sql.Row{ - {"Warning", 1235, "Incompatible schema change, skipping data diff for table 't'"}, + {4, "t", "data", "UPDATE `t` SET `c0`=1 WHERE `pk`=0;"}, + {5, "t", "data", "UPDATE `t` SET `c0`=1 WHERE `pk`=1;"}, }, }, { @@ -3155,6 +3147,8 @@ var PatchTableFunctionScriptTests = []queries.ScriptTest{ {1, "STAGED", "WORKING", "t", "schema", "ALTER TABLE `t` RENAME COLUMN `c1` TO `c0`;"}, {2, "STAGED", "WORKING", "t", "schema", "ALTER TABLE `t` DROP `c4`;"}, {3, "STAGED", "WORKING", "t", "schema", "ALTER TABLE `t` ADD `c6` bigint;"}, + {4, "STAGED", "WORKING", "t", "data", "UPDATE `t` SET `c0`=1 WHERE `pk`=0;"}, + {5, "STAGED", "WORKING", "t", "data", "UPDATE `t` SET `c0`=1 WHERE `pk`=1;"}, }, }, { @@ -3163,6 +3157,8 @@ var PatchTableFunctionScriptTests = []queries.ScriptTest{ {1, "STAGED", "WORKING", "t", "schema", "ALTER TABLE `t` RENAME COLUMN `c1` TO `c0`;"}, {2, "STAGED", "WORKING", "t", "schema", "ALTER TABLE `t` DROP `c4`;"}, {3, "STAGED", "WORKING", "t", "schema", "ALTER TABLE `t` ADD `c6` bigint;"}, + {4, "STAGED", "WORKING", "t", "data", "UPDATE `t` SET `c0`=1 WHERE `pk`=0;"}, + {5, "STAGED", "WORKING", "t", "data", "UPDATE `t` SET `c0`=1 WHERE `pk`=1;"}, }, }, { @@ -3171,6 +3167,8 @@ var PatchTableFunctionScriptTests = []queries.ScriptTest{ {1, "WORKING", "STAGED", "t", "schema", "ALTER TABLE `t` RENAME COLUMN `c0` TO `c1`;"}, {2, "WORKING", "STAGED", "t", "schema", "ALTER TABLE `t` DROP `c6`;"}, {3, "WORKING", "STAGED", "t", "schema", "ALTER TABLE `t` ADD `c4` int;"}, + {4, "WORKING", "STAGED", "t", "data", "UPDATE `t` SET `c1`=1,`c4`=4 WHERE `pk`=0;"}, + {5, "WORKING", "STAGED", "t", "data", "UPDATE `t` SET `c1`=1,`c4`=4 WHERE `pk`=1;"}, }, }, { @@ -3199,6 +3197,8 @@ var PatchTableFunctionScriptTests = []queries.ScriptTest{ {1, "t", "schema", "ALTER TABLE `t` RENAME COLUMN `c1` TO `c0`;"}, {2, "t", "schema", "ALTER TABLE `t` DROP `c4`;"}, {3, "t", "schema", "ALTER TABLE `t` ADD `c6` bigint;"}, + {4, "t", "data", "UPDATE `t` SET `c0`=1 WHERE `pk`=0;"}, + {5, "t", "data", "UPDATE `t` SET `c0`=1 WHERE `pk`=1;"}, }, }, }, @@ -3208,54 +3208,60 @@ var PatchTableFunctionScriptTests = []queries.ScriptTest{ SetUpScript: []string{ "create table t (pk int primary key, c1 varchar(20), c2 varchar(20));", "call dolt_add('.')", - "set @Commit1 = '';", - "call dolt_commit_hash_out(@Commit1, '-am', 'creating table t');", + "call dolt_commit('-am', 'creating table t');", + "set @Commit1 = hashof('HEAD');", "insert into t values(1, 'one', 'two');", - "set @Commit2 = '';", - "call dolt_commit_hash_out(@Commit2, '-am', 'inserting row 1 into t in main');", + "call dolt_commit('-am', 'inserting row 1 into t in main');", + "set @Commit2 = hashof('HEAD');", "CALL DOLT_checkout('-b', 'branch1');", "alter table t drop column c2;", - "set @Commit3 = '';", - "call dolt_commit_hash_out(@Commit3, '-am', 'dropping column c2 in branch1');", + "call dolt_commit('-am', 'dropping column c2 in branch1');", + "set @Commit3 = hashof('HEAD');", "delete from t where pk=1;", - "set @Commit4 = '';", - "call dolt_commit_hash_out(@Commit4, '-am', 'deleting row 1 in branch1');", + "call dolt_commit('-am', 'deleting row 1 in branch1');", + "set @Commit4 = hashof('HEAD');", "insert into t values (2, 'two');", - "set @Commit5 = '';", - "call dolt_commit_hash_out(@Commit5, '-am', 'inserting row 2 in branch1');", + "call dolt_commit('-am', 'inserting row 2 in branch1');", + "set @Commit5 = hashof('HEAD');", "CALL DOLT_checkout('main');", "insert into t values (2, 'two', 'three');", - "set @Commit6 = '';", - "call dolt_commit_hash_out(@Commit6, '-am', 'inserting row 2 in main');", + "call dolt_commit('-am', 'inserting row 2 in main');", + "set @Commit6 = hashof('HEAD');", "create table newtable (pk int primary key);", "insert into newtable values (1), (2);", - "set @Commit7 = '';", - "call dolt_commit_hash_out(@Commit7, '-Am', 'new table newtable');", + "call dolt_commit('-Am', 'new table newtable');", + "set @Commit7 = hashof('HEAD');", }, Assertions: []queries.ScriptTestAssertion{ { - Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('main', 'branch1', 't');", - Expected: []sql.Row{{1, "t", "schema", "ALTER TABLE `t` DROP `c2`;"}}, - }, - { - Query: "SHOW WARNINGS", - Expected: []sql.Row{{"Warning", 1235, "Incompatible schema change, skipping data diff for table 't'"}}, + Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('main', 'branch1', 't');", + Expected: []sql.Row{ + {1, "t", "schema", "ALTER TABLE `t` DROP `c2`;"}, + {2, "t", "data", "DELETE FROM `t` WHERE `pk`=1;"}, + {3, "t", "data", "UPDATE `t` SET WHERE `pk`=2;"}, + }, }, { - Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('main..branch1', 't');", - Expected: []sql.Row{{1, "t", "schema", "ALTER TABLE `t` DROP `c2`;"}}, + Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('main..branch1', 't');", + Expected: []sql.Row{ + {1, "t", "schema", "ALTER TABLE `t` DROP `c2`;"}, + {2, "t", "data", "DELETE FROM `t` WHERE `pk`=1;"}, + {3, "t", "data", "UPDATE `t` SET WHERE `pk`=2;"}, + }, }, { Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('main', 'branch1');", Expected: []sql.Row{ {1, "newtable", "schema", "DROP TABLE `newtable`;"}, {2, "t", "schema", "ALTER TABLE `t` DROP `c2`;"}, + {3, "t", "data", "DELETE FROM `t` WHERE `pk`=1;"}, + {4, "t", "data", "UPDATE `t` SET WHERE `pk`=2;"}, }, }, { @@ -3263,32 +3269,58 @@ var PatchTableFunctionScriptTests = []queries.ScriptTest{ Expected: []sql.Row{ {1, "newtable", "schema", "DROP TABLE `newtable`;"}, {2, "t", "schema", "ALTER TABLE `t` DROP `c2`;"}, + {3, "t", "data", "DELETE FROM `t` WHERE `pk`=1;"}, + {4, "t", "data", "UPDATE `t` SET WHERE `pk`=2;"}, }, }, { - Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('branch1', 'main', 't');", - Expected: []sql.Row{{1, "t", "schema", "ALTER TABLE `t` ADD `c2` varchar(20);"}}, + Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('branch1', 'main', 't');", + Expected: []sql.Row{ + {1, "t", "schema", "ALTER TABLE `t` ADD `c2` varchar(20);"}, + {2, "t", "data", "INSERT INTO `t` (`pk`,`c1`,`c2`) VALUES (1,'one','two');"}, + {3, "t", "data", "UPDATE `t` SET `c2`='three' WHERE `pk`=2;"}, + }, }, { - Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('branch1..main', 't');", - Expected: []sql.Row{{1, "t", "schema", "ALTER TABLE `t` ADD `c2` varchar(20);"}}, + Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('branch1..main', 't');", + Expected: []sql.Row{ + {1, "t", "schema", "ALTER TABLE `t` ADD `c2` varchar(20);"}, + {2, "t", "data", "INSERT INTO `t` (`pk`,`c1`,`c2`) VALUES (1,'one','two');"}, + {3, "t", "data", "UPDATE `t` SET `c2`='three' WHERE `pk`=2;"}, + }, }, { - Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('main~2', 'branch1', 't');", - Expected: []sql.Row{{1, "t", "schema", "ALTER TABLE `t` DROP `c2`;"}}, + Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('main~2', 'branch1', 't');", + Expected: []sql.Row{ + {1, "t", "schema", "ALTER TABLE `t` DROP `c2`;"}, + {2, "t", "data", "DELETE FROM `t` WHERE `pk`=1;"}, + {3, "t", "data", "INSERT INTO `t` (`pk`,`c1`) VALUES (2,'two');"}, + }, }, { - Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('main~2..branch1', 't');", - Expected: []sql.Row{{1, "t", "schema", "ALTER TABLE `t` DROP `c2`;"}}, + Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('main~2..branch1', 't');", + Expected: []sql.Row{ + {1, "t", "schema", "ALTER TABLE `t` DROP `c2`;"}, + {2, "t", "data", "DELETE FROM `t` WHERE `pk`=1;"}, + {3, "t", "data", "INSERT INTO `t` (`pk`,`c1`) VALUES (2,'two');"}, + }, }, // Three dot { - Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('main...branch1', 't');", - Expected: []sql.Row{{1, "t", "schema", "ALTER TABLE `t` DROP `c2`;"}}, + Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('main...branch1', 't');", + Expected: []sql.Row{ + {1, "t", "schema", "ALTER TABLE `t` DROP `c2`;"}, + {2, "t", "data", "DELETE FROM `t` WHERE `pk`=1;"}, + {3, "t", "data", "INSERT INTO `t` (`pk`,`c1`) VALUES (2,'two');"}, + }, }, { - Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('main...branch1');", - Expected: []sql.Row{{1, "t", "schema", "ALTER TABLE `t` DROP `c2`;"}}, + Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('main...branch1');", + Expected: []sql.Row{ + {1, "t", "schema", "ALTER TABLE `t` DROP `c2`;"}, + {2, "t", "data", "DELETE FROM `t` WHERE `pk`=1;"}, + {3, "t", "data", "INSERT INTO `t` (`pk`,`c1`) VALUES (2,'two');"}, + }, }, { Query: "SELECT statement_order, table_name, diff_type, statement FROM dolt_patch('branch1...main', 't');", @@ -3371,8 +3403,7 @@ var PatchTableFunctionScriptTests = []queries.ScriptTest{ SetUpScript: []string{ "CREATE TABLE parent (id int PRIMARY KEY, id_ext int, v1 int, v2 text COMMENT 'tag:1', INDEX v1 (v1));", "CREATE TABLE child (id int primary key, v1 int);", - "call dolt_add('.')", - "call dolt_commit('-am', 'new tables')", + "call dolt_commit('-Am', 'new tables')", "ALTER TABLE child ADD CONSTRAINT fk_named FOREIGN KEY (v1) REFERENCES parent(v1);", "insert into parent values (0, 1, 2, NULL);", "ALTER TABLE parent DROP PRIMARY KEY;", @@ -3396,12 +3427,11 @@ var PatchTableFunctionScriptTests = []queries.ScriptTest{ {3, "STAGED", "parent", "schema", "ALTER TABLE `parent` DROP PRIMARY KEY;"}, {4, "STAGED", "parent", "schema", "ALTER TABLE `parent` ADD PRIMARY KEY (id,id_ext);"}, }, - ExpectedWarningsCount: 2, + ExpectedWarningsCount: 1, }, { Query: "SHOW WARNINGS;", Expected: []sql.Row{ - {"Warning", 1235, "Incompatible schema change, skipping data diff for table 'child'"}, {"Warning", 1235, "Primary key sets differ between revisions for table 'parent', skipping data diff"}, }, }, @@ -3419,6 +3449,38 @@ var PatchTableFunctionScriptTests = []queries.ScriptTest{ }, }, }, + { + Name: "charset and collation changes", + SetUpScript: []string{ + "create table t (pk int primary key) collate='utf8mb4_0900_bin';", + "call dolt_commit('-Am', 'empty table')", + "set @commit0=hashof('HEAD');", + "insert into t values (1)", + "alter table t collate='utf8mb4_0900_ai_ci';", + "call dolt_commit('-am', 'inserting a row and altering the collation')", + "set @commit1=hashof('HEAD');", + "alter table t CHARACTER SET='utf8mb3';", + "insert into t values (2)", + "call dolt_commit('-am', 'inserting a row and altering the collation')", + "set @commit2=hashof('HEAD');", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "select * from dolt_patch(@commit1, @commit0);", + Expected: []sql.Row{ + {1, doltCommit, doltCommit, "t", "schema", "ALTER TABLE `t` COLLATE='utf8mb4_0900_bin';"}, + {2, doltCommit, doltCommit, "t", "data", "DELETE FROM `t` WHERE `pk`=1;"}, + }, + }, + { + Query: "select * from dolt_patch(@commit1, @commit2);", + Expected: []sql.Row{ + {1, doltCommit, doltCommit, "t", "schema", "ALTER TABLE `t` COLLATE='utf8mb3_general_ci';"}, + {2, doltCommit, doltCommit, "t", "data", "INSERT INTO `t` (`pk`) VALUES (2);"}, + }, + }, + }, + }, { Name: "patch DDL changes", SetUpScript: []string{ @@ -3439,6 +3501,10 @@ var PatchTableFunctionScriptTests = []queries.ScriptTest{ Expected: []sql.Row{ {"ALTER TABLE `t` DROP `b`;"}, {"ALTER TABLE `t` ADD `d` int;"}, + {"UPDATE `t` SET WHERE `pk`=1;"}, + {"UPDATE `t` SET WHERE `pk`=2;"}, + {"DELETE FROM `t` WHERE `pk`=3;"}, + {"INSERT INTO `t` (`pk`,`a`,`c`,`d`) VALUES (7,7,7,7);"}, }, }, }, diff --git a/go/libraries/doltcore/sqle/sqlfmt/schema_fmt.go b/go/libraries/doltcore/sqle/sqlfmt/schema_fmt.go index 73f92e0a3d..99a0105a46 100644 --- a/go/libraries/doltcore/sqle/sqlfmt/schema_fmt.go +++ b/go/libraries/doltcore/sqle/sqlfmt/schema_fmt.go @@ -197,6 +197,15 @@ func AlterTableDropIndexStmt(tableName string, idx schema.Index) string { return b.String() } +func AlterTableCollateStmt(tableName string, fromCollation, toCollation schema.Collation) string { + var b strings.Builder + b.WriteString("ALTER TABLE ") + b.WriteString(QuoteIdentifier(tableName)) + toCollationId := sql.CollationID(toCollation) + b.WriteString(" COLLATE=" + QuoteComment(toCollationId.Name()) + ";") + return b.String() +} + func AlterTableAddForeignKeyStmt(fk doltdb.ForeignKey, sch, parentSch schema.Schema) string { var b strings.Builder b.WriteString("ALTER TABLE ")