From ddeabc7598f8e73c1014469e4dc77eb0309025ea Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 27 Mar 2023 13:50:51 +0200 Subject: [PATCH] [planner fix] make unknown column an error only for sharded queries (#12704) (#12726) Signed-off-by: Andres Taylor --- .../planbuilder/testdata/from_cases.json | 84 ++++++++++++++++++- .../testdata/systemtables_cases80.json | 6 +- .../planbuilder/testdata/union_cases.json | 37 ++++++++ go/vt/vtgate/semantics/analyzer.go | 12 +-- go/vt/vtgate/semantics/analyzer_test.go | 69 ++++++++++----- go/vt/vtgate/semantics/binder.go | 2 +- 6 files changed, 176 insertions(+), 34 deletions(-) diff --git a/go/vt/vtgate/planbuilder/testdata/from_cases.json b/go/vt/vtgate/planbuilder/testdata/from_cases.json index bfef302f98..49b0cf5a3d 100644 --- a/go/vt/vtgate/planbuilder/testdata/from_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/from_cases.json @@ -4588,7 +4588,25 @@ { "comment": "verify ',' vs JOIN precedence", "query": "select u1.a from unsharded u1, unsharded u2 join unsharded u3 on u1.a = u2.a", - "plan": "symbol u1.a not found" + "v3-plan": "symbol u1.a not found", + "gen4-plan": { + "QueryType": "SELECT", + "Original": "select u1.a from unsharded u1, unsharded u2 join unsharded u3 on u1.a = u2.a", + "Instructions": { + "OperatorType": "Route", + "Variant": "Unsharded", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "FieldQuery": "select u1.a from unsharded as u1, unsharded as u2 join unsharded as u3 on u1.a = u2.a where 1 != 1", + "Query": "select u1.a from unsharded as u1, unsharded as u2 join unsharded as u3 on u1.a = u2.a", + "Table": "unsharded" + }, + "TablesUsed": [ + "main.unsharded" + ] + } }, { "comment": "first expression fails for ',' join (code coverage: ensure error is returned)", @@ -4598,7 +4616,25 @@ { "comment": "table names should be case-sensitive", "query": "select unsharded.id from unsharded where Unsharded.val = 1", - "plan": "symbol Unsharded.val not found" + "v3-plan": "symbol Unsharded.val not found", + "gen4-plan": { + "QueryType": "SELECT", + "Original": "select unsharded.id from unsharded where Unsharded.val = 1", + "Instructions": { + "OperatorType": "Route", + "Variant": "Unsharded", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "FieldQuery": "select unsharded.id from unsharded where 1 != 1", + "Query": "select unsharded.id from unsharded where Unsharded.val = 1", + "Table": "unsharded" + }, + "TablesUsed": [ + "main.unsharded" + ] + } }, { "comment": "implicit table reference for sharded keyspace", @@ -6207,5 +6243,49 @@ "user.user" ] } + }, + { + "comment": "missing and ambiguous column info is OK as long as we can send the query to a single unsharded keyspace", + "query": "select missing_column from unsharded, unsharded_a", + "v3-plan": { + "QueryType": "SELECT", + "Original": "select missing_column from unsharded, unsharded_a", + "Instructions": { + "OperatorType": "Route", + "Variant": "Unsharded", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "FieldQuery": "select missing_column from unsharded, unsharded_a where 1 != 1", + "Query": "select missing_column from unsharded, unsharded_a", + "Table": "unsharded, unsharded_a" + } + }, + "gen4-plan": { + "QueryType": "SELECT", + "Original": "select missing_column from unsharded, unsharded_a", + "Instructions": { + "OperatorType": "Route", + "Variant": "Unsharded", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "FieldQuery": "select missing_column from unsharded, unsharded_a where 1 != 1", + "Query": "select missing_column from unsharded, unsharded_a", + "Table": "unsharded, unsharded_a" + }, + "TablesUsed": [ + "main.unsharded", + "main.unsharded_a" + ] + } + }, + { + "comment": "missing and ambiguous column info is not valid when we have two different unsharded keyspaces in the query", + "query": "select missing_column from unsharded, unsharded_tab", + "v3-plan": "symbol missing_column not found", + "gen4-plan": "Column 'missing_column' in field list is ambiguous" } ] diff --git a/go/vt/vtgate/planbuilder/testdata/systemtables_cases80.json b/go/vt/vtgate/planbuilder/testdata/systemtables_cases80.json index aaaa47879b..41b30d3215 100644 --- a/go/vt/vtgate/planbuilder/testdata/systemtables_cases80.json +++ b/go/vt/vtgate/planbuilder/testdata/systemtables_cases80.json @@ -37,7 +37,7 @@ "Table": "information_schema.a, information_schema.b" } }, - "gen4-plan": "symbol a.id not found" + "gen4-plan": "symbol b.id not found" }, { "comment": "information schema query that uses table_schema", @@ -382,7 +382,7 @@ ] } }, - "gen4-plan": "symbol KCU.DELETE_RULE not found" + "gen4-plan": "symbol S.TABLE_NAME not found" }, { "comment": "information_schema.routines", @@ -1182,7 +1182,7 @@ "Table": "INFORMATION_SCHEMA.`TABLES`" } }, - "gen4-plan": "symbol other_column not found" + "gen4-plan": "symbol foobar not found" }, { "comment": "expand star with information schema", diff --git a/go/vt/vtgate/planbuilder/testdata/union_cases.json b/go/vt/vtgate/planbuilder/testdata/union_cases.json index c527b69de7..62970b1b2b 100644 --- a/go/vt/vtgate/planbuilder/testdata/union_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/union_cases.json @@ -2342,5 +2342,42 @@ "user.user" ] } + }, + { + "comment": "unknown columns are OK as long as the whole query is unsharded", + "query": "(SELECT * FROM (SELECT * FROM unsharded WHERE branchId = 203622 AND buildNumber <= 113893 AND state = 'FAILED' ORDER BY buildNumber DESC LIMIT 1) AS last_failed) UNION ALL (SELECT * FROM (SELECT * FROM unsharded WHERE branchId = 203622 AND buildNumber <= 113893 AND state = 'SUCCEEDED' ORDER BY buildNumber DESC LIMIT 1) AS last_succeeded) ORDER BY buildNumber DESC LIMIT 1", + "v3-plan": { + "QueryType": "SELECT", + "Original": "(SELECT * FROM (SELECT * FROM unsharded WHERE branchId = 203622 AND buildNumber <= 113893 AND state = 'FAILED' ORDER BY buildNumber DESC LIMIT 1) AS last_failed) UNION ALL (SELECT * FROM (SELECT * FROM unsharded WHERE branchId = 203622 AND buildNumber <= 113893 AND state = 'SUCCEEDED' ORDER BY buildNumber DESC LIMIT 1) AS last_succeeded) ORDER BY buildNumber DESC LIMIT 1", + "Instructions": { + "OperatorType": "Route", + "Variant": "Unsharded", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "FieldQuery": "select * from (select * from unsharded where 1 != 1) as last_failed where 1 != 1 union all select * from (select * from unsharded where 1 != 1) as last_succeeded where 1 != 1", + "Query": "select * from (select * from unsharded where branchId = 203622 and buildNumber <= 113893 and state = 'FAILED' order by buildNumber desc limit 1) as last_failed union all select * from (select * from unsharded where branchId = 203622 and buildNumber <= 113893 and state = 'SUCCEEDED' order by buildNumber desc limit 1) as last_succeeded order by buildNumber desc limit 1", + "Table": "unsharded" + } + }, + "gen4-plan": { + "QueryType": "SELECT", + "Original": "(SELECT * FROM (SELECT * FROM unsharded WHERE branchId = 203622 AND buildNumber <= 113893 AND state = 'FAILED' ORDER BY buildNumber DESC LIMIT 1) AS last_failed) UNION ALL (SELECT * FROM (SELECT * FROM unsharded WHERE branchId = 203622 AND buildNumber <= 113893 AND state = 'SUCCEEDED' ORDER BY buildNumber DESC LIMIT 1) AS last_succeeded) ORDER BY buildNumber DESC LIMIT 1", + "Instructions": { + "OperatorType": "Route", + "Variant": "Unsharded", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "FieldQuery": "select * from (select * from unsharded where 1 != 1) as last_failed where 1 != 1 union all select * from (select * from unsharded where 1 != 1) as last_succeeded where 1 != 1", + "Query": "select * from (select * from unsharded where branchId = 203622 and buildNumber <= 113893 and state = 'FAILED' order by buildNumber desc limit 1) as last_failed union all select * from (select * from unsharded where branchId = 203622 and buildNumber <= 113893 and state = 'SUCCEEDED' order by buildNumber desc limit 1) as last_succeeded order by buildNumber desc limit 1", + "Table": "unsharded" + }, + "TablesUsed": [ + "main.unsharded" + ] + } } ] diff --git a/go/vt/vtgate/semantics/analyzer.go b/go/vt/vtgate/semantics/analyzer.go index a0e1ec2eba..c68952143a 100644 --- a/go/vt/vtgate/semantics/analyzer.go +++ b/go/vt/vtgate/semantics/analyzer.go @@ -105,7 +105,7 @@ func (a *analyzer) setError(err error) { switch err := err.(type) { case ProjError: a.projErr = err.Inner - case UnshardedError: + case ShardedError: a.unshardedErr = err.Inner default: if a.inProjection > 0 && vterrors.ErrState(err) == vterrors.NonUniqError { @@ -256,11 +256,11 @@ func (a *analyzer) checkForInvalidConstructs(cursor *sqlparser.Cursor) error { switch node := cursor.Node().(type) { case *sqlparser.Update: if len(node.TableExprs) != 1 { - return UnshardedError{Inner: vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: multiple tables in update")} + return ShardedError{Inner: vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: multiple tables in update")} } alias, isAlias := node.TableExprs[0].(*sqlparser.AliasedTableExpr) if !isAlias { - return UnshardedError{Inner: vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: multiple tables in update")} + return ShardedError{Inner: vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: multiple tables in update")} } _, isDerived := alias.Expr.(*sqlparser.DerivedTable) if isDerived { @@ -353,12 +353,12 @@ func (p ProjError) Error() string { return p.Inner.Error() } -// UnshardedError is used to mark an error as something that should only be returned +// ShardedError is used to mark an error as something that should only be returned // if the query is not unsharded -type UnshardedError struct { +type ShardedError struct { Inner error } -func (p UnshardedError) Error() string { +func (p ShardedError) Error() string { return p.Inner.Error() } diff --git a/go/vt/vtgate/semantics/analyzer_test.go b/go/vt/vtgate/semantics/analyzer_test.go index d67a960fd9..07826fd7a9 100644 --- a/go/vt/vtgate/semantics/analyzer_test.go +++ b/go/vt/vtgate/semantics/analyzer_test.go @@ -124,10 +124,10 @@ func TestBindingSingleTableNegative(t *testing.T) { t.Run(query, func(t *testing.T) { parse, err := sqlparser.Parse(query) require.NoError(t, err) - _, err = Analyze(parse.(sqlparser.SelectStatement), "d", &FakeSI{}) - require.Error(t, err) - require.Contains(t, err.Error(), "symbol") - require.Contains(t, err.Error(), "not found") + st, err := Analyze(parse.(sqlparser.SelectStatement), "d", &FakeSI{}) + require.NoError(t, err) + require.ErrorContains(t, st.NotUnshardedErr, "symbol") + require.ErrorContains(t, st.NotUnshardedErr, "not found") }) } } @@ -144,12 +144,13 @@ func TestBindingSingleAliasedTableNegative(t *testing.T) { t.Run(query, func(t *testing.T) { parse, err := sqlparser.Parse(query) require.NoError(t, err) - _, err = Analyze(parse.(sqlparser.SelectStatement), "", &FakeSI{ + st, err := Analyze(parse.(sqlparser.SelectStatement), "", &FakeSI{ Tables: map[string]*vindexes.Table{ "t": {Name: sqlparser.NewIdentifierCS("t")}, }, }) - require.Error(t, err) + require.NoError(t, err) + require.Error(t, st.NotUnshardedErr) }) } } @@ -310,9 +311,9 @@ func TestMissingTable(t *testing.T) { for _, query := range queries { t.Run(query, func(t *testing.T) { parse, _ := sqlparser.Parse(query) - _, err := Analyze(parse.(sqlparser.SelectStatement), "", &FakeSI{}) - require.Error(t, err) - require.Contains(t, err.Error(), "symbol t.col not found") + st, err := Analyze(parse.(sqlparser.SelectStatement), "", &FakeSI{}) + require.NoError(t, err) + require.ErrorContains(t, st.NotUnshardedErr, "symbol t.col not found") }) } } @@ -470,16 +471,13 @@ func TestScoping(t *testing.T) { t.Run(query.query, func(t *testing.T) { parse, err := sqlparser.Parse(query.query) require.NoError(t, err) - _, err = Analyze(parse.(sqlparser.SelectStatement), "user", &FakeSI{ + st, err := Analyze(parse.(sqlparser.SelectStatement), "user", &FakeSI{ Tables: map[string]*vindexes.Table{ "t": {Name: sqlparser.NewIdentifierCS("t")}, }, }) - if query.errorMessage == "" { - require.NoError(t, err) - } else { - require.EqualError(t, err, query.errorMessage) - } + require.NoError(t, err) + require.EqualError(t, st.NotUnshardedErr, query.errorMessage) }) } } @@ -871,8 +869,9 @@ func TestUnionOrderByRewrite(t *testing.T) { func TestInvalidQueries(t *testing.T) { tcases := []struct { - sql string - err string + sql string + err string + shardedErr string }{{ sql: "select t1.id, t1.col1 from t1 union select t2.uid from t2", err: "The used SELECT statements have a different number of columns", @@ -900,15 +899,37 @@ func TestInvalidQueries(t *testing.T) { }, { sql: "select (select sql_calc_found_rows id from a) as t", err: "Incorrect usage/placement of 'SQL_CALC_FOUND_ROWS'", + }, { + sql: "select id from t1 natural join t2", + err: "unsupported: natural join", + }, { + sql: "select * from music where user_id IN (select sql_calc_found_rows * from music limit 10)", + err: "Incorrect usage/placement of 'SQL_CALC_FOUND_ROWS'", + }, { + sql: "select is_free_lock('xyz') from user", + err: "is_free_lock('xyz') allowed only with dual", + }, { + sql: "SELECT * FROM JSON_TABLE('[ {\"c1\": null} ]','$[*]' COLUMNS( c1 INT PATH '$.c1' ERROR ON ERROR )) as jt", + err: "unsupported: json_table expressions", + }, { + sql: "select does_not_exist from t1", + shardedErr: "symbol does_not_exist not found", + }, { + sql: "select t1.does_not_exist from t1, t2", + shardedErr: "symbol t1.does_not_exist not found", }} for _, tc := range tcases { t.Run(tc.sql, func(t *testing.T) { parse, err := sqlparser.Parse(tc.sql) require.NoError(t, err) - _, err = Analyze(parse.(sqlparser.SelectStatement), "dbName", fakeSchemaInfo()) - require.Error(t, err) - require.Equal(t, tc.err, err.Error()) + st, err := Analyze(parse.(sqlparser.SelectStatement), "dbName", fakeSchemaInfo()) + if tc.err != "" { + require.EqualError(t, err, tc.err) + } else { + require.NoError(t, err, tc.err) + require.EqualError(t, st.NotUnshardedErr, tc.shardedErr) + } }) } } @@ -1021,9 +1042,13 @@ func TestScopingWDerivedTables(t *testing.T) { "t": {Name: sqlparser.NewIdentifierCS("t")}, }, }) - if query.errorMessage != "" { + + switch { + case query.errorMessage != "" && err != nil: require.EqualError(t, err, query.errorMessage) - } else { + case query.errorMessage != "": + require.EqualError(t, st.NotUnshardedErr, query.errorMessage) + default: require.NoError(t, err) sel := parse.(*sqlparser.Select) assert.Equal(t, query.recursiveExpectation, st.RecursiveDeps(extract(sel, 0)), "RecursiveDeps") diff --git a/go/vt/vtgate/semantics/binder.go b/go/vt/vtgate/semantics/binder.go index 99e65d87ab..8d1e2c434f 100644 --- a/go/vt/vtgate/semantics/binder.go +++ b/go/vt/vtgate/semantics/binder.go @@ -260,7 +260,7 @@ func (b *binder) resolveColumn(colName *sqlparser.ColName, current *scope, allow } current = current.parent } - return dependency{}, vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.BadFieldError, "symbol %s not found", sqlparser.String(colName)) + return dependency{}, ShardedError{Inner: vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.BadFieldError, "symbol %s not found", sqlparser.String(colName))} } func (b *binder) resolveColumnInScope(current *scope, expr *sqlparser.ColName, allowMulti bool) (dependencies, error) {