schemadiff: logically Apply() a diff to a CreateTable, CreateView or to a Schema #10183

shlomi-noach · 2022-05-01T11:36:37Z

Description

In this PR schemadiff can Apply a change onto a declarative table, view or schema, via these new public functions:

func (c *CreateTableEntity) Apply(diff EntityDiff) (Entity, error)
func (c *CreateViewEntity) Apply(diff EntityDiff) (Entity, error)
func (s *Schema) Apply(diffs []EntityDiff) (*Schema, error)

as can be seen in the above, the things we can apply are:

An ALTER TABLE onto a CREATE TABLE entity
An ALTER VIEW onto a CREATE VIEW entity
A list of diffs on a Schema object. These diffs are CREATE/DROP/ALTER TABLE/SCHEMA statements

In each of the above functions:

we return an error if the operation is invalid (e.g. dropping a column when the column does not exist; adding a column when it already exists; etc)
we do not modify the receiver; instead, is successful, a new object is returns
we only support diffs that are generated by Diff()

So the idea is to be able to run purely-declarative schema changes, including ALTER TABLE statements.

The tests are trivially integrated into pre-existing tests: where we already validate a diff between object1 and object2, now attempt to apply the diff to object1 and expect the result to be identical to object2.

Related Issue(s)

This PR extends #10164 as it relies on some of its functionality

Checklist

"Backport me!" label has been added if this change should be backported
Tests were added or are not required
Documentation was added or is not required

cc @dbussink

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2022-05-01T12:44:35Z

Working on final test failures. Moving to draft in the meantime.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2022-05-01T13:26:26Z

OK all good. I accidentally made a regression and pushed it before testing. Now fixed.

dbussink · 2022-05-01T18:34:08Z

go/vt/schemadiff/schema.go

@@ -134,6 +135,8 @@ func getViewDependentTableNames(createView *sqlparser.CreateView) (names []strin
 // normalize is called as part of Schema creation process. The user may only get a hold of normalized schema.
 // It validates some cross-entity constraints, and orders entity based on dependencies (e.g. tables, views that read from tables, 2nd level views, etc.)
 func (s *Schema) normalize() error {
+	s.named = map[string]Entity{}
+	s.sorted = []Entity{}


Are these added for a specific reason?

They are. At the end of apply() we call normalize, and we need to make sure that existing named values are erased. sorted happens to be unpopulated; but normalize really expects both to be empty.
We could clear both in apply, as right now this is the only path where the values get populated before normalize. But I feel normalize is the safer place to do that.

dbussink · 2022-05-01T18:38:06Z

go/vt/schemadiff/schema.go

+					if !ok {
+						return ErrEntityTypeMismatch
+					}
+					s.tables[i] = toCreateTableEntity


Does s.named need to be updated here?

It does not, because it is implied that the view name already exists in named, and an update would just re-apply same name.

@shlomi-noach I'm probably missing something, but doesn't then calling for example Entity(name) return the old entity without the change applied? Or is this updated then somewhere else?

The reason I was asking is because this does get updated on create / drop so that's what caught my eye.

@dbussink gotcha. named is only used in this function to determine existence/nonexistence of an entity. At the end of this function, we call normalize() which completely rebuilds named. We could update the entity value here, but it then gets deleted and recreated in normalize anyhow.

@shlomi-noach Ah ok, if that's the case I think there's an inconsistency though in the add / drop logic.

When a create is done, we don't add it to named so if the diffs slice has multiple creates for the same table, it would not error but have a last one wins.

On the other hand, if diffs contains multiple drops for the same table, it would error on the second one since it does delete from named.

I have no super strong opinion on what the behavior should be, but I think it should be consistent between create and drop?

You're right, there is an inconsistency. Fixing.

Please see updates to Schema.apply() and consistent use of s.named

dbussink · 2022-05-01T18:38:36Z

go/vt/schemadiff/schema.go

+					if !ok {
+						return ErrEntityTypeMismatch
+					}
+					s.views[i] = toCreateViewEntity


Same here, does s.named need to be updated?

same as above, there is no need.

dbussink · 2022-05-01T18:39:33Z

go/vt/schemadiff/table.go

@@ -275,6 +277,46 @@ func (c *CreateTableEntity) diffTableCharset(
 	return ""
 }

+// isDefaultTableOptionValue sees if the value for a TableOption is also its default value
+func isDefaultTableOptionValue(option *sqlparser.TableOption) bool {


Not for this PR, but should there be something similar for what the defaults are for column level options?

for our discussion later on.

dbussink · 2022-05-01T18:40:47Z

go/vt/schemadiff/table.go

+				}
+			}
+			if !afterColFound {
+				return errors.Wrapf(ErrApplyColumnNotFound, "%v", after.Name.String())


Can this be simplified to errors.Wrap(ErrApplyColumnNotFound, after.Name.String())? And for all the other further cases down in this file?

dbussink · 2022-05-01T18:41:03Z

go/vt/schemadiff/table.go

+				return errors.Wrapf(ErrApplyColumnNotFound, "%v", after.Name.String())
+			}
+		default:
+			// no change in positiion


Double ii

dbussink · 2022-05-01T18:41:34Z

go/vt/schemadiff/table.go

+			// Remove partitioning
+			c.TableSpec.PartitionOption = nil
+		default:
+			return errors.Wrapf(ErrUnsupportedApplyOperation, "%v", sqlparser.String(spec))


Can this be errors.Wrap(ErrUnsupportedApplyOperation, sqlparser.String(spec))?

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2022-05-03T12:48:08Z

Tracking issue: #10203

shlomi-noach added 8 commits April 28, 2022 19:29

schemadiff: Schema.Entity(), ToStatements(), ToQueries(), ToSQL()

fd15133

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

ToSQL: semicolon

d3ed1d3

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

schemadiff: Apply() a diff onto a Create statement

a2b052c

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

schema.Apply diffs

b35589e

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

duplicate schema by exporting to statements and importing

f39f5b4

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

duplicate table by exporting to SQL and parsing again

e03306e

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

validate Diff generated errors when applied wrongly

b9c0eb8

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

code comments

fa2c227

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving release notes none labels May 1, 2022

shlomi-noach marked this pull request as draft May 1, 2022 12:44

shlomi-noach added 2 commits May 1, 2022 16:05

duplicate CreateTable construct

c699a5e

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

verbose output on test

eb2ff02

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach marked this pull request as ready for review May 1, 2022 13:26

dbussink reviewed May 1, 2022

View reviewed changes

shlomi-noach added 3 commits May 2, 2022 06:24

fix/improve tablespec duplication

3eadf59

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

wrapf->wrap

2d53580

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

consistent use and update of s.named in apply()

6db2854

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

dbussink approved these changes May 2, 2022

View reviewed changes

shlomi-noach added 2 commits May 2, 2022 11:30

Merge branch 'main' into schemadiff-apply-diff

7c1f25c

goimports

a366214

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach merged commit 92e0bed into vitessio:main May 2, 2022

shlomi-noach deleted the schemadiff-apply-diff branch May 2, 2022 10:35

shlomi-noach mentioned this pull request May 2, 2022

schemadiff: Schema.Entity(), ToStatements(), ToQueries(), ToSQL() #10164

Closed

3 tasks

shlomi-noach added release notes and removed release notes none labels May 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schemadiff: logically Apply() a diff to a CreateTable, CreateView or to a Schema #10183

schemadiff: logically Apply() a diff to a CreateTable, CreateView or to a Schema #10183

shlomi-noach commented May 1, 2022

shlomi-noach commented May 1, 2022

shlomi-noach commented May 1, 2022

dbussink May 1, 2022

shlomi-noach May 2, 2022

dbussink May 1, 2022

shlomi-noach May 2, 2022

dbussink May 2, 2022

shlomi-noach May 2, 2022

dbussink May 2, 2022

shlomi-noach May 2, 2022

shlomi-noach May 2, 2022

dbussink May 1, 2022

shlomi-noach May 2, 2022

dbussink May 1, 2022

shlomi-noach May 2, 2022

dbussink May 1, 2022

shlomi-noach May 2, 2022

dbussink May 1, 2022

shlomi-noach May 2, 2022

dbussink May 1, 2022

shlomi-noach May 2, 2022

shlomi-noach commented May 3, 2022

schemadiff: logically Apply() a diff to a CreateTable, CreateView or to a Schema #10183

schemadiff: logically Apply() a diff to a CreateTable, CreateView or to a Schema #10183

Conversation

shlomi-noach commented May 1, 2022

Description

Related Issue(s)

Checklist

shlomi-noach commented May 1, 2022

shlomi-noach commented May 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomi-noach commented May 3, 2022