From 0d815b245ce585726de5d26747f603960f75dbc4 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 23 Apr 2023 13:23:26 +0300 Subject: [PATCH 1/3] naming: Dep->Dependency Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/schema.go | 10 +-- go/vt/schemadiff/schema_diff.go | 92 ++++++++++++++-------------- go/vt/schemadiff/schema_diff_test.go | 4 +- 3 files changed, 53 insertions(+), 53 deletions(-) diff --git a/go/vt/schemadiff/schema.go b/go/vt/schemadiff/schema.go index 84946768c61..a9ef60fbb27 100644 --- a/go/vt/schemadiff/schema.go +++ b/go/vt/schemadiff/schema.go @@ -789,7 +789,7 @@ func (s *Schema) SchemaDiff(other *Schema, hints *DiffHints) (*SchemaDiff, error // 'diff' refers to an entity (call it "e") that has changed. But here we find that one of the // entities that "e" depends on, has also changed. relationsMade = true - schemaDiff.addDep(diff, dependentDiff, DiffDepOrderUnknown) + schemaDiff.addDep(diff, dependentDiff, DiffDependencyOrderUnknown) } } return dependentDiffs, relationsMade @@ -827,7 +827,7 @@ func (s *Schema) SchemaDiff(other *Schema, hints *DiffHints) (*SchemaDiff, error case *CreateTableEntityDiff: // We add a foreign key constraint onto a new table... That table must therefore be first created, // and only then can we proceed to add the FK - schemaDiff.addDep(diff, parentDiff, DiffDepSequentialExecution) + schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) case *AlterTableEntityDiff: // The current diff is ALTER TABLE ... ADD FOREIGN KEY // and the parent table also has an ALTER TABLE. @@ -842,17 +842,17 @@ func (s *Schema) SchemaDiff(other *Schema, hints *DiffHints) (*SchemaDiff, error switch node := node.(type) { case *sqlparser.ModifyColumn: if referencedColumnNames[node.NewColDefinition.Name.Lowered()] { - schemaDiff.addDep(diff, parentDiff, DiffDepSequentialExecution) + schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) } case *sqlparser.AddColumns: for _, col := range node.Columns { if referencedColumnNames[col.Name.Lowered()] { - schemaDiff.addDep(diff, parentDiff, DiffDepSequentialExecution) + schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) } } case *sqlparser.DropColumn: if referencedColumnNames[node.Name.Name.Lowered()] { - schemaDiff.addDep(diff, parentDiff, DiffDepSequentialExecution) + schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) } } return true, nil diff --git a/go/vt/schemadiff/schema_diff.go b/go/vt/schemadiff/schema_diff.go index 9b3d1f11630..aa8cd1061d4 100644 --- a/go/vt/schemadiff/schema_diff.go +++ b/go/vt/schemadiff/schema_diff.go @@ -22,34 +22,34 @@ import ( "vitess.io/vitess/go/mathutil" ) -type DiffDepType int +type DiffDependencyType int // diff dependencies in increasing restriction severity const ( - DiffDepNone DiffDepType = iota // not a dependency - DiffDepOrderUnknown - DiffDepInOrderCompletion - DiffDepSequentialExecution + DiffDependencyNone DiffDependencyType = iota // not a dependency + DiffDependencyOrderUnknown + DiffDependencyInOrderCompletion + DiffDependencySequentialExecution ) -// DiffDep indicates a dependency between two diffs, and the type of that dependency -type DiffDep struct { - diff EntityDiff - depDiff EntityDiff // depends on the above diff - depType DiffDepType +// DiffDependency indicates a dependency between two diffs, and the type of that dependency +type DiffDependency struct { + Diff EntityDiff + DependentDiff EntityDiff // depends on the above diff + Type DiffDependencyType } -// NewDiffDep returns a new diff dependency pairing. -func NewDiffDep(diff EntityDiff, depDiff EntityDiff, depType DiffDepType) *DiffDep { - return &DiffDep{ - diff: diff, - depDiff: depDiff, - depType: depType, +// NewDiffDependency returns a new diff dependency pairing. +func NewDiffDependency(diff EntityDiff, dependentDiff EntityDiff, depType DiffDependencyType) *DiffDependency { + return &DiffDependency{ + Diff: diff, + DependentDiff: dependentDiff, + Type: depType, } } -func (d *DiffDep) hashKey() string { - return d.diff.CanonicalStatementString() + "/" + d.depDiff.CanonicalStatementString() +func (d *DiffDependency) hashKey() string { + return d.Diff.CanonicalStatementString() + "/" + d.DependentDiff.CanonicalStatementString() } /* @@ -96,18 +96,18 @@ type SchemaDiff struct { schema *Schema diffs []EntityDiff - diffMap map[string]EntityDiff // key is diff's CanonicalStatementString() - deps map[string]*DiffDep + diffMap map[string]EntityDiff // key is diff's CanonicalStatementString() + dependencies map[string]*DiffDependency r *mathutil.EquivalenceRelation // internal structure to help determine diffs } func NewSchemaDiff(schema *Schema) *SchemaDiff { return &SchemaDiff{ - schema: schema, - deps: make(map[string]*DiffDep), - diffMap: make(map[string]EntityDiff), - r: mathutil.NewEquivalenceRelation(), + schema: schema, + dependencies: make(map[string]*DiffDependency), + diffMap: make(map[string]EntityDiff), + r: mathutil.NewEquivalenceRelation(), } } @@ -123,7 +123,7 @@ func (d *SchemaDiff) loadDiffs(diffs []EntityDiff) { if i > 0 { // So this is a 2nd, 3rd etc. diff operating on same table // Two migrations on same entity (table in our case) must run sequentially. - d.addDep(sdiff, allSubsequent[0], DiffDepSequentialExecution) + d.addDep(sdiff, allSubsequent[0], DiffDependencySequentialExecution) } d.r.Add(sdiff.CanonicalStatementString()) // since we've exploded the subsequent diffs, we now clear any subsequent diffs @@ -133,20 +133,20 @@ func (d *SchemaDiff) loadDiffs(diffs []EntityDiff) { } } -// addDep adds a dependency: `depDiff` depends on `diff`, with given `depType`. If there's an +// addDep adds a dependency: `dependentDiff` depends on `diff`, with given `depType`. If there's an // already existing dependency between the two diffs, then we compare the dependency type; if the new // type has a higher order (ie stricter) then we replace the existing dependency with the new one. -func (d *SchemaDiff) addDep(diff EntityDiff, depDiff EntityDiff, depType DiffDepType) *DiffDep { - _, _ = d.r.Relate(diff.CanonicalStatementString(), depDiff.CanonicalStatementString()) - diffDep := NewDiffDep(diff, depDiff, depType) - if existingDep, ok := d.deps[diffDep.hashKey()]; ok { - if existingDep.depType >= diffDep.depType { +func (d *SchemaDiff) addDep(diff EntityDiff, dependentDiff EntityDiff, depType DiffDependencyType) *DiffDependency { + _, _ = d.r.Relate(diff.CanonicalStatementString(), dependentDiff.CanonicalStatementString()) + diffDep := NewDiffDependency(diff, dependentDiff, depType) + if existingDep, ok := d.dependencies[diffDep.hashKey()]; ok { + if existingDep.Type >= diffDep.Type { // nothing new here, the new dependency is weaker or equals to an existing dependency return existingDep } } // Either the dep wasn't found, or we've just introduced a dep with a more severe type - d.deps[diffDep.hashKey()] = diffDep + d.dependencies[diffDep.hashKey()] = diffDep return diffDep } @@ -179,35 +179,35 @@ func (d *SchemaDiff) UnorderedDiffs() []EntityDiff { return d.diffs } -// AllDeps returns all known dependencies -func (d *SchemaDiff) AllDeps() (deps []*DiffDep) { - for _, dep := range d.deps { +// AllDependenciess returns all known dependencies +func (d *SchemaDiff) AllDependenciess() (deps []*DiffDependency) { + for _, dep := range d.dependencies { deps = append(deps, dep) } return deps } -// HasDeps returns `true` if there is at least one known diff dependency. +// HasDependencies returns `true` if there is at least one known diff dependency. // If this function returns `false` then that means there is no restriction whatsoever to the order of diffs. -func (d *SchemaDiff) HasDeps() bool { - return len(d.deps) > 0 +func (d *SchemaDiff) HasDependencies() bool { + return len(d.dependencies) > 0 } -// AllSequentialExecutionDeps returns all diffs that are of "sequential execution" type. -func (d *SchemaDiff) AllSequentialExecutionDeps() (deps []*DiffDep) { - for _, dep := range d.deps { - if dep.depType >= DiffDepSequentialExecution { +// AllSequentialExecutionDependencies returns all diffs that are of "sequential execution" type. +func (d *SchemaDiff) AllSequentialExecutionDependencies() (deps []*DiffDependency) { + for _, dep := range d.dependencies { + if dep.Type >= DiffDependencySequentialExecution { deps = append(deps, dep) } } return deps } -// HasSequentialExecutionDeps return `true` if there is at least one "subsequential execution" type diff. +// HasSequentialExecutionDependencies return `true` if there is at least one "subsequential execution" type diff. // If not, that means all diffs can be applied in parallel. -func (d *SchemaDiff) HasSequentialExecutionDeps() bool { - for _, dep := range d.deps { - if dep.depType >= DiffDepSequentialExecution { +func (d *SchemaDiff) HasSequentialExecutionDependencies() bool { + for _, dep := range d.dependencies { + if dep.Type >= DiffDependencySequentialExecution { return true } } diff --git a/go/vt/schemadiff/schema_diff_test.go b/go/vt/schemadiff/schema_diff_test.go index b5d8734da91..670e84c6f1a 100644 --- a/go/vt/schemadiff/schema_diff_test.go +++ b/go/vt/schemadiff/schema_diff_test.go @@ -674,13 +674,13 @@ func TestSchemaDiff(t *testing.T) { } assert.Equalf(t, tc.expectDiffs, len(allDiffs), "found diffs: %v", allDiffsStatements) - deps := schemaDiff.AllDeps() + deps := schemaDiff.AllDependenciess() depsKeys := []string{} for _, dep := range deps { depsKeys = append(depsKeys, dep.hashKey()) } assert.Equalf(t, tc.expectDeps, len(deps), "found deps: %v", depsKeys) - assert.Equal(t, tc.sequential, schemaDiff.HasSequentialExecutionDeps()) + assert.Equal(t, tc.sequential, schemaDiff.HasSequentialExecutionDependencies()) orderedDiffs, err := schemaDiff.OrderedDiffs() if tc.conflictingDiffs > 0 { From 8e98bce31629958c778ce8c8e7bc337e9da9c27f Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 23 Apr 2023 13:25:52 +0300 Subject: [PATCH 2/3] DiffSchemas returns a rich SchemaDiff object Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/diff.go | 18 +++++------------- go/vt/schemadiff/diff_test.go | 4 +++- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/go/vt/schemadiff/diff.go b/go/vt/schemadiff/diff.go index 5225471ad0b..354a3473888 100644 --- a/go/vt/schemadiff/diff.go +++ b/go/vt/schemadiff/diff.go @@ -148,10 +148,10 @@ func DiffViews(create1 *sqlparser.CreateView, create2 *sqlparser.CreateView, hin } } -// DiffSchemasSQL compares two schemas and returns the list of diffs that turn +// DiffSchemasSQL compares two schemas and returns the rich diff that turns // 1st schema into 2nd. Schemas are build from SQL, each of which can contain an arbitrary number of // CREATE TABLE and CREATE VIEW statements. -func DiffSchemasSQL(sql1 string, sql2 string, hints *DiffHints) ([]EntityDiff, error) { +func DiffSchemasSQL(sql1 string, sql2 string, hints *DiffHints) (*SchemaDiff, error) { schema1, err := NewSchemaFromSQL(sql1) if err != nil { return nil, err @@ -160,25 +160,17 @@ func DiffSchemasSQL(sql1 string, sql2 string, hints *DiffHints) ([]EntityDiff, e if err != nil { return nil, err } - diff, err := schema1.SchemaDiff(schema2, hints) - if err != nil { - return nil, err - } - return diff.OrderedDiffs() + return schema1.SchemaDiff(schema2, hints) } // DiffSchemasSQL compares two schemas and returns the list of diffs that turn // 1st schema into 2nd. Any of the schemas may be nil. -func DiffSchemas(schema1 *Schema, schema2 *Schema, hints *DiffHints) ([]EntityDiff, error) { +func DiffSchemas(schema1 *Schema, schema2 *Schema, hints *DiffHints) (*SchemaDiff, error) { if schema1 == nil { schema1 = newEmptySchema() } if schema2 == nil { schema2 = newEmptySchema() } - diff, err := schema1.SchemaDiff(schema2, hints) - if err != nil { - return nil, err - } - return diff.OrderedDiffs() + return schema1.SchemaDiff(schema2, hints) } diff --git a/go/vt/schemadiff/diff_test.go b/go/vt/schemadiff/diff_test.go index 5c3398381eb..c2ac15fece5 100644 --- a/go/vt/schemadiff/diff_test.go +++ b/go/vt/schemadiff/diff_test.go @@ -770,13 +770,15 @@ func TestDiffSchemas(t *testing.T) { hints := &DiffHints{ TableRenameStrategy: ts.tableRename, } - diffs, err := DiffSchemasSQL(ts.from, ts.to, hints) + diff, err := DiffSchemasSQL(ts.from, ts.to, hints) if ts.expectError != "" { require.Error(t, err) assert.Contains(t, err.Error(), ts.expectError) } else { assert.NoError(t, err) + diffs, err := diff.OrderedDiffs() + assert.NoError(t, err) statements := []string{} cstatements := []string{} for _, d := range diffs { From e5515ca010a2cab094e243d8732d151be1cfe1be Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 23 Apr 2023 13:31:41 +0300 Subject: [PATCH 3/3] Access functions to DiffDependency Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/schema_diff.go | 42 +++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/go/vt/schemadiff/schema_diff.go b/go/vt/schemadiff/schema_diff.go index aa8cd1061d4..b6c539aea95 100644 --- a/go/vt/schemadiff/schema_diff.go +++ b/go/vt/schemadiff/schema_diff.go @@ -34,22 +34,38 @@ const ( // DiffDependency indicates a dependency between two diffs, and the type of that dependency type DiffDependency struct { - Diff EntityDiff - DependentDiff EntityDiff // depends on the above diff - Type DiffDependencyType + diff EntityDiff + dependentDiff EntityDiff // depends on the above diff + typ DiffDependencyType } // NewDiffDependency returns a new diff dependency pairing. -func NewDiffDependency(diff EntityDiff, dependentDiff EntityDiff, depType DiffDependencyType) *DiffDependency { +func NewDiffDependency(diff EntityDiff, dependentDiff EntityDiff, typ DiffDependencyType) *DiffDependency { return &DiffDependency{ - Diff: diff, - DependentDiff: dependentDiff, - Type: depType, + diff: diff, + dependentDiff: dependentDiff, + typ: typ, } } func (d *DiffDependency) hashKey() string { - return d.Diff.CanonicalStatementString() + "/" + d.DependentDiff.CanonicalStatementString() + return d.diff.CanonicalStatementString() + "/" + d.dependentDiff.CanonicalStatementString() +} + +// Diff returns the "benefactor" diff, on which DependentDiff() depends on, ie, should run 1st. +func (d *DiffDependency) Diff() EntityDiff { + return d.diff +} + +// DependentDiff returns the diff that depends on the "benefactor" diff, ie must run 2nd +func (d *DiffDependency) DependentDiff() EntityDiff { + return d.dependentDiff +} + +// Type returns the dependency type. Types are numeric and comparable: the higher the value, the +// stricter, or more constrained, the dependency is. +func (d *DiffDependency) Type() DiffDependencyType { + return d.typ } /* @@ -136,11 +152,11 @@ func (d *SchemaDiff) loadDiffs(diffs []EntityDiff) { // addDep adds a dependency: `dependentDiff` depends on `diff`, with given `depType`. If there's an // already existing dependency between the two diffs, then we compare the dependency type; if the new // type has a higher order (ie stricter) then we replace the existing dependency with the new one. -func (d *SchemaDiff) addDep(diff EntityDiff, dependentDiff EntityDiff, depType DiffDependencyType) *DiffDependency { +func (d *SchemaDiff) addDep(diff EntityDiff, dependentDiff EntityDiff, typ DiffDependencyType) *DiffDependency { _, _ = d.r.Relate(diff.CanonicalStatementString(), dependentDiff.CanonicalStatementString()) - diffDep := NewDiffDependency(diff, dependentDiff, depType) + diffDep := NewDiffDependency(diff, dependentDiff, typ) if existingDep, ok := d.dependencies[diffDep.hashKey()]; ok { - if existingDep.Type >= diffDep.Type { + if existingDep.typ >= diffDep.typ { // nothing new here, the new dependency is weaker or equals to an existing dependency return existingDep } @@ -196,7 +212,7 @@ func (d *SchemaDiff) HasDependencies() bool { // AllSequentialExecutionDependencies returns all diffs that are of "sequential execution" type. func (d *SchemaDiff) AllSequentialExecutionDependencies() (deps []*DiffDependency) { for _, dep := range d.dependencies { - if dep.Type >= DiffDependencySequentialExecution { + if dep.typ >= DiffDependencySequentialExecution { deps = append(deps, dep) } } @@ -207,7 +223,7 @@ func (d *SchemaDiff) AllSequentialExecutionDependencies() (deps []*DiffDependenc // If not, that means all diffs can be applied in parallel. func (d *SchemaDiff) HasSequentialExecutionDependencies() bool { for _, dep := range d.dependencies { - if dep.Type >= DiffDependencySequentialExecution { + if dep.typ >= DiffDependencySequentialExecution { return true } }