Skip to content

Commit

Permalink
Bugfix for Panic on Joined Queries with Non-Authoritative Tables in V…
Browse files Browse the repository at this point in the history
…itess 19.0 (#17103)

Signed-off-by: Andres Taylor <[email protected]>
  • Loading branch information
systay authored Oct 30, 2024
1 parent 246ec43 commit 5c08da6
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 1 deletion.
25 changes: 25 additions & 0 deletions go/vt/vtgate/planbuilder/testdata/from_cases.json
Original file line number Diff line number Diff line change
Expand Up @@ -1069,6 +1069,31 @@
]
}
},
{
"comment": "Ambiguous column is not a problem when we can merge and push down",
"query": "select foobar from user join music on user.id = music.user_id order by foobar",
"plan": {
"QueryType": "SELECT",
"Original": "select foobar from user join music on user.id = music.user_id order by foobar",
"Instructions": {
"OperatorType": "Route",
"Variant": "Scatter",
"Keyspace": {
"Name": "user",
"Sharded": true
},
"FieldQuery": "select foobar, weight_string(foobar) from `user`, music where 1 != 1",
"OrderBy": "(0|1) ASC",
"Query": "select foobar, weight_string(foobar) from `user`, music where `user`.id = music.user_id order by foobar asc",
"ResultColumns": 1,
"Table": "`user`, music"
},
"TablesUsed": [
"user.music",
"user.user"
]
}
},
{
"comment": "index hints, make sure they are not stripped.",
"query": "select user.col from user use index(a)",
Expand Down
1 change: 1 addition & 0 deletions go/vt/vtgate/semantics/analyzer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1401,6 +1401,7 @@ func fakeSchemaInfo() *FakeSI {
si := &FakeSI{
Tables: map[string]*vindexes.Table{
"t": {Name: sqlparser.NewIdentifierCS("t"), Keyspace: unsharded},
"t3": {Name: sqlparser.NewIdentifierCS("t3"), Keyspace: unsharded},
"t1": {Name: sqlparser.NewIdentifierCS("t1"), Columns: cols1, ColumnListAuthoritative: true, Keyspace: ks2},
"t2": {Name: sqlparser.NewIdentifierCS("t2"), Columns: cols2, ColumnListAuthoritative: true, Keyspace: ks3},
},
Expand Down
10 changes: 9 additions & 1 deletion go/vt/vtgate/semantics/early_rewriter.go
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,15 @@ func (r *earlyRewriter) fillInQualifiers(cursor *sqlparser.CopyOnWriteCursor) {
if !found {
panic("uh oh")
}
tbl := r.tables.Tables[ts.TableOffset()]
offset := ts.TableOffset()
if offset < 0 {
// this is a column that is not coming from a table - it's an alias introduced in a SELECT expression
// Example: select (1+1) as foo from bar order by foo
// we don't want to add a qualifier to foo here
cursor.Replace(sqlparser.NewColName(col.Name.String()))
return
}
tbl := r.tables.Tables[offset]
tblName, err := tbl.Name()
if err != nil {
panic(err)
Expand Down
27 changes: 27 additions & 0 deletions go/vt/vtgate/semantics/early_rewriter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,33 @@ func TestOrderByColumnName(t *testing.T) {
}
}

func TestOrderByNotAllColumnsAuthoritative(t *testing.T) {
schemaInfo := fakeSchemaInfo()
cDB := "db"
tcases := []struct {
sql string
expSQL string
deps TableSet
}{{
sql: "select foo from t3 join t order by foo",
expSQL: "select foo from t3 join t order by foo asc",
deps: None,
}}
for _, tcase := range tcases {
t.Run(tcase.sql, func(t *testing.T) {
ast, err := sqlparser.NewTestParser().Parse(tcase.sql)
require.NoError(t, err)
selectStatement := ast.(sqlparser.SelectStatement)
semTable, err := Analyze(selectStatement, cDB, schemaInfo)

require.NoError(t, err)
assert.Equal(t, tcase.expSQL, sqlparser.String(selectStatement))
orderByExpr := selectStatement.GetOrderBy()[0].Expr
assert.Equal(t, tcase.deps, semTable.RecursiveDeps(orderByExpr))
})
}
}

func TestSemTableDependenciesAfterExpandStar(t *testing.T) {
schemaInfo := &FakeSI{Tables: map[string]*vindexes.Table{
"t1": {
Expand Down

0 comments on commit 5c08da6

Please sign in to comment.