Skip to content

Commit

Permalink
Merge pull request #8664 from planetscale/fix-8575
Browse files Browse the repository at this point in the history 
Gen4: Move expand star inside the semantic analysis
  • Loading branch information
@harshit-gangal
harshit-gangal authored Aug 24, 2021
2 parents 6afc58d + 92e07c2 commit 4358c6a
Show file tree
Hide file tree
Showing 15 changed files with 246 additions and 223 deletions.
2 changes: 1 addition & 1 deletion go/vt/vtgate/planbuilder/abstract/fuzz.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func FuzzAnalyse(data []byte) int {
}
switch stmt := tree.(type) {
case *sqlparser.Select:
semTable, err := semantics.Analyze(stmt, "", &fakeFuzzSI{})
semTable, err := semantics.Analyze(stmt, "", &fakeFuzzSI{}, semantics.NoRewrite)
if err != nil {
return 0
}
Expand Down
2 changes: 1 addition & 1 deletion go/vt/vtgate/planbuilder/abstract/operator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ JoinPredicates:
t.Run(fmt.Sprintf("%d %s", i, sql), func(t *testing.T) {
tree, err := sqlparser.Parse(sql)
require.NoError(t, err)
semTable, err := semantics.Analyze(tree.(sqlparser.SelectStatement), "", &semantics.FakeSI{})
semTable, err := semantics.Analyze(tree.(sqlparser.SelectStatement), "", &semantics.FakeSI{}, semantics.NoRewrite)
require.NoError(t, err)
optree, err := CreateOperatorFromSelect(tree.(*sqlparser.Select), semTable)
require.NoError(t, err)
Expand Down
4 changes: 2 additions & 2 deletions go/vt/vtgate/planbuilder/abstract/queryprojection_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func TestQP(t *testing.T) {
require.NoError(t, err)

sel := stmt.(*sqlparser.Select)
_, err = semantics.Analyze(sel, "", &semantics.FakeSI{})
_, err = semantics.Analyze(sel, "", &semantics.FakeSI{}, semantics.NoRewrite)
require.NoError(t, err)

qp, err := CreateQPFromSelect(sel)
Expand Down Expand Up @@ -200,7 +200,7 @@ func TestQPSimplifiedExpr(t *testing.T) {
ast, err := sqlparser.Parse(tc.query)
require.NoError(t, err)
sel := ast.(*sqlparser.Select)
_, err = semantics.Analyze(sel, "", &semantics.FakeSI{})
_, err = semantics.Analyze(sel, "", &semantics.FakeSI{}, semantics.NoRewrite)
require.NoError(t, err)

qp, err := CreateQPFromSelect(sel)
Expand Down
2 changes: 1 addition & 1 deletion go/vt/vtgate/planbuilder/plan_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -745,7 +745,7 @@ func exerciseAnalyzer(query, database string, s semantics.SchemaInformation) {
return
}

_, _ = semantics.Analyze(sel, database, s)
_, _ = semantics.Analyze(sel, database, s, starRewrite)
}

func BenchmarkSelectVsDML(b *testing.B) {
Expand Down
80 changes: 42 additions & 38 deletions go/vt/vtgate/planbuilder/rewrite.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,21 @@ import (
)

type (
expandStarInfo struct {
proceed bool
tblColMap map[*sqlparser.AliasedTableExpr]sqlparser.SelectExprs
}
rewriter struct {
err error
semTable *semantics.SemTable
reservedVars *sqlparser.ReservedVars
}
)

func starRewrite(statement sqlparser.SelectStatement, semTable *semantics.SemTable) error {
r := rewriter{
semTable: semTable,
}
sqlparser.Rewrite(statement, r.starRewrite, nil)
return r.err
}

func (r *rewriter) starRewrite(cursor *sqlparser.Cursor) bool {
switch node := cursor.Node().(type) {
case *sqlparser.Select:
Expand All @@ -47,60 +51,74 @@ func (r *rewriter) starRewrite(cursor *sqlparser.Cursor) bool {
selExprs = append(selExprs, selectExpr)
continue
}
colNames, expStar, err := expandTableColumns(tables, starExpr)
starExpanded, colNames, err := expandTableColumns(tables, starExpr)
if err != nil {
r.err = err
return false
}
if !expStar.proceed {
if !starExpanded {
selExprs = append(selExprs, selectExpr)
continue
}
selExprs = append(selExprs, colNames...)
for tbl, cols := range expStar.tblColMap {
r.semTable.AddExprs(tbl, cols)
}
}
node.SelectExprs = selExprs
}
return true
}

func expandTableColumns(tables []semantics.TableInfo, starExpr *sqlparser.StarExpr) (sqlparser.SelectExprs, *expandStarInfo, error) {
func expandTableColumns(tables []semantics.TableInfo, starExpr *sqlparser.StarExpr) (bool, sqlparser.SelectExprs, error) {
unknownTbl := true
var colNames sqlparser.SelectExprs
expStar := &expandStarInfo{
tblColMap: map[*sqlparser.AliasedTableExpr]sqlparser.SelectExprs{},
}

starExpanded := true
for _, tbl := range tables {
if !starExpr.TableName.IsEmpty() && !tbl.Matches(starExpr.TableName) {
continue
}
unknownTbl = false
if !tbl.Authoritative() {
expStar.proceed = false
starExpanded = false
break
}
expStar.proceed = true
tblName, err := tbl.Name()
if err != nil {
return nil, nil, err
return false, nil, err
}

withAlias := len(tables) > 1
withQualifier := withAlias || !tbl.GetExpr().As.IsEmpty()
for _, col := range tbl.GetColumns() {
colNames = append(colNames, &sqlparser.AliasedExpr{
Expr: sqlparser.NewColNameWithQualifier(col.Name, tblName),
As: sqlparser.NewColIdent(col.Name),
})
var colName *sqlparser.ColName
var alias sqlparser.ColIdent
if withQualifier {
colName = sqlparser.NewColNameWithQualifier(col.Name, tblName)
} else {
colName = sqlparser.NewColName(col.Name)
}
if withAlias {
alias = sqlparser.NewColIdent(col.Name)
}
colNames = append(colNames, &sqlparser.AliasedExpr{Expr: colName, As: alias})
}
expStar.tblColMap[tbl.GetExpr()] = colNames
}

if unknownTbl {
// This will only happen for case when starExpr has qualifier.
return nil, nil, vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.BadDb, "Unknown table '%s'", sqlparser.String(starExpr.TableName))
return false, nil, vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.BadDb, "Unknown table '%s'", sqlparser.String(starExpr.TableName))
}
return colNames, expStar, nil
return starExpanded, colNames, nil
}

func subqueryRewrite(statement sqlparser.SelectStatement, semTable *semantics.SemTable, reservedVars *sqlparser.ReservedVars) error {
if len(semTable.SubqueryMap) == 0 {
return nil
}
r := rewriter{
semTable: semTable,
reservedVars: reservedVars,
}
sqlparser.Rewrite(statement, r.subqueryRewrite, nil)
return nil
}

func (r *rewriter) subqueryRewrite(cursor *sqlparser.Cursor) bool {
Expand Down Expand Up @@ -136,17 +154,3 @@ func (r *rewriter) subqueryRewrite(cursor *sqlparser.Cursor) bool {
}
return true
}

func (r *rewriter) rewriterPre(cursor *sqlparser.Cursor) bool {
return r.starRewrite(cursor) && (len(r.semTable.SubqueryMap) == 0 || r.subqueryRewrite(cursor))
}

func rewrite(sel *sqlparser.Select, semTable *semantics.SemTable, reservedVars *sqlparser.ReservedVars) (*sqlparser.Select, error) {
r := &rewriter{semTable: semTable, reservedVars: reservedVars}

_ = sqlparser.Rewrite(sel, r.rewriterPre, nil)
if r.err != nil {
return nil, r.err
}
return sel, nil
}
39 changes: 18 additions & 21 deletions go/vt/vtgate/planbuilder/rewrite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,16 +73,16 @@ func TestExpandStar(t *testing.T) {
expErr string
}{{
sql: "select * from t1",
expSQL: "select t1.a as a, t1.b as b, t1.c as c from t1",
expSQL: "select a, b, c from t1",
}, {
sql: "select t1.* from t1",
expSQL: "select t1.a as a, t1.b as b, t1.c as c from t1",
expSQL: "select a, b, c from t1",
}, {
sql: "select *, 42, t1.* from t1",
expSQL: "select t1.a as a, t1.b as b, t1.c as c, 42, t1.a as a, t1.b as b, t1.c as c from t1",
expSQL: "select a, b, c, 42, a, b, c from t1",
}, {
sql: "select 42, t1.* from t1",
expSQL: "select 42, t1.a as a, t1.b as b, t1.c as c from t1",
expSQL: "select 42, a, b, c from t1",
}, {
sql: "select * from t1, t2",
expSQL: "select t1.a as a, t1.b as b, t1.c as c, t2.c1 as c1, t2.c2 as c2 from t1, t2",
Expand Down Expand Up @@ -114,12 +114,10 @@ func TestExpandStar(t *testing.T) {
require.NoError(t, err)
selectStatement, isSelectStatement := ast.(*sqlparser.Select)
require.True(t, isSelectStatement, "analyzer expects a select statement")
semTable, err := semantics.Analyze(selectStatement, cDB, schemaInfo)
require.NoError(t, err)
expandedSelect, err := rewrite(selectStatement, semTable, nil)
_, err = semantics.Analyze(selectStatement, cDB, schemaInfo, starRewrite)
if tcase.expErr == "" {
require.NoError(t, err)
assert.Equal(t, tcase.expSQL, sqlparser.String(expandedSelect))
assert.Equal(t, tcase.expSQL, sqlparser.String(selectStatement))
} else {
require.EqualError(t, err, tcase.expErr)
}
Expand All @@ -145,7 +143,7 @@ func TestSemTableDependenciesAfterExpandStar(t *testing.T) {
expandedCol int
}{{
sql: "select a, * from t1",
expSQL: "select a, t1.a as a from t1",
expSQL: "select a, a from t1",
otherTbl: -1, sameTbl: 0, expandedCol: 1,
}, {
sql: "select t2.a, t1.a, t1.* from t1, t2",
Expand All @@ -162,21 +160,19 @@ func TestSemTableDependenciesAfterExpandStar(t *testing.T) {
require.NoError(t, err)
selectStatement, isSelectStatement := ast.(*sqlparser.Select)
require.True(t, isSelectStatement, "analyzer expects a select statement")
semTable, err := semantics.Analyze(selectStatement, "", schemaInfo)
require.NoError(t, err)
expandedSelect, err := rewrite(selectStatement, semTable, nil)
semTable, err := semantics.Analyze(selectStatement, "", schemaInfo, starRewrite)
require.NoError(t, err)
assert.Equal(t, tcase.expSQL, sqlparser.String(expandedSelect))
assert.Equal(t, tcase.expSQL, sqlparser.String(selectStatement))
if tcase.otherTbl != -1 {
assert.NotEqual(t,
semTable.BaseTableDependencies(expandedSelect.SelectExprs[tcase.otherTbl].(*sqlparser.AliasedExpr).Expr),
semTable.BaseTableDependencies(expandedSelect.SelectExprs[tcase.expandedCol].(*sqlparser.AliasedExpr).Expr),
semTable.BaseTableDependencies(selectStatement.SelectExprs[tcase.otherTbl].(*sqlparser.AliasedExpr).Expr),
semTable.BaseTableDependencies(selectStatement.SelectExprs[tcase.expandedCol].(*sqlparser.AliasedExpr).Expr),
)
}
if tcase.sameTbl != -1 {
assert.Equal(t,
semTable.BaseTableDependencies(expandedSelect.SelectExprs[tcase.sameTbl].(*sqlparser.AliasedExpr).Expr),
semTable.BaseTableDependencies(expandedSelect.SelectExprs[tcase.expandedCol].(*sqlparser.AliasedExpr).Expr),
semTable.BaseTableDependencies(selectStatement.SelectExprs[tcase.sameTbl].(*sqlparser.AliasedExpr).Expr),
semTable.BaseTableDependencies(selectStatement.SelectExprs[tcase.expandedCol].(*sqlparser.AliasedExpr).Expr),
)
}
})
Expand Down Expand Up @@ -226,15 +222,16 @@ func TestSubqueryRewrite(t *testing.T) {
}}
for _, tcase := range tcases {
t.Run(tcase.input, func(t *testing.T) {
ast, err := sqlparser.Parse(tcase.input)
ast, vars, err := sqlparser.Parse2(tcase.input)
require.NoError(t, err)
reservedVars := sqlparser.NewReservedVars("vtg", vars)
selectStatement, isSelectStatement := ast.(*sqlparser.Select)
require.True(t, isSelectStatement, "analyzer expects a select statement")
semTable, err := semantics.Analyze(selectStatement, "", &semantics.FakeSI{})
semTable, err := semantics.Analyze(selectStatement, "", &semantics.FakeSI{}, semantics.NoRewrite)
require.NoError(t, err)
rewrittenAST, err := rewrite(selectStatement, semTable, sqlparser.NewReservedVars("vtg", map[string]struct{}{}))
err = subqueryRewrite(selectStatement, semTable, reservedVars)
require.NoError(t, err)
assert.Equal(t, tcase.output, sqlparser.String(rewrittenAST))
assert.Equal(t, tcase.output, sqlparser.String(selectStatement))
})
}
}
4 changes: 2 additions & 2 deletions go/vt/vtgate/planbuilder/route_planning.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,12 @@ func newBuildSelectPlan(sel *sqlparser.Select, reservedVars *sqlparser.ReservedV
if ks, _ := vschema.DefaultKeyspace(); ks != nil {
ksName = ks.Name
}
semTable, err := semantics.Analyze(sel, ksName, vschema)
semTable, err := semantics.Analyze(sel, ksName, vschema, starRewrite)
if err != nil {
return nil, err
}

sel, err = rewrite(sel, semTable, reservedVars)
err = subqueryRewrite(sel, semTable, reservedVars)
if err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ Gen4 plan same as above
"Name": "user",
"Sharded": true
},
"Query": "create view view_a as select authoritative.user_id as user_id, authoritative.col1 as col1, authoritative.col2 as col2 from authoritative"
"Query": "create view view_a as select user_id, col1, col2 from authoritative"
}
}

Expand Down Expand Up @@ -166,7 +166,7 @@ Gen4 plan same as above
"Name": "user",
"Sharded": true
},
"Query": "create view view_a as select a.user_id as user_id, a.col1 as col1, a.col2 as col2 from authoritative as a"
"Query": "create view view_a as select a.user_id, a.col1, a.col2 from authoritative as a"
}
}

Expand Down
20 changes: 1 addition & 19 deletions go/vt/vtgate/planbuilder/testdata/filter_cases.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2190,25 +2190,7 @@ Gen4 plan same as above
"Vindex": "vindex1"
}
}
{
"QueryType": "SELECT",
"Original": "select * from samecolvin where col = :col",
"Instructions": {
"OperatorType": "Route",
"Variant": "SelectEqualUnique",
"Keyspace": {
"Name": "user",
"Sharded": true
},
"FieldQuery": "select samecolvin.col as col from samecolvin where 1 != 1",
"Query": "select samecolvin.col as col from samecolvin where col = :col",
"Table": "samecolvin",
"Values": [
":col"
],
"Vindex": "vindex1"
}
}
Gen4 plan same as above

# non unique predicate on vindex
"select id from user where user.id > 5"
Expand Down
36 changes: 2 additions & 34 deletions go/vt/vtgate/planbuilder/testdata/postprocess_cases.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,23 +299,7 @@ Gen4 plan same as above
"Table": "authoritative"
}
}
{
"QueryType": "SELECT",
"Original": "select * from authoritative order by user_id",
"Instructions": {
"OperatorType": "Route",
"Variant": "SelectScatter",
"Keyspace": {
"Name": "user",
"Sharded": true
},
"FieldQuery": "select authoritative.user_id as user_id, authoritative.col1 as col1, authoritative.col2 as col2, weight_string(authoritative.user_id) from authoritative where 1 != 1",
"OrderBy": "(0|3) ASC",
"Query": "select authoritative.user_id as user_id, authoritative.col1 as col1, authoritative.col2 as col2, weight_string(authoritative.user_id) from authoritative order by user_id asc",
"ResultColumns": 3,
"Table": "authoritative"
}
}
Gen4 plan same as above

# ORDER BY works for select * from authoritative table
"select * from authoritative order by col1"
Expand All @@ -336,23 +320,7 @@ Gen4 plan same as above
"Table": "authoritative"
}
}
{
"QueryType": "SELECT",
"Original": "select * from authoritative order by col1",
"Instructions": {
"OperatorType": "Route",
"Variant": "SelectScatter",
"Keyspace": {
"Name": "user",
"Sharded": true
},
"FieldQuery": "select authoritative.user_id as user_id, authoritative.col1 as col1, authoritative.col2 as col2, weight_string(authoritative.col1) from authoritative where 1 != 1",
"OrderBy": "(1|3) ASC",
"Query": "select authoritative.user_id as user_id, authoritative.col1 as col1, authoritative.col2 as col2, weight_string(authoritative.col1) from authoritative order by col1 asc",
"ResultColumns": 3,
"Table": "authoritative"
}
}
Gen4 plan same as above

# ORDER BY on scatter with text column
"select a, textcol1, b from user order by a, textcol1, b"
Expand Down
Loading

0 comments on commit 4358c6a

Please sign in to comment.