`schemadiff`: multi-error in schema normalization #12675

shlomi-noach · 2023-03-21T03:53:47Z

Description

In this PR a Schema's normalize() function, which also runs validations for the entire schema, tables and views, returns a potentially aggregated error rather than a single error.

The idea is to return as much information as possible about problems in the schema, we the user can have multiple action points they can handle at once. This is instead of receiving a single error, fixing it, trying again, receiving the next single error, etc.

Not all errors make sense to aggregate; we specifically aggregate errors that relate to view dependencies.

To top it all, NewSchemaFromEntities() returns whatever Schema object it may have processed, even if invalid, as opposed to returning nil on error.

Related Issue(s)

#10203

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

Deployment Notes

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

…t views reading from DUAL Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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

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

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

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

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

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

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

go/vt/schemadiff/schema.go

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

rsajwani · 2023-03-22T15:40:12Z

go/vt/schemadiff/errors.go

+}
+
+// Unwrap unwraps an error created by errors.Join() in Go 1.20, into its components
+func Unwrap(err error) []error {


This method will not be called right. u.Unwrap on line 27 is called on object level.

Right now no one is using this function... I added it for completeness.

rsajwani · 2023-03-22T15:41:40Z

go/vt/schemadiff/schema_test.go

@@ -153,6 +153,7 @@ func TestNewSchemaFromQueriesLoop(t *testing.T) {
 	)
 	_, err := NewSchemaFromQueries(queries)
 	assert.Error(t, err)
+	err = UnwrapOne(err)
 	assert.EqualError(t, err, (&ViewDependencyUnresolvedError{View: "v7"}).Error())


can we assert err length here as well .. to assure the numbers of errors are as expected

UnwrapOne() is guaranteed to return a slice of at least one element.

And yes! I'll add that assertion anyway.

Rather, I'll create a errors_test.go with a bunch of unit tests.

Unit tests added

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

ajm188 · 2023-03-22T18:00:04Z

go/vt/schemadiff/errors.go

+// Wrapped is used to unwrap an error created by errors.Join() in Go 1.20
+type Wrapped interface {


this is more a structural comment; i'd suggest either renaming or relocating these types. right now you import schemadiff.Wrapped which, somehow, is for errors? i would expect from that name alone it's some sort of, well, wrapped diff structure (ditto the Unwrap* functions).

so, i'd either advocate for schemadiff.WrappedError or vterrors.Wrapped (or a new package!)

I agree. The location is suboptimal. I initially wanted to do this in vterrors, but vterrors has its own Unwrap. I'll create a new errors package.

Done, refactored!

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

The logic added in vitessio#12675 has a small issue. When a view is found that has an error, it's not present anymore when a subsequent `.ToSQL()` call happens, so it looks like the view disappears. We should never have a view disappear since that could potentially break things and lead to unexpected results. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

rsajwani

LGTM

The logic added in #12675 has a small issue. When a view is found that has an error, it's not present anymore when a subsequent `.ToSQL()` call happens, so it looks like the view disappears. We should never have a view disappear since that could potentially break things and lead to unexpected results. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

shlomi-noach added 30 commits January 24, 2023 14:50

validate table columns referenced by views

c4c213b

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

validate views' columns

3dac907

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

support star expression

e88d426

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

removed 'TODO'

61de6b0

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

rename: errs

389872c

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

use mutierr

56c8421

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

excessive test, removed

11d6379

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

go mod tidy

f4fd673

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

update to latest mutierr

a4ae98d

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

merge main, resolve conflict

ef16bdf

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

avoid setting entity columns in case of error

54fb73a

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

grammar

2d3e70b

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

schemadiff: fix scenario where no tables exist in schema and with jus…

b69ede6

…t views reading from DUAL Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

dual, not dual2

bc34369

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

stripped irrelevant comments

bc8799d

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

merge main, resolve conflict

dd1bc82

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

using AllErrorRecorder instead of multierr package

7f6fd13

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

multierr update

4939d01

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

using FakeSI as table-column model

89d6cba

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

rename variable

2194291

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

clone

38eab14

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

add InvalidStarExprInViewError

7eb83c1

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

Make 'args()' accessible

f98729b

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

using 'go/vt/vtgate/semantics' to analyze view queries. Handling star…

36075ec

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

more test cases

26e9577

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

typo

5718b58

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

unexpected error

fe90b31

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

use ColumnName()

db43534

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

iterate columns instead of Walk

3bf836f

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

Merge branch 'main' into schemadiff-validate-view-columns-semantics

95a963e

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

shlomi-noach added 5 commits March 20, 2023 13:48

Merge branch 'resolve-semantics-engine-deps' into schemadiff-validate…

c66b6e9

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

aggregate errors in normalize()

3004533

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

Unwrap logic for go 1.20 error wrapping

1586e03

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

aggregate errors using go1.20 errors.Join(); aggregate errors on s.Va…

05e2364

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

merge main, resolve conflict

1098f65

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 Component: Online DDL Online DDL (vitess/native/gh-ost/pt-osc) labels Mar 21, 2023

shlomi-noach requested a review from deepthi as a code owner March 21, 2023 03:53

shlomi-noach requested review from dbussink and a team March 21, 2023 03:53

shlomi-noach removed the Component: Online DDL Online DDL (vitess/native/gh-ost/pt-osc) label Mar 21, 2023

dbussink reviewed Mar 21, 2023

View reviewed changes

go/vt/schemadiff/schema.go Show resolved Hide resolved

collect errors

8a03a3f

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

shlomi-noach requested review from a team and dbussink March 22, 2023 10:27

dbussink approved these changes Mar 22, 2023

View reviewed changes

shlomi-noach requested a review from a team March 22, 2023 14:19

rsajwani reviewed Mar 22, 2023

View reviewed changes

unit tests for Unwrap* functions

2bb6e71

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

ajm188 reviewed Mar 22, 2023

View reviewed changes

refactor: new 'errors' package

8c49bd2

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

ajm188 approved these changes Mar 22, 2023

View reviewed changes

shlomi-noach merged commit 7c033f4 into vitessio:main Mar 22, 2023

shlomi-noach deleted the schemadiff-normalize-multi-err branch March 22, 2023 20:01

dbussink mentioned this pull request Mar 23, 2023

Fix the returned schema object for broken views #12713

Merged

4 tasks

rsajwani reviewed Mar 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`schemadiff`: multi-error in schema normalization #12675

`schemadiff`: multi-error in schema normalization #12675

shlomi-noach commented Mar 21, 2023

rsajwani Mar 22, 2023

shlomi-noach Mar 22, 2023

rsajwani Mar 22, 2023

shlomi-noach Mar 22, 2023 •

edited

Loading

shlomi-noach Mar 22, 2023

shlomi-noach Mar 22, 2023

shlomi-noach Mar 22, 2023

ajm188 Mar 22, 2023

shlomi-noach Mar 22, 2023

shlomi-noach Mar 22, 2023

rsajwani left a comment

		// Wrapped is used to unwrap an error created by errors.Join() in Go 1.20
		type Wrapped interface {

schemadiff: multi-error in schema normalization #12675

schemadiff: multi-error in schema normalization #12675

Conversation

shlomi-noach commented Mar 21, 2023

Description

Related Issue(s)

Checklist

Deployment Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomi-noach Mar 22, 2023 • edited Loading

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

rsajwani left a comment

Choose a reason for hiding this comment

`schemadiff`: multi-error in schema normalization #12675

`schemadiff`: multi-error in schema normalization #12675

shlomi-noach Mar 22, 2023 •

edited

Loading