schemadiff: validate views' referenced columns via semantics #12565

shlomi-noach · 2023-03-07T15:44:34Z

An altrnative to #12147, that uses go/vt/vtgate/semantics to validate view column references.

Description

Up till now, schemadiff views validation and analysis was limited to:

Obviously validate statement
Infer tables (or views) reverenced by a view
Validate those tables/views exist
Validate no dependency loop

As of this PR, schemadiff also validates columns used and returned by views:

Validate that columns in SELECT and in WHERE clause do in fact exist in tables/views referenced by the view. Error when an unknown column is encountered.
Allow unqualified table name where there is no ambiguity. Error on unqualified name with ambiguity.
Analyze view's column names, whether aliased or not.
Support cascaded views.
Support star expression, either qualified (create view v as select t.* from t, t2 ...) or unqualified (create view v as select * from t1,t2) ; infer list of columns ; support cascaded views based on star expression views.

Plenty tests added to validate behavior.

Joint with @dbussink and based on his preliminary work.

Related Issue(s)

Tracking issue: schemadiff: tracking issue #10203

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
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>

shlomi-noach · 2023-03-14T11:03:47Z

are any more reviews required from @vitessio/query-serving ?

harshit-gangal · 2023-03-20T07:55:07Z

go/vt/schemadiff/errors.go

+	switch {
+	case e.Column == "":
+		return fmt.Sprintf("view %s references non-existent table %s", sqlescape.EscapeID(e.View), sqlescape.EscapeID(e.Table))
+	case e.Table != "":
+		return fmt.Sprintf("view %s references non-existent column %s.%s", sqlescape.EscapeID(e.View), sqlescape.EscapeID(e.Table), sqlescape.EscapeID(e.Column))
+	case e.Ambiguous:
+		return fmt.Sprintf("view %s references unqualified but non unique column %s", sqlescape.EscapeID(e.View), sqlescape.EscapeID(e.Column))
+	default:
+		return fmt.Sprintf("view %s references unqualified but non-existent column %s", sqlescape.EscapeID(e.View), sqlescape.EscapeID(e.Column))


the error message looks very fragile based on what struct fields are set.

can we also create the message ahead of time, when the struct is created?

I think that’s actually the wrong pattern. The core is the structured error and the string format is just a detail here.

The structured error removes the issue of string being brittle because anyone wants more details doesn’t look at the string at all.

can we also create the message ahead of time, when the struct is created?

Id' like to avoid that right now in the context of this PR, because that would require us to rewrite all error handling.

Also, I'm really not sure, because we want to be able to create errors using various combination of fields. We don't have a single "constructor" ; we'd need different New*() function for any combination of fields.

the error message looks very fragile based on what struct fields are set.

I can rewrite that to be more clear

I was able to simplify InvalidColumnReferencedInViewError by removing unused Table field and subsequent error message generation cases.

@harshit-gangal allocating the string is also wasteful since when using structured errors it is never used. So I don’t think we should do that.

At the MySQL protocol level, error strings are transferred over the wire, so we might be generating them eventually.

@harshit-gangal this is never used for MySQL level errors. But even then, we’d allocate when needed and not always so then it’s imho still an improvement.

It also improved the ergonomics imho since it’s now easier to create a structured error then.

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

shlomi-noach · 2023-03-20T08:26:28Z

PR seems to suffer a cyclic import path.

harshit-gangal · 2023-03-20T08:33:05Z

go/vt/schemadiff/semantics.go

+	semantics.FakeSI
+}
+
+func newDeclarativeSchemaInformation() *declarativeSchemaInformation {
+	return &declarativeSchemaInformation{
+		semantics.FakeSI{
+			Tables: make(map[string]*vindexes.Table),
+		},


FakeSI is for testing, better to take the field from there and create your own full struct.

harshit-gangal

2 comments. Overall looks good.

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

frouioui · 2023-03-20T09:08:52Z

go/vt/schemadiff/schema.go

+	return errs.AggrError(vterrors.Aggregate)
+}
+
+// getTableColumnNames returns the names of columns in given table.


Suggested change

// getTableColumnNames returns the names of columns in given table.

// getEntityColumnNames returns the names of columns in given table.

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

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

shlomi-noach · 2023-03-20T09:56:15Z

Depends on #12663 to solve import loop

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

shlomi-noach · 2023-03-20T11:49:48Z

Merged resolve-semantics-engine-deps, introduced here.

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

shlomi-noach · 2023-03-20T17:14:31Z

only merge this after #12663 is merged.

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

shlomi-noach added 25 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>

shlomi-noach added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving labels Mar 7, 2023

shlomi-noach requested review from harshit-gangal, systay and frouioui as code owners March 7, 2023 15:44

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

95a963e

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

systay approved these changes Mar 9, 2023

View reviewed changes

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

ebf8a12

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

shlomi-noach mentioned this pull request Mar 13, 2023

schemadiff: diffing two schemas generates a rich SchemaDiff object #12551

Merged

4 tasks

harshit-gangal reviewed Mar 20, 2023

View reviewed changes

simplify InvalidColumnReferencedInViewError

1244c0b

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

harshit-gangal reviewed Mar 20, 2023

View reviewed changes

harshit-gangal approved these changes Mar 20, 2023

View reviewed changes

do not use FakeSI, create my own implementation of semantics.SchemaIn…

8b07d21

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

frouioui reviewed Mar 20, 2023

View reviewed changes

frouioui approved these changes Mar 20, 2023

View reviewed changes

shlomi-noach added 2 commits March 20, 2023 11:32

Refactor: go/vt/vtgate/engine/opcode to reduce semantics package depe…

017e10d

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

add new package

eac2860

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

shlomi-noach mentioned this pull request Mar 20, 2023

Refactor: go/vt/vtgate/engine/opcode to reduce semantics package dependencies #12663

Merged

4 tasks

shlomi-noach added 2 commits March 20, 2023 11:36

copyright

4daa08b

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

fix function comment

1699955

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

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>

shlomi-noach requested review from mattlord, rohit-nayak-ps, rsajwani and arthurschreiber as code owners March 20, 2023 11:49

empty commit to kick CI

1c54579

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

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

dfd1cc5

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

shlomi-noach merged commit 61b73b6 into vitessio:main Mar 21, 2023

shlomi-noach deleted the schemadiff-validate-view-columns-semantics branch March 21, 2023 03:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schemadiff: validate views' referenced columns via semantics #12565

schemadiff: validate views' referenced columns via semantics #12565

shlomi-noach commented Mar 7, 2023 •

edited by frouioui

Loading

shlomi-noach commented Mar 14, 2023

harshit-gangal Mar 20, 2023

harshit-gangal Mar 20, 2023

dbussink Mar 20, 2023

shlomi-noach Mar 20, 2023

shlomi-noach Mar 20, 2023

shlomi-noach Mar 20, 2023 •

edited

Loading

dbussink Mar 20, 2023

harshit-gangal Mar 20, 2023

dbussink Mar 20, 2023

shlomi-noach commented Mar 20, 2023

harshit-gangal Mar 20, 2023

shlomi-noach Mar 20, 2023

harshit-gangal left a comment

frouioui Mar 20, 2023

shlomi-noach commented Mar 20, 2023

shlomi-noach commented Mar 20, 2023

shlomi-noach commented Mar 20, 2023

	// getTableColumnNames returns the names of columns in given table.
	// getEntityColumnNames returns the names of columns in given table.

schemadiff: validate views' referenced columns via semantics #12565

schemadiff: validate views' referenced columns via semantics #12565

Conversation

shlomi-noach commented Mar 7, 2023 • edited by frouioui Loading

Description

Related Issue(s)

Checklist

Deployment Notes

shlomi-noach commented Mar 14, 2023

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 Mar 20, 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

shlomi-noach commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshit-gangal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomi-noach commented Mar 20, 2023

shlomi-noach commented Mar 20, 2023

shlomi-noach commented Mar 20, 2023

shlomi-noach commented Mar 7, 2023 •

edited by frouioui

Loading

shlomi-noach Mar 20, 2023 •

edited

Loading