From 1c39c52afe364c2a7426973916eddd93283b5e2e Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Wed, 8 May 2024 11:06:47 +0300 Subject: [PATCH] `schemadiff`: atomic diffs for range partition `DROP PARTITION` statement (#15843) Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/diff.go | 30 +++++++++++++++ go/vt/schemadiff/diff_test.go | 21 +++++++++++ go/vt/schemadiff/table.go | 68 ++++++++++++++++++++++++++++++++++ go/vt/schemadiff/table_test.go | 26 +++++++++++++ go/vt/schemadiff/types.go | 3 +- go/vt/schemadiff/view.go | 41 ++++++++++++++++++++ go/vt/schemadiff/view_test.go | 9 +++++ 7 files changed, 197 insertions(+), 1 deletion(-) diff --git a/go/vt/schemadiff/diff.go b/go/vt/schemadiff/diff.go index 88eb6a7b326..ff0d861516b 100644 --- a/go/vt/schemadiff/diff.go +++ b/go/vt/schemadiff/diff.go @@ -24,6 +24,36 @@ func AllSubsequent(diff EntityDiff) (diffs []EntityDiff) { return diffs } +// AtomicDiffs attempts to break a given diff statement into its smallest components. +// This isn't necessarily the _correct_ thing to do, as MySQL goes, but it assists in +// identifying the distinct changes that are being made. +// Currently, the only implementation is to break up `ALTER TABLE ... DROP PARTITION` statements. +func AtomicDiffs(diff EntityDiff) []EntityDiff { + if diff == nil || diff.IsEmpty() { + return nil + } + trivial := func() []EntityDiff { + return []EntityDiff{diff} + } + switch diff := diff.(type) { + case *AlterTableEntityDiff: + alterTable := diff.alterTable + // Examine the scenario where we have e.g. `ALTER TABLE ... DROP PARTITION p1, p2, p3` + // and explode it into separate diffs + if alterTable.PartitionSpec != nil && alterTable.PartitionSpec.Action == sqlparser.DropAction && len(alterTable.PartitionSpec.Names) > 1 { + var distinctDiffs []EntityDiff + for i := range alterTable.PartitionSpec.Names { + clone := diff.Clone() + cloneAlterTableEntityDiff := clone.(*AlterTableEntityDiff) + cloneAlterTableEntityDiff.alterTable.PartitionSpec.Names = cloneAlterTableEntityDiff.alterTable.PartitionSpec.Names[i : i+1] + distinctDiffs = append(distinctDiffs, clone) + } + return distinctDiffs + } + } + return trivial() +} + // DiffCreateTablesQueries compares two `CREATE TABLE ...` queries (in string form) and returns the diff from table1 to table2. // Either or both of the queries can be empty. Based on this, the diff could be // nil, CreateTable, DropTable or AlterTable diff --git a/go/vt/schemadiff/diff_test.go b/go/vt/schemadiff/diff_test.go index 1f4861087a5..d78308c90e0 100644 --- a/go/vt/schemadiff/diff_test.go +++ b/go/vt/schemadiff/diff_test.go @@ -569,6 +569,11 @@ func TestDiffViews(t *testing.T) { diff := dq.StatementString() assert.Equal(t, ts.diff, diff) } + if d != nil && !d.IsEmpty() { + // Validate Clone() works + clone := d.Clone() + assert.Equal(t, d.Statement(), clone.Statement()) + } }) } } @@ -1061,6 +1066,22 @@ func TestDiffSchemas(t *testing.T) { _, err := env.Parser().ParseStrictDDL(s) assert.NoError(t, err) } + // Validate Clone() works + for _, d := range diffs { + if d.IsEmpty() { + continue + } + dFrom, dTo := d.Entities() + clone := d.Clone() + clonedFrom, clonedTo := clone.Entities() + assert.Equal(t, d.CanonicalStatementString(), clone.CanonicalStatementString()) + if dFrom != nil { + assert.Equal(t, dFrom.Name(), clonedFrom.Name()) + } + if dTo != nil { + assert.Equal(t, dTo.Name(), clonedTo.Name()) + } + } if ts.annotated != nil { // Optional test for assorted scenarios. diff --git a/go/vt/schemadiff/table.go b/go/vt/schemadiff/table.go index f8ecfd6081e..def83fa7f19 100644 --- a/go/vt/schemadiff/table.go +++ b/go/vt/schemadiff/table.go @@ -139,6 +139,29 @@ func (d *AlterTableEntityDiff) InstantDDLCapability() InstantDDLCapability { return d.instantDDLCapability } +// Clone implements EntityDiff +func (d *AlterTableEntityDiff) Clone() EntityDiff { + if d == nil { + return nil + } + ann := *d.annotations + clone := &AlterTableEntityDiff{ + alterTable: sqlparser.CloneRefOfAlterTable(d.alterTable), + instantDDLCapability: d.instantDDLCapability, + annotations: &ann, + } + if d.from != nil { + clone.from = d.from.Clone().(*CreateTableEntity) + } + if d.to != nil { + clone.to = d.to.Clone().(*CreateTableEntity) + } + if d.subsequentDiff != nil { + clone.subsequentDiff = d.subsequentDiff.Clone().(*AlterTableEntityDiff) + } + return clone +} + type CreateTableEntityDiff struct { to *CreateTableEntity createTable *sqlparser.CreateTable @@ -216,6 +239,20 @@ func (d *CreateTableEntityDiff) InstantDDLCapability() InstantDDLCapability { return InstantDDLCapabilityIrrelevant } +// Clone implements EntityDiff +func (d *CreateTableEntityDiff) Clone() EntityDiff { + if d == nil { + return nil + } + clone := &CreateTableEntityDiff{ + createTable: sqlparser.CloneRefOfCreateTable(d.createTable), + } + if d.to != nil { + clone.to = d.to.Clone().(*CreateTableEntity) + } + return clone +} + type DropTableEntityDiff struct { from *CreateTableEntity dropTable *sqlparser.DropTable @@ -293,6 +330,20 @@ func (d *DropTableEntityDiff) InstantDDLCapability() InstantDDLCapability { return InstantDDLCapabilityIrrelevant } +// Clone implements EntityDiff +func (d *DropTableEntityDiff) Clone() EntityDiff { + if d == nil { + return nil + } + clone := &DropTableEntityDiff{ + dropTable: sqlparser.CloneRefOfDropTable(d.dropTable), + } + if d.from != nil { + clone.from = d.from.Clone().(*CreateTableEntity) + } + return clone +} + type RenameTableEntityDiff struct { from *CreateTableEntity to *CreateTableEntity @@ -371,6 +422,23 @@ func (d *RenameTableEntityDiff) InstantDDLCapability() InstantDDLCapability { return InstantDDLCapabilityIrrelevant } +// Clone implements EntityDiff +func (d *RenameTableEntityDiff) Clone() EntityDiff { + if d == nil { + return nil + } + clone := &RenameTableEntityDiff{ + renameTable: sqlparser.CloneRefOfRenameTable(d.renameTable), + } + if d.from != nil { + clone.from = d.from.Clone().(*CreateTableEntity) + } + if d.to != nil { + clone.to = d.to.Clone().(*CreateTableEntity) + } + return clone +} + // CreateTableEntity stands for a TABLE construct. It contains the table's CREATE statement. type CreateTableEntity struct { *sqlparser.CreateTable diff --git a/go/vt/schemadiff/table_test.go b/go/vt/schemadiff/table_test.go index cb734245876..09997057e16 100644 --- a/go/vt/schemadiff/table_test.go +++ b/go/vt/schemadiff/table_test.go @@ -48,6 +48,7 @@ func TestCreateTableDiff(t *testing.T) { enumreorder int subsequent int textdiffs []string + atomicdiffs []string }{ { name: "identical", @@ -1365,6 +1366,10 @@ func TestCreateTableDiff(t *testing.T) { "-(PARTITION `p1` VALUES LESS THAN (10),", "- PARTITION `p2` VALUES LESS THAN (20),", }, + atomicdiffs: []string{ + "ALTER TABLE `t1` DROP PARTITION `p1`", + "ALTER TABLE `t1` DROP PARTITION `p2`", + }, }, { name: "change partitioning range: statements, multiple adds", @@ -1503,6 +1508,10 @@ func TestCreateTableDiff(t *testing.T) { "+ PARTITION `p3` VALUES LESS THAN (35),", "+ PARTITION `p4` VALUES LESS THAN (40))", }, + atomicdiffs: []string{ + "ALTER TABLE `t1` DROP PARTITION `p1`", + "ALTER TABLE `t1` DROP PARTITION `p3`", + }, }, { name: "change partitioning range: complex rotate 2, ignore", @@ -1523,6 +1532,10 @@ func TestCreateTableDiff(t *testing.T) { "+ PARTITION `pX` VALUES LESS THAN (30),", "+ PARTITION `p4` VALUES LESS THAN (40))", }, + atomicdiffs: []string{ + "ALTER TABLE `t1` DROP PARTITION `p1`", + "ALTER TABLE `t1` DROP PARTITION `p3`", + }, }, { name: "change partitioning range: not a rotation", @@ -2077,6 +2090,7 @@ func TestCreateTableDiff(t *testing.T) { t.Logf("other: %v", sqlparser.CanonicalString(other.CreateTable)) } assert.Empty(t, ts.textdiffs) + assert.Empty(t, AtomicDiffs(alter)) return } @@ -2130,6 +2144,18 @@ func TestCreateTableDiff(t *testing.T) { applied.Create().CanonicalStatementString(), ) } + // Validate atomic diffs + atomicDiffs := AtomicDiffs(alter) + if len(ts.atomicdiffs) > 0 { + assert.Equal(t, len(ts.atomicdiffs), len(atomicDiffs), "%+v", atomicDiffs) + for i := range ts.atomicdiffs { + assert.Equal(t, ts.atomicdiffs[i], atomicDiffs[i].CanonicalStatementString()) + } + } else { + assert.Equal(t, 1, len(atomicDiffs)) + assert.Equal(t, alter.CanonicalStatementString(), atomicDiffs[0].CanonicalStatementString()) + } + { // Validate annotations alterEntityDiff, ok := alter.(*AlterTableEntityDiff) require.True(t, ok) diff --git a/go/vt/schemadiff/types.go b/go/vt/schemadiff/types.go index 16cc15eb746..2049387140c 100644 --- a/go/vt/schemadiff/types.go +++ b/go/vt/schemadiff/types.go @@ -69,7 +69,8 @@ type EntityDiff interface { SetSubsequentDiff(EntityDiff) // InstantDDLCapability returns the ability of this diff to run with ALGORITHM=INSTANT InstantDDLCapability() InstantDDLCapability - + // Clone returns a deep copy of the entity diff, and of all referenced entities. + Clone() EntityDiff Annotated() (from *TextualAnnotations, to *TextualAnnotations, unified *TextualAnnotations) } diff --git a/go/vt/schemadiff/view.go b/go/vt/schemadiff/view.go index 9566979a529..c4d48ac66cc 100644 --- a/go/vt/schemadiff/view.go +++ b/go/vt/schemadiff/view.go @@ -100,6 +100,23 @@ func (d *AlterViewEntityDiff) InstantDDLCapability() InstantDDLCapability { return InstantDDLCapabilityIrrelevant } +// Clone implements EntityDiff +func (d *AlterViewEntityDiff) Clone() EntityDiff { + if d == nil { + return nil + } + clone := &AlterViewEntityDiff{ + alterView: sqlparser.CloneRefOfAlterView(d.alterView), + } + if d.from != nil { + clone.from = d.from.Clone().(*CreateViewEntity) + } + if d.to != nil { + clone.to = d.to.Clone().(*CreateViewEntity) + } + return clone +} + type CreateViewEntityDiff struct { createView *sqlparser.CreateView @@ -177,6 +194,16 @@ func (d *CreateViewEntityDiff) InstantDDLCapability() InstantDDLCapability { return InstantDDLCapabilityIrrelevant } +// Clone implements EntityDiff +func (d *CreateViewEntityDiff) Clone() EntityDiff { + if d == nil { + return nil + } + return &CreateViewEntityDiff{ + createView: sqlparser.CloneRefOfCreateView(d.createView), + } +} + type DropViewEntityDiff struct { from *CreateViewEntity dropView *sqlparser.DropView @@ -254,6 +281,20 @@ func (d *DropViewEntityDiff) InstantDDLCapability() InstantDDLCapability { return InstantDDLCapabilityIrrelevant } +// Clone implements EntityDiff +func (d *DropViewEntityDiff) Clone() EntityDiff { + if d == nil { + return nil + } + clone := &DropViewEntityDiff{ + dropView: sqlparser.CloneRefOfDropView(d.dropView), + } + if d.from != nil { + clone.from = d.from.Clone().(*CreateViewEntity) + } + return clone +} + // CreateViewEntity stands for a VIEW construct. It contains the view's CREATE statement. type CreateViewEntity struct { *sqlparser.CreateView diff --git a/go/vt/schemadiff/view_test.go b/go/vt/schemadiff/view_test.go index 2aade1dc3e8..d1a26c3cdaa 100644 --- a/go/vt/schemadiff/view_test.go +++ b/go/vt/schemadiff/view_test.go @@ -196,6 +196,15 @@ func TestCreateViewDiff(t *testing.T) { require.NoError(t, err) assert.True(t, appliedDiff.IsEmpty(), "expected empty diff, found changes: %v", appliedDiff.CanonicalStatementString()) } + // Validate Clone() works + { + clone := alter.Clone() + alterClone, ok := clone.(*AlterViewEntityDiff) + require.True(t, ok) + assert.Equal(t, eFrom.Create().CanonicalStatementString(), alterClone.from.Create().CanonicalStatementString()) + alterClone.from.CreateView.ViewName.Name = sqlparser.NewIdentifierCS("something_else") + assert.NotEqual(t, eFrom.Create().CanonicalStatementString(), alterClone.from.Create().CanonicalStatementString()) + } } { cdiff := alter.CanonicalStatementString()