From 48e970f9664bbedbd54a08705eb26c0f35c0108e Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 10 Jun 2021 15:53:45 +0200 Subject: [PATCH 1/7] first failing expand star test Signed-off-by: Andres Taylor --- go/vt/vtgate/planbuilder/route_planning.go | 7 +++++ .../vtgate/planbuilder/route_planning_test.go | 30 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index 6ed1768f407..75c4d2b4de3 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -55,6 +55,8 @@ func newBuildSelectPlan(sel *sqlparser.Select, vschema ContextVSchema) (engine.P return nil, err } + sel = expandStar(sel, semTable, vschema) + qgraph, err := createQGFromSelect(sel, semTable) if err != nil { return nil, err @@ -97,6 +99,11 @@ func newBuildSelectPlan(sel *sqlparser.Select, vschema ContextVSchema) (engine.P return plan.Primitive(), nil } +func expandStar(sel *sqlparser.Select, semTable *semantics.SemTable, vschema semantics.SchemaInformation) *sqlparser.Select { + // TODO we could store in semTable whether there are any * in the query that needs expanding or not + return sel +} + func planLimit(limit *sqlparser.Limit, plan logicalPlan) (logicalPlan, error) { if limit == nil { return plan, nil diff --git a/go/vt/vtgate/planbuilder/route_planning_test.go b/go/vt/vtgate/planbuilder/route_planning_test.go index 70161799c35..ecf861474b9 100644 --- a/go/vt/vtgate/planbuilder/route_planning_test.go +++ b/go/vt/vtgate/planbuilder/route_planning_test.go @@ -20,6 +20,10 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/require" + + "vitess.io/vitess/go/sqltypes" + "vitess.io/vitess/go/vt/vtgate/semantics" "github.com/stretchr/testify/assert" @@ -122,3 +126,29 @@ func TestClone(t *testing.T) { assert.NotNil(t, clonedRP.vindexPreds[0].foundVindex) assert.Nil(t, original.vindexPreds[0].foundVindex) } + +func TestExpandStar(t *testing.T) { + ast, err := sqlparser.Parse("select * from tbl") + require.NoError(t, err) + schemaInfo := &fakeSI{ + tables: map[string]*vindexes.Table{ + "tbl": { + Columns: []vindexes.Column{{ + Name: sqlparser.NewColIdent("a"), + Type: sqltypes.VarChar, + }, { + Name: sqlparser.NewColIdent("b"), + Type: sqltypes.VarChar, + }, { + Name: sqlparser.NewColIdent("c"), + Type: sqltypes.VarChar, + }}, + ColumnListAuthoritative: true, + }, + }, + } + semState, err := semantics.Analyze(ast, "db", schemaInfo) + require.NoError(t, err) + expanded := expandStar(ast.(*sqlparser.Select), semState, schemaInfo) + assert.Equal(t, "select tbl.a, tbl.b, tbl.c from tbl", sqlparser.String(expanded)) +} From 8edb2842ddf7e7daf64dff30ea16ae4def421c5d Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Fri, 11 Jun 2021 23:20:54 +0530 Subject: [PATCH 2/7] sem table to store scope of each sqlparser.Select Signed-off-by: Harshit Gangal --- go/vt/vtgate/semantics/analyzer.go | 13 +++++++++---- go/vt/vtgate/semantics/semantic_state.go | 7 +++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/go/vt/vtgate/semantics/analyzer.go b/go/vt/vtgate/semantics/analyzer.go index cc82c982669..f9f70871aef 100644 --- a/go/vt/vtgate/semantics/analyzer.go +++ b/go/vt/vtgate/semantics/analyzer.go @@ -34,15 +34,18 @@ type ( exprDeps map[sqlparser.Expr]TableSet err error currentDb string + + selectScope map[*sqlparser.Select]*scope } ) // newAnalyzer create the semantic analyzer func newAnalyzer(dbName string, si SchemaInformation) *analyzer { return &analyzer{ - exprDeps: map[sqlparser.Expr]TableSet{}, - currentDb: dbName, - si: si, + exprDeps: map[sqlparser.Expr]TableSet{}, + selectScope: map[*sqlparser.Select]*scope{}, + currentDb: dbName, + si: si, } } @@ -54,7 +57,7 @@ func Analyze(statement sqlparser.Statement, currentDb string, si SchemaInformati if err != nil { return nil, err } - return &SemTable{exprDependencies: analyzer.exprDeps, Tables: analyzer.Tables}, nil + return &SemTable{exprDependencies: analyzer.exprDeps, Tables: analyzer.Tables, selectScope: analyzer.selectScope}, nil } // analyzeDown pushes new scopes when we encounter sub queries, @@ -65,6 +68,7 @@ func (a *analyzer) analyzeDown(cursor *sqlparser.Cursor) bool { switch node := n.(type) { case *sqlparser.Select: a.push(newScope(current)) + a.selectScope[node] = a.currentScope() if err := a.analyzeTableExprs(node.From); err != nil { a.err = err return false @@ -87,6 +91,7 @@ func (a *analyzer) analyzeDown(cursor *sqlparser.Cursor) bool { a.exprDeps[node] = t } } + // this is the visitor going down the tree. Returning false here would just not visit the children // to the current node, but that is not what we want if we have encountered an error. // In order to abort the whole visitation, we have to return true here and then return false in the `analyzeUp` method diff --git a/go/vt/vtgate/semantics/semantic_state.go b/go/vt/vtgate/semantics/semantic_state.go index 3fbfb749ba4..7f05ada4e61 100644 --- a/go/vt/vtgate/semantics/semantic_state.go +++ b/go/vt/vtgate/semantics/semantic_state.go @@ -43,6 +43,7 @@ type ( SemTable struct { Tables []*TableInfo exprDependencies map[sqlparser.Expr]TableSet + selectScope map[*sqlparser.Select]*scope } scope struct { @@ -100,6 +101,12 @@ func (st *SemTable) Dependencies(expr sqlparser.Expr) TableSet { return deps } +// GetSelectTables returns the table in the select. +func (st *SemTable) GetSelectTables(node *sqlparser.Select) []*TableInfo { + scope := st.selectScope[node] + return scope.tables +} + func newScope(parent *scope) *scope { return &scope{parent: parent} } From 5e81293fd915402439f1dc6ba9daa1a80b9cee07 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Fri, 11 Jun 2021 23:24:33 +0530 Subject: [PATCH 3/7] expand the starExpr in select Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/route_planning.go | 43 ++++++++++++- .../vtgate/planbuilder/route_planning_test.go | 61 ++++++++++++++++--- 2 files changed, 95 insertions(+), 9 deletions(-) diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index 75c4d2b4de3..4cedb6e137d 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -55,7 +55,7 @@ func newBuildSelectPlan(sel *sqlparser.Select, vschema ContextVSchema) (engine.P return nil, err } - sel = expandStar(sel, semTable, vschema) + sel = expandStar(sel, semTable) qgraph, err := createQGFromSelect(sel, semTable) if err != nil { @@ -99,8 +99,47 @@ func newBuildSelectPlan(sel *sqlparser.Select, vschema ContextVSchema) (engine.P return plan.Primitive(), nil } -func expandStar(sel *sqlparser.Select, semTable *semantics.SemTable, vschema semantics.SchemaInformation) *sqlparser.Select { +func expandStar(sel *sqlparser.Select, semTable *semantics.SemTable) *sqlparser.Select { // TODO we could store in semTable whether there are any * in the query that needs expanding or not + + _ = sqlparser.Rewrite(sel, func(cursor *sqlparser.Cursor) bool { + switch node := cursor.Node().(type) { + case *sqlparser.Select: + tables := semTable.GetSelectTables(node) + var selExprs sqlparser.SelectExprs + for _, selectExpr := range node.SelectExprs { + _, isStarExpr := selectExpr.(*sqlparser.StarExpr) + if !isStarExpr { + selExprs = append(selExprs, selectExpr) + continue + } + //if !starExpr.TableName.IsEmpty() { + // // TODO: only expand specific table + //} + var colNames sqlparser.SelectExprs + expandStar := true + for _, tbl := range tables { + if !tbl.Table.ColumnListAuthoritative { + expandStar = false + break + } + for _, col := range tbl.Table.Columns { + colNames = append(colNames, &sqlparser.AliasedExpr{ + Expr: sqlparser.NewColNameWithQualifier(col.Name.String(), sqlparser.TableName{Name: tbl.Table.Name}), + }) + } + } + if !expandStar { + selExprs = append(selExprs, selectExpr) + continue + } + selExprs = append(selExprs, colNames...) + } + node.SelectExprs = selExprs + } + + return true + }, nil) return sel } diff --git a/go/vt/vtgate/planbuilder/route_planning_test.go b/go/vt/vtgate/planbuilder/route_planning_test.go index ecf861474b9..8e2f1ee3950 100644 --- a/go/vt/vtgate/planbuilder/route_planning_test.go +++ b/go/vt/vtgate/planbuilder/route_planning_test.go @@ -128,11 +128,10 @@ func TestClone(t *testing.T) { } func TestExpandStar(t *testing.T) { - ast, err := sqlparser.Parse("select * from tbl") - require.NoError(t, err) schemaInfo := &fakeSI{ tables: map[string]*vindexes.Table{ - "tbl": { + "t1": { + Name: sqlparser.NewTableIdent("t1"), Columns: []vindexes.Column{{ Name: sqlparser.NewColIdent("a"), Type: sqltypes.VarChar, @@ -145,10 +144,58 @@ func TestExpandStar(t *testing.T) { }}, ColumnListAuthoritative: true, }, + "t2": { + Name: sqlparser.NewTableIdent("t2"), + Columns: []vindexes.Column{{ + Name: sqlparser.NewColIdent("c1"), + Type: sqltypes.VarChar, + }, { + Name: sqlparser.NewColIdent("c2"), + Type: sqltypes.VarChar, + }}, + ColumnListAuthoritative: true, + }, + "t3": { // non authoritative table. + Name: sqlparser.NewTableIdent("t3"), + Columns: []vindexes.Column{{ + Name: sqlparser.NewColIdent("col"), + Type: sqltypes.VarChar, + }}, + ColumnListAuthoritative: false, + }, }, } - semState, err := semantics.Analyze(ast, "db", schemaInfo) - require.NoError(t, err) - expanded := expandStar(ast.(*sqlparser.Select), semState, schemaInfo) - assert.Equal(t, "select tbl.a, tbl.b, tbl.c from tbl", sqlparser.String(expanded)) + cDB := "db" + tcases := []struct { + sql string + expSQL string + }{{ + sql: "select * from t1", + expSQL: "select t1.a, t1.b, t1.c from t1", + }, { + sql: "select t1.* from t1", + expSQL: "select t1.a, t1.b, t1.c from t1", + }, { + sql: "select *, 42, t1.* from t1", + expSQL: "select t1.a, t1.b, t1.c, 42, t1.a, t1.b, t1.c from t1", + }, { + sql: "select 42, t1.* from t1", + expSQL: "select 42, t1.a, t1.b, t1.c from t1", + }, { + sql: "select * from t1, t2", + expSQL: "select t1.a, t1.b, t1.c, t2.c1, t2.c2 from t1, t2", + }, { + sql: "select t1.* from t1, t2", + expSQL: "select t1.a, t1.b, t1.c from t1, t2", + }} + for _, tcase := range tcases { + t.Run(tcase.sql, func(t *testing.T) { + ast, err := sqlparser.Parse(tcase.sql) + require.NoError(t, err) + semState, err := semantics.Analyze(ast, cDB, schemaInfo) + require.NoError(t, err) + expanded := expandStar(ast.(*sqlparser.Select), semState) + assert.Equal(t, tcase.expSQL, sqlparser.String(expanded)) + }) + } } From a9c30c4f28d5c9e1f689145eba40fef271178d05 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 14 Jun 2021 12:55:39 +0530 Subject: [PATCH 4/7] check if starExpr has qualifier and then only expand for that table Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/route_planning.go | 29 +++++++++++++++---- .../vtgate/planbuilder/route_planning_test.go | 6 ++++ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index 4cedb6e137d..7c179729542 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -108,21 +108,38 @@ func expandStar(sel *sqlparser.Select, semTable *semantics.SemTable) *sqlparser. tables := semTable.GetSelectTables(node) var selExprs sqlparser.SelectExprs for _, selectExpr := range node.SelectExprs { - _, isStarExpr := selectExpr.(*sqlparser.StarExpr) + starExpr, isStarExpr := selectExpr.(*sqlparser.StarExpr) if !isStarExpr { selExprs = append(selExprs, selectExpr) continue } - //if !starExpr.TableName.IsEmpty() { - // // TODO: only expand specific table - //} var colNames sqlparser.SelectExprs - expandStar := true + expandStar := false for _, tbl := range tables { + if !starExpr.TableName.IsEmpty() { + if !tbl.ASTNode.As.IsEmpty() { + if !starExpr.TableName.Qualifier.IsEmpty() { + continue + } + if starExpr.TableName.Name.String() != tbl.ASTNode.As.String() { + continue + } + } else { + if !starExpr.TableName.Qualifier.IsEmpty() { + if starExpr.TableName.Qualifier.String() != tbl.Table.Keyspace.Name { + continue + } + } + tblName := tbl.ASTNode.Expr.(sqlparser.TableName) + if starExpr.TableName.Name.String() != tblName.Name.String() { + continue + } + } + } if !tbl.Table.ColumnListAuthoritative { - expandStar = false break } + expandStar = true for _, col := range tbl.Table.Columns { colNames = append(colNames, &sqlparser.AliasedExpr{ Expr: sqlparser.NewColNameWithQualifier(col.Name.String(), sqlparser.TableName{Name: tbl.Table.Name}), diff --git a/go/vt/vtgate/planbuilder/route_planning_test.go b/go/vt/vtgate/planbuilder/route_planning_test.go index 8e2f1ee3950..c17ae60efb2 100644 --- a/go/vt/vtgate/planbuilder/route_planning_test.go +++ b/go/vt/vtgate/planbuilder/route_planning_test.go @@ -187,6 +187,12 @@ func TestExpandStar(t *testing.T) { }, { sql: "select t1.* from t1, t2", expSQL: "select t1.a, t1.b, t1.c from t1, t2", + }, { + sql: "select *, t1.* from t1, t2", + expSQL: "select t1.a, t1.b, t1.c, t2.c1, t2.c2, t1.a, t1.b, t1.c from t1, t2", + }, { // TODO: This should fail on analyze step and should not reach down. + sql: "select t3.* from t1, t2", + expSQL: "select t3.* from t1, t2", }} for _, tcase := range tcases { t.Run(tcase.sql, func(t *testing.T) { From ed06e513e797a6b72e4781ac012d53e1858c2b36 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 14 Jun 2021 14:14:11 +0530 Subject: [PATCH 5/7] make expandStar as false if the table is non-authoritative Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/route_planning.go | 1 + go/vt/vtgate/planbuilder/route_planning_test.go | 13 +++++++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index 7c179729542..be696bca770 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -137,6 +137,7 @@ func expandStar(sel *sqlparser.Select, semTable *semantics.SemTable) *sqlparser. } } if !tbl.Table.ColumnListAuthoritative { + expandStar = false break } expandStar = true diff --git a/go/vt/vtgate/planbuilder/route_planning_test.go b/go/vt/vtgate/planbuilder/route_planning_test.go index c17ae60efb2..24ed2bd688f 100644 --- a/go/vt/vtgate/planbuilder/route_planning_test.go +++ b/go/vt/vtgate/planbuilder/route_planning_test.go @@ -190,9 +190,18 @@ func TestExpandStar(t *testing.T) { }, { sql: "select *, t1.* from t1, t2", expSQL: "select t1.a, t1.b, t1.c, t2.c1, t2.c2, t1.a, t1.b, t1.c from t1, t2", + }, { // t3 is non-authoritative table + sql: "select * from t3", + expSQL: "select * from t3", + }, { // t3 is non-authoritative table + sql: "select * from t1, t2, t3", + expSQL: "select * from t1, t2, t3", + }, { // t3 is non-authoritative table + sql: "select t1.*, t2.*, t3.* from t1, t2, t3", + expSQL: "select t1.a, t1.b, t1.c, t2.c1, t2.c2, t3.* from t1, t2, t3", }, { // TODO: This should fail on analyze step and should not reach down. - sql: "select t3.* from t1, t2", - expSQL: "select t3.* from t1, t2", + sql: "select foo.* from t1, t2", + expSQL: "select foo.* from t1, t2", }} for _, tcase := range tcases { t.Run(tcase.sql, func(t *testing.T) { From 0f520d5e0ab5bd97a936d24ef911c0b29807cd68 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Thu, 17 Jun 2021 16:16:35 +0530 Subject: [PATCH 6/7] add expanded colnames to semTable, error out when aliased star expr table is not found Signed-off-by: Harshit Gangal --- go/mysql/sql_error.go | 1 + go/vt/vterrors/state.go | 1 + go/vt/vtgate/planbuilder/route_planning.go | 139 +++++++++++------- .../vtgate/planbuilder/route_planning_test.go | 89 +++++++++-- go/vt/vtgate/semantics/analyzer.go | 2 +- go/vt/vtgate/semantics/semantic_state.go | 8 + 6 files changed, 175 insertions(+), 65 deletions(-) diff --git a/go/mysql/sql_error.go b/go/mysql/sql_error.go index b2d12af0c49..d0bc64803c9 100644 --- a/go/mysql/sql_error.go +++ b/go/mysql/sql_error.go @@ -163,6 +163,7 @@ var stateToMysqlCode = map[vterrors.State]struct { vterrors.AccessDeniedError: {num: ERAccessDeniedError, state: SSAccessDeniedError}, vterrors.BadDb: {num: ERBadDb, state: SSClientError}, vterrors.BadFieldError: {num: ERBadFieldError, state: SSBadFieldError}, + vterrors.BadTableError: {num: ERBadTable, state: SSUnknownTable}, vterrors.CantUseOptionHere: {num: ERCantUseOptionHere, state: SSClientError}, vterrors.DataOutOfRange: {num: ERDataOutOfRange, state: SSDataOutOfRange}, vterrors.DbCreateExists: {num: ERDbCreateExists, state: SSUnknownSQLState}, diff --git a/go/vt/vterrors/state.go b/go/vt/vterrors/state.go index 8b4e8b9b4aa..b8dc746685e 100644 --- a/go/vt/vterrors/state.go +++ b/go/vt/vterrors/state.go @@ -25,6 +25,7 @@ const ( // invalid argument BadFieldError + BadTableError CantUseOptionHere DataOutOfRange EmptyQuery diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index be696bca770..6c72a76aa69 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -55,7 +55,10 @@ func newBuildSelectPlan(sel *sqlparser.Select, vschema ContextVSchema) (engine.P return nil, err } - sel = expandStar(sel, semTable) + sel, err = expandStar(sel, semTable) + if err != nil { + return nil, err + } qgraph, err := createQGFromSelect(sel, semTable) if err != nil { @@ -99,66 +102,100 @@ func newBuildSelectPlan(sel *sqlparser.Select, vschema ContextVSchema) (engine.P return plan.Primitive(), nil } -func expandStar(sel *sqlparser.Select, semTable *semantics.SemTable) *sqlparser.Select { - // TODO we could store in semTable whether there are any * in the query that needs expanding or not +type starRewriter struct { + err error + semTable *semantics.SemTable +} - _ = sqlparser.Rewrite(sel, func(cursor *sqlparser.Cursor) bool { - switch node := cursor.Node().(type) { - case *sqlparser.Select: - tables := semTable.GetSelectTables(node) - var selExprs sqlparser.SelectExprs - for _, selectExpr := range node.SelectExprs { - starExpr, isStarExpr := selectExpr.(*sqlparser.StarExpr) - if !isStarExpr { - selExprs = append(selExprs, selectExpr) - continue - } - var colNames sqlparser.SelectExprs - expandStar := false - for _, tbl := range tables { - if !starExpr.TableName.IsEmpty() { - if !tbl.ASTNode.As.IsEmpty() { - if !starExpr.TableName.Qualifier.IsEmpty() { - continue - } - if starExpr.TableName.Name.String() != tbl.ASTNode.As.String() { - continue - } - } else { - if !starExpr.TableName.Qualifier.IsEmpty() { - if starExpr.TableName.Qualifier.String() != tbl.Table.Keyspace.Name { - continue - } - } - tblName := tbl.ASTNode.Expr.(sqlparser.TableName) - if starExpr.TableName.Name.String() != tblName.Name.String() { +func (sr *starRewriter) starRewrite(cursor *sqlparser.Cursor) bool { + switch node := cursor.Node().(type) { + case *sqlparser.Select: + tables := sr.semTable.GetSelectTables(node) + var selExprs sqlparser.SelectExprs + for _, selectExpr := range node.SelectExprs { + starExpr, isStarExpr := selectExpr.(*sqlparser.StarExpr) + if !isStarExpr { + selExprs = append(selExprs, selectExpr) + continue + } + expStar := &expandStarInfo{ + tblColMap: map[*sqlparser.AliasedTableExpr]sqlparser.SelectExprs{}, + } + var colNames sqlparser.SelectExprs + unknownTbl := true + for _, tbl := range tables { + if !starExpr.TableName.IsEmpty() { + if !tbl.ASTNode.As.IsEmpty() { + if !starExpr.TableName.Qualifier.IsEmpty() { + continue + } + if starExpr.TableName.Name.String() != tbl.ASTNode.As.String() { + continue + } + } else { + if !starExpr.TableName.Qualifier.IsEmpty() { + if starExpr.TableName.Qualifier.String() != tbl.Table.Keyspace.Name { continue } } + tblName := tbl.ASTNode.Expr.(sqlparser.TableName) + if starExpr.TableName.Name.String() != tblName.Name.String() { + continue + } } - if !tbl.Table.ColumnListAuthoritative { - expandStar = false - break - } - expandStar = true - for _, col := range tbl.Table.Columns { - colNames = append(colNames, &sqlparser.AliasedExpr{ - Expr: sqlparser.NewColNameWithQualifier(col.Name.String(), sqlparser.TableName{Name: tbl.Table.Name}), - }) - } } - if !expandStar { - selExprs = append(selExprs, selectExpr) - continue + unknownTbl = false + if tbl.Table == nil || !tbl.Table.ColumnListAuthoritative { + expStar.proceed = false + break + } + expStar.proceed = true + tblName, err := tbl.ASTNode.TableName() + if err != nil { + sr.err = err + return false + } + for _, col := range tbl.Table.Columns { + colNames = append(colNames, &sqlparser.AliasedExpr{ + Expr: sqlparser.NewColNameWithQualifier(col.Name.String(), tblName), + As: sqlparser.NewColIdent(col.Name.String()), + }) } - selExprs = append(selExprs, colNames...) + expStar.tblColMap[tbl.ASTNode] = colNames + } + if unknownTbl { + // This will only happen for case when starExpr has qualifier. + sr.err = vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.BadDb, "Unknown table '%s'", sqlparser.String(starExpr.TableName)) + return false + } + if !expStar.proceed { + selExprs = append(selExprs, selectExpr) + continue + } + selExprs = append(selExprs, colNames...) + for tbl, cols := range expStar.tblColMap { + sr.semTable.AddExprs(tbl, cols) } - node.SelectExprs = selExprs } + node.SelectExprs = selExprs + } + return true +} - return true - }, nil) - return sel +type expandStarInfo struct { + proceed bool + tblColMap map[*sqlparser.AliasedTableExpr]sqlparser.SelectExprs +} + +func expandStar(sel *sqlparser.Select, semTable *semantics.SemTable) (*sqlparser.Select, error) { + // TODO we could store in semTable whether there are any * in the query that needs expanding or not + sr := &starRewriter{semTable: semTable} + + _ = sqlparser.Rewrite(sel, sr.starRewrite, nil) + if sr.err != nil { + return nil, sr.err + } + return sel, nil } func planLimit(limit *sqlparser.Limit, plan logicalPlan) (logicalPlan, error) { diff --git a/go/vt/vtgate/planbuilder/route_planning_test.go b/go/vt/vtgate/planbuilder/route_planning_test.go index 24ed2bd688f..0def30a8d07 100644 --- a/go/vt/vtgate/planbuilder/route_planning_test.go +++ b/go/vt/vtgate/planbuilder/route_planning_test.go @@ -169,27 +169,31 @@ func TestExpandStar(t *testing.T) { tcases := []struct { sql string expSQL string + expErr string }{{ sql: "select * from t1", - expSQL: "select t1.a, t1.b, t1.c from t1", + expSQL: "select t1.a as a, t1.b as b, t1.c as c from t1", }, { sql: "select t1.* from t1", - expSQL: "select t1.a, t1.b, t1.c from t1", + expSQL: "select t1.a as a, t1.b as b, t1.c as c from t1", }, { sql: "select *, 42, t1.* from t1", - expSQL: "select t1.a, t1.b, t1.c, 42, t1.a, t1.b, t1.c 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", }, { sql: "select 42, t1.* from t1", - expSQL: "select 42, t1.a, t1.b, t1.c from t1", + expSQL: "select 42, t1.a as a, t1.b as b, t1.c as c from t1", }, { sql: "select * from t1, t2", - expSQL: "select t1.a, t1.b, t1.c, t2.c1, t2.c2 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", }, { sql: "select t1.* from t1, t2", - expSQL: "select t1.a, t1.b, t1.c from t1, t2", + expSQL: "select t1.a as a, t1.b as b, t1.c as c from t1, t2", }, { sql: "select *, t1.* from t1, t2", - expSQL: "select t1.a, t1.b, t1.c, t2.c1, t2.c2, t1.a, t1.b, t1.c 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, t1.a as a, t1.b as b, t1.c as c from t1, t2", + }, { // aliased table + sql: "select * from t1 a, t2 b", + expSQL: "select a.a as a, a.b as b, a.c as c, b.c1 as c1, b.c2 as c2 from t1 as a, t2 as b", }, { // t3 is non-authoritative table sql: "select * from t3", expSQL: "select * from t3", @@ -198,19 +202,78 @@ func TestExpandStar(t *testing.T) { expSQL: "select * from t1, t2, t3", }, { // t3 is non-authoritative table sql: "select t1.*, t2.*, t3.* from t1, t2, t3", - expSQL: "select t1.a, t1.b, t1.c, t2.c1, t2.c2, t3.* from t1, t2, t3", - }, { // TODO: This should fail on analyze step and should not reach down. + expSQL: "select t1.a as a, t1.b as b, t1.c as c, t2.c1 as c1, t2.c2 as c2, t3.* from t1, t2, t3", + }, { sql: "select foo.* from t1, t2", - expSQL: "select foo.* from t1, t2", + expErr: "Unknown table 'foo'", + }} + for _, tcase := range tcases { + t.Run(tcase.sql, func(t *testing.T) { + ast, err := sqlparser.Parse(tcase.sql) + require.NoError(t, err) + semTable, err := semantics.Analyze(ast, cDB, schemaInfo) + require.NoError(t, err) + expandedSelect, err := expandStar(ast.(*sqlparser.Select), semTable) + if tcase.expErr == "" { + require.NoError(t, err) + assert.Equal(t, tcase.expSQL, sqlparser.String(expandedSelect)) + } else { + require.EqualError(t, err, tcase.expErr) + } + }) + } +} + +func TestSemTableDependenciesAfterExpandStar(t *testing.T) { + schemaInfo := &fakeSI{tables: map[string]*vindexes.Table{ + "t1": { + Name: sqlparser.NewTableIdent("t1"), + Columns: []vindexes.Column{{ + Name: sqlparser.NewColIdent("a"), + Type: sqltypes.VarChar, + }}, + ColumnListAuthoritative: true, + }}} + tcases := []struct { + sql string + expSQL string + sameTbl int + otherTbl int + expandedCol int + }{{ + sql: "select a, * from t1", + expSQL: "select a, t1.a as a from t1", + otherTbl: -1, sameTbl: 0, expandedCol: 1, + }, { + sql: "select t2.a, t1.a, t1.* from t1, t2", + expSQL: "select t2.a, t1.a, t1.a as a from t1, t2", + otherTbl: 0, sameTbl: 1, expandedCol: 2, + }, { + sql: "select t2.a, t.a, t.* from t1 t, t2", + expSQL: "select t2.a, t.a, t.a as a from t1 as t, t2", + otherTbl: 0, sameTbl: 1, expandedCol: 2, }} for _, tcase := range tcases { t.Run(tcase.sql, func(t *testing.T) { ast, err := sqlparser.Parse(tcase.sql) require.NoError(t, err) - semState, err := semantics.Analyze(ast, cDB, schemaInfo) + semTable, err := semantics.Analyze(ast, "", schemaInfo) + require.NoError(t, err) + expandedSelect, err := expandStar(ast.(*sqlparser.Select), semTable) require.NoError(t, err) - expanded := expandStar(ast.(*sqlparser.Select), semState) - assert.Equal(t, tcase.expSQL, sqlparser.String(expanded)) + assert.Equal(t, tcase.expSQL, sqlparser.String(expandedSelect)) + if tcase.otherTbl != -1 { + assert.NotEqual(t, + semTable.Dependencies(expandedSelect.SelectExprs[tcase.otherTbl].(*sqlparser.AliasedExpr).Expr), + semTable.Dependencies(expandedSelect.SelectExprs[tcase.expandedCol].(*sqlparser.AliasedExpr).Expr), + ) + } + if tcase.sameTbl != -1 { + assert.Equal(t, + semTable.Dependencies(expandedSelect.SelectExprs[tcase.sameTbl].(*sqlparser.AliasedExpr).Expr), + semTable.Dependencies(expandedSelect.SelectExprs[tcase.expandedCol].(*sqlparser.AliasedExpr).Expr), + ) + } }) } } diff --git a/go/vt/vtgate/semantics/analyzer.go b/go/vt/vtgate/semantics/analyzer.go index f9f70871aef..0594b86309e 100644 --- a/go/vt/vtgate/semantics/analyzer.go +++ b/go/vt/vtgate/semantics/analyzer.go @@ -168,7 +168,7 @@ func (a *analyzer) resolveUnQualifiedColumn(current *scope, expr *sqlparser.ColN var tblInfo *TableInfo for _, tbl := range current.tables { - if !tbl.Table.ColumnListAuthoritative { + if tbl.Table == nil || !tbl.Table.ColumnListAuthoritative { return nil, vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.NonUniqError, fmt.Sprintf("Column '%s' in field list is ambiguous", sqlparser.String(expr))) } for _, col := range tbl.Table.Columns { diff --git a/go/vt/vtgate/semantics/semantic_state.go b/go/vt/vtgate/semantics/semantic_state.go index 7f05ada4e61..648ad473138 100644 --- a/go/vt/vtgate/semantics/semantic_state.go +++ b/go/vt/vtgate/semantics/semantic_state.go @@ -107,6 +107,14 @@ func (st *SemTable) GetSelectTables(node *sqlparser.Select) []*TableInfo { return scope.tables } +// AddExprs adds new select exprs to the SemTable. +func (st *SemTable) AddExprs(tbl *sqlparser.AliasedTableExpr, cols sqlparser.SelectExprs) { + tableSet := st.TableSetFor(tbl) + for _, col := range cols { + st.exprDependencies[col.(*sqlparser.AliasedExpr).Expr] = tableSet + } +} + func newScope(parent *scope) *scope { return &scope{parent: parent} } From f267c165b13bfe812fcb6191d82e424b9ce0cb8d Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 17 Jun 2021 13:15:58 +0200 Subject: [PATCH 7/7] refactor starRewrite a little Signed-off-by: Andres Taylor --- go/vt/vtgate/planbuilder/route_planning.go | 105 +++++++++++---------- 1 file changed, 57 insertions(+), 48 deletions(-) diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index 6c72a76aa69..3caaca20399 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -118,54 +118,9 @@ func (sr *starRewriter) starRewrite(cursor *sqlparser.Cursor) bool { selExprs = append(selExprs, selectExpr) continue } - expStar := &expandStarInfo{ - tblColMap: map[*sqlparser.AliasedTableExpr]sqlparser.SelectExprs{}, - } - var colNames sqlparser.SelectExprs - unknownTbl := true - for _, tbl := range tables { - if !starExpr.TableName.IsEmpty() { - if !tbl.ASTNode.As.IsEmpty() { - if !starExpr.TableName.Qualifier.IsEmpty() { - continue - } - if starExpr.TableName.Name.String() != tbl.ASTNode.As.String() { - continue - } - } else { - if !starExpr.TableName.Qualifier.IsEmpty() { - if starExpr.TableName.Qualifier.String() != tbl.Table.Keyspace.Name { - continue - } - } - tblName := tbl.ASTNode.Expr.(sqlparser.TableName) - if starExpr.TableName.Name.String() != tblName.Name.String() { - continue - } - } - } - unknownTbl = false - if tbl.Table == nil || !tbl.Table.ColumnListAuthoritative { - expStar.proceed = false - break - } - expStar.proceed = true - tblName, err := tbl.ASTNode.TableName() - if err != nil { - sr.err = err - return false - } - for _, col := range tbl.Table.Columns { - colNames = append(colNames, &sqlparser.AliasedExpr{ - Expr: sqlparser.NewColNameWithQualifier(col.Name.String(), tblName), - As: sqlparser.NewColIdent(col.Name.String()), - }) - } - expStar.tblColMap[tbl.ASTNode] = colNames - } - if unknownTbl { - // This will only happen for case when starExpr has qualifier. - sr.err = vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.BadDb, "Unknown table '%s'", sqlparser.String(starExpr.TableName)) + colNames, expStar, err := expandTableColumns(tables, starExpr) + if err != nil { + sr.err = err return false } if !expStar.proceed { @@ -182,6 +137,60 @@ func (sr *starRewriter) starRewrite(cursor *sqlparser.Cursor) bool { return true } +func expandTableColumns(tables []*semantics.TableInfo, starExpr *sqlparser.StarExpr) (sqlparser.SelectExprs, *expandStarInfo, error) { + unknownTbl := true + var colNames sqlparser.SelectExprs + expStar := &expandStarInfo{ + tblColMap: map[*sqlparser.AliasedTableExpr]sqlparser.SelectExprs{}, + } + + for _, tbl := range tables { + if !starExpr.TableName.IsEmpty() { + if !tbl.ASTNode.As.IsEmpty() { + if !starExpr.TableName.Qualifier.IsEmpty() { + continue + } + if starExpr.TableName.Name.String() != tbl.ASTNode.As.String() { + continue + } + } else { + if !starExpr.TableName.Qualifier.IsEmpty() { + if starExpr.TableName.Qualifier.String() != tbl.Table.Keyspace.Name { + continue + } + } + tblName := tbl.ASTNode.Expr.(sqlparser.TableName) + if starExpr.TableName.Name.String() != tblName.Name.String() { + continue + } + } + } + unknownTbl = false + if tbl.Table == nil || !tbl.Table.ColumnListAuthoritative { + expStar.proceed = false + break + } + expStar.proceed = true + tblName, err := tbl.ASTNode.TableName() + if err != nil { + return nil, nil, err + } + for _, col := range tbl.Table.Columns { + colNames = append(colNames, &sqlparser.AliasedExpr{ + Expr: sqlparser.NewColNameWithQualifier(col.Name.String(), tblName), + As: sqlparser.NewColIdent(col.Name.String()), + }) + } + expStar.tblColMap[tbl.ASTNode] = 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 colNames, expStar, nil +} + type expandStarInfo struct { proceed bool tblColMap map[*sqlparser.AliasedTableExpr]sqlparser.SelectExprs