Skip to content

Commit

Permalink
[planner fix] make unknown column an error only for sharded queries (#…
Browse files Browse the repository at this point in the history 
…12704) (#12725)

Signed-off-by: Andres Taylor <[email protected]>
  • Loading branch information
@systay
systay authored Mar 27, 2023
1 parent 7f9e1e1 commit 965610d
Show file tree
Hide file tree
Showing 5 changed files with 171 additions and 31 deletions.
82 changes: 80 additions & 2 deletions go/vt/vtgate/planbuilder/testdata/from_cases.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4588,7 +4588,24 @@
"comment": "verify ',' vs JOIN precedence",
"query": "select u1.a from unsharded u1, unsharded u2 join unsharded u3 on u1.a = u2.a",
"v3-plan": "VT03019: symbol u1.a not found",
"gen4-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)",
Expand All @@ -4599,7 +4616,24 @@
"comment": "table names should be case-sensitive",
"query": "select unsharded.id from unsharded where Unsharded.val = 1",
"v3-plan": "VT03019: symbol Unsharded.val not found",
"gen4-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",
Expand Down Expand Up @@ -6343,5 +6377,49 @@
"user.user_extra"
]
}
},
{
"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": "VT03019: symbol missing_column not found",
"gen4-plan": "Column 'missing_column' in field list is ambiguous"
}
]
37 changes: 37 additions & 0 deletions go/vt/vtgate/planbuilder/testdata/union_cases.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2416,5 +2416,42 @@
"Table": "information_schema.key_column_usage"
}
}
},
{
"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"
]
}
}
]
12 changes: 6 additions & 6 deletions go/vt/vtgate/semantics/analyzer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,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 {
Expand Down Expand Up @@ -260,11 +260,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: NewError(UnsupportedMultiTablesInUpdate)}
return ShardedError{Inner: NewError(UnsupportedMultiTablesInUpdate)}
}
alias, isAlias := node.TableExprs[0].(*sqlparser.AliasedTableExpr)
if !isAlias {
return UnshardedError{Inner: NewError(UnsupportedMultiTablesInUpdate)}
return ShardedError{Inner: NewError(UnsupportedMultiTablesInUpdate)}
}
_, isDerived := alias.Expr.(*sqlparser.DerivedTable)
if isDerived {
Expand Down Expand Up @@ -357,12 +357,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()
}
69 changes: 47 additions & 22 deletions go/vt/vtgate/semantics/analyzer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
})
}
}
Expand All @@ -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)
})
}
}
Expand Down Expand Up @@ -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")
})
}
}
Expand Down Expand Up @@ -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)
})
}
}
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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: "VT12001: 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: "VT12001: 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)
}
})
}
}
Expand Down Expand Up @@ -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")
Expand Down
2 changes: 1 addition & 1 deletion go/vt/vtgate/semantics/binder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ func (b *binder) resolveColumn(colName *sqlparser.ColName, current *scope, allow
}
current = current.parent
}
return dependency{}, NewError(ColumnNotFound, colName)
return dependency{}, ShardedError{Inner: NewError(ColumnNotFound, colName)}
}

func (b *binder) resolveColumnInScope(current *scope, expr *sqlparser.ColName, allowMulti bool) (dependencies, error) {
Expand Down

0 comments on commit 965610d

Please sign in to comment.