From ce2ee9644ca43e8af3817858373f7161730ff58a Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Wed, 6 Sep 2023 17:29:44 -0400 Subject: [PATCH 01/10] Rewrite USING to ON condition for Joins Signed-off-by: Florent Poinsard --- .../endtoend/vtgate/queries/misc/misc_test.go | 8 +++++ .../planbuilder/testdata/from_cases.json | 22 ++++++++++++++ go/vt/vtgate/semantics/binder.go | 18 +++++++++++- go/vt/vtgate/semantics/early_rewriter.go | 29 +++---------------- 4 files changed, 51 insertions(+), 26 deletions(-) diff --git a/go/test/endtoend/vtgate/queries/misc/misc_test.go b/go/test/endtoend/vtgate/queries/misc/misc_test.go index fe9699b8267..77cb1784c43 100644 --- a/go/test/endtoend/vtgate/queries/misc/misc_test.go +++ b/go/test/endtoend/vtgate/queries/misc/misc_test.go @@ -296,3 +296,11 @@ func TestBuggyOuterJoin(t *testing.T) { mcmp.Exec("insert into t1(id1, id2) values (1,2), (42,5), (5, 42)") mcmp.Exec("select t1.id1, t2.id1 from t1 left join t1 as t2 on t2.id1 = t2.id2") } + +func TestLeftJoinUsingUnsharded(t *testing.T) { + mcmp, closer := start(t) + defer closer() + + utils.Exec(t, mcmp.VtConn, "insert into uks.unsharded(id1) values (1),(2),(3),(4),(5)") + utils.Exec(t, mcmp.VtConn, "select * from uks.unsharded as A left join uks.unsharded as B using(id1)") +} diff --git a/go/vt/vtgate/planbuilder/testdata/from_cases.json b/go/vt/vtgate/planbuilder/testdata/from_cases.json index 46db9519fcc..efa04bfa7ca 100644 --- a/go/vt/vtgate/planbuilder/testdata/from_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/from_cases.json @@ -4017,5 +4017,27 @@ "zlookup_unique.t1" ] } + }, + { + "comment": "left join with using has to be transformed into inner join with on condition", + "query": "SELECT * FROM unsharded_authoritative as A LEFT JOIN unsharded_authoritative as B USING(col1)", + "plan": { + "QueryType": "SELECT", + "Original": "SELECT * FROM unsharded_authoritative as A LEFT JOIN unsharded_authoritative as B USING(col1)", + "Instructions": { + "OperatorType": "Route", + "Variant": "Unsharded", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "FieldQuery": "select A.col1 as col1, A.col2 as col2, B.col2 as col2 from unsharded_authoritative as A left join unsharded_authoritative as B on A.col1 = B.col1 where 1 != 1", + "Query": "select A.col1 as col1, A.col2 as col2, B.col2 as col2 from unsharded_authoritative as A left join unsharded_authoritative as B on A.col1 = B.col1", + "Table": "unsharded_authoritative" + }, + "TablesUsed": [ + "main.unsharded_authoritative" + ] + } } ] diff --git a/go/vt/vtgate/semantics/binder.go b/go/vt/vtgate/semantics/binder.go index c43180d1efa..6269c8d46ae 100644 --- a/go/vt/vtgate/semantics/binder.go +++ b/go/vt/vtgate/semantics/binder.go @@ -85,7 +85,23 @@ func (b *binder) up(cursor *sqlparser.Cursor) error { currScope.joinUsing[ident.Lowered()] = deps.direct } if len(node.Using) > 0 { - err := rewriteJoinUsing(currScope, node.Using, b.org) + err := rewriteJoinUsing(currScope, node, b.org) + if err != nil { + return err + } + err = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { + column, isColumn := node.(*sqlparser.ColName) + if !isColumn { + return true, nil + } + deps, err := b.resolveColumn(column, currScope, false) + if err != nil { + return false, err + } + b.recursive[column] = deps.recursive + b.direct[column] = deps.direct + return true, nil + }, node.On) if err != nil { return err } diff --git a/go/vt/vtgate/semantics/early_rewriter.go b/go/vt/vtgate/semantics/early_rewriter.go index 7dfdbf78247..f0671488738 100644 --- a/go/vt/vtgate/semantics/early_rewriter.go +++ b/go/vt/vtgate/semantics/early_rewriter.go @@ -344,34 +344,13 @@ func rewriteOrFalse(orExpr sqlparser.OrExpr) sqlparser.Expr { // // This function returns an error if it encounters a non-authoritative table or // if it cannot find a SELECT statement to add the WHERE predicate to. -func rewriteJoinUsing( - current *scope, - using sqlparser.Columns, - org originable, -) error { - predicates, err := buildJoinPredicates(current, using, org) +func rewriteJoinUsing(current *scope, cond *sqlparser.JoinCondition, org originable) error { + predicates, err := buildJoinPredicates(current, cond.Using, org) if err != nil { return err } - // now, we go up the scope until we find a SELECT - // with a where clause we can add this predicate to - for current != nil { - sel, found := current.stmt.(*sqlparser.Select) - if !found { - current = current.parent - continue - } - if sel.Where != nil { - predicates = append(predicates, sel.Where.Expr) - sel.Where = nil - } - sel.Where = &sqlparser.Where{ - Type: sqlparser.WhereClause, - Expr: sqlparser.AndExpressions(predicates...), - } - return nil - } - return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "did not find WHERE clause") + cond.On = sqlparser.AndExpressions(predicates...) + return nil } // buildJoinPredicates constructs the join predicates for a given set of USING columns. From 882363e8b687d17919fa7128b25170d5bbb8c278 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Wed, 6 Sep 2023 18:07:29 -0400 Subject: [PATCH 02/10] Fix test results Signed-off-by: Florent Poinsard --- go/vt/vtgate/semantics/early_rewriter_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/go/vt/vtgate/semantics/early_rewriter_test.go b/go/vt/vtgate/semantics/early_rewriter_test.go index f1b16853cfc..dcdd1c71eb7 100644 --- a/go/vt/vtgate/semantics/early_rewriter_test.go +++ b/go/vt/vtgate/semantics/early_rewriter_test.go @@ -146,24 +146,24 @@ func TestExpandStar(t *testing.T) { expSQL: "select t1.a as a, t1.b as b, t1.c as c, t2.c1 as c1, t2.c2 as c2 from t1 join t2 on t1.a = t2.c1", }, { sql: "select * from t2 join t4 using (c1)", - expSQL: "select t2.c1 as c1, t2.c2 as c2, t4.c4 as c4 from t2 join t4 where t2.c1 = t4.c1", + expSQL: "select t2.c1 as c1, t2.c2 as c2, t4.c4 as c4 from t2 join t4 on t2.c1 = t4.c1", expanded: "main.t2.c1, main.t2.c2, main.t4.c4", }, { sql: "select * from t2 join t4 using (c1) join t2 as X using (c1)", - expSQL: "select t2.c1 as c1, t2.c2 as c2, t4.c4 as c4, X.c2 as c2 from t2 join t4 join t2 as X where t2.c1 = t4.c1 and t2.c1 = X.c1 and t4.c1 = X.c1", + expSQL: "select t2.c1 as c1, t2.c2 as c2, t4.c4 as c4, X.c2 as c2 from t2 join t4 on t2.c1 = t4.c1 join t2 as X on t2.c1 = t4.c1 and t2.c1 = X.c1 and t4.c1 = X.c1", }, { sql: "select * from t2 join t4 using (c1), t2 as t2b join t4 as t4b using (c1)", - expSQL: "select t2.c1 as c1, t2.c2 as c2, t4.c4 as c4, t2b.c1 as c1, t2b.c2 as c2, t4b.c4 as c4 from t2 join t4, t2 as t2b join t4 as t4b where t2b.c1 = t4b.c1 and t2.c1 = t4.c1", + expSQL: "select t2.c1 as c1, t2.c2 as c2, t4.c4 as c4, t2b.c1 as c1, t2b.c2 as c2, t4b.c4 as c4 from t2 join t4 on t2.c1 = t4.c1, t2 as t2b join t4 as t4b on t2b.c1 = t4b.c1", }, { sql: "select * from t1 join t5 using (b)", - expSQL: "select t1.b as b, t1.a as a, t1.c as c, t5.a as a from t1 join t5 where t1.b = t5.b", + expSQL: "select t1.b as b, t1.a as a, t1.c as c, t5.a as a from t1 join t5 on t1.b = t5.b", expanded: "main.t1.a, main.t1.b, main.t1.c, main.t5.a", }, { sql: "select * from t1 join t5 using (b) having b = 12", - expSQL: "select t1.b as b, t1.a as a, t1.c as c, t5.a as a from t1 join t5 where t1.b = t5.b having b = 12", + expSQL: "select t1.b as b, t1.a as a, t1.c as c, t5.a as a from t1 join t5 on t1.b = t5.b having b = 12", }, { sql: "select 1 from t1 join t5 using (b) having b = 12", - expSQL: "select 1 from t1 join t5 where t1.b = t5.b having t1.b = 12", + expSQL: "select 1 from t1 join t5 on t1.b = t5.b having t1.b = 12", }, { sql: "select * from (select 12) as t", expSQL: "select t.`12` from (select 12 from dual) as t", @@ -265,7 +265,7 @@ func TestRewriteJoinUsingColumns(t *testing.T) { expErr string }{{ sql: "select 1 from t1 join t2 using (a) where a = 42", - expSQL: "select 1 from t1 join t2 where t1.a = t2.a and t1.a = 42", + expSQL: "select 1 from t1 join t2 on t1.a = t2.a where t1.a = 42", }, { sql: "select 1 from t1 join t2 using (a), t3 where a = 42", expErr: "Column 'a' in field list is ambiguous", From 4faa0def6d3a1a973dfc0df161403c82062dad84 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Thu, 7 Sep 2023 09:25:53 -0400 Subject: [PATCH 03/10] add more test cases to early rewritter Signed-off-by: Florent Poinsard --- go/vt/vtgate/semantics/early_rewriter_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/go/vt/vtgate/semantics/early_rewriter_test.go b/go/vt/vtgate/semantics/early_rewriter_test.go index dcdd1c71eb7..fe830e3406f 100644 --- a/go/vt/vtgate/semantics/early_rewriter_test.go +++ b/go/vt/vtgate/semantics/early_rewriter_test.go @@ -144,6 +144,12 @@ func TestExpandStar(t *testing.T) { }, { sql: "select * from t1 join t2 on t1.a = t2.c1", expSQL: "select t1.a as a, t1.b as b, t1.c as c, t2.c1 as c1, t2.c2 as c2 from t1 join t2 on t1.a = t2.c1", + }, { + sql: "select * from t1 left join t2 on t1.a = t2.c1", + expSQL: "select t1.a as a, t1.b as b, t1.c as c, t2.c1 as c1, t2.c2 as c2 from t1 left join t2 on t1.a = t2.c1", + }, { + sql: "select * from t1 right join t2 on t1.a = t2.c1", + expSQL: "select t1.a as a, t1.b as b, t1.c as c, t2.c1 as c1, t2.c2 as c2 from t1 right join t2 on t1.a = t2.c1", }, { sql: "select * from t2 join t4 using (c1)", expSQL: "select t2.c1 as c1, t2.c2 as c2, t4.c4 as c4 from t2 join t4 on t2.c1 = t4.c1", From 9b0cf5c6de210959f6d09c8c377236753ca1c8f6 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Thu, 7 Sep 2023 16:28:24 -0400 Subject: [PATCH 04/10] enhance table name detection in join rewritting Signed-off-by: Florent Poinsard --- .../testdata/unsupported_cases.json | 4 +- go/vt/vtgate/semantics/binder.go | 49 ++++---- go/vt/vtgate/semantics/early_rewriter.go | 105 ++++++++++++------ 3 files changed, 99 insertions(+), 59 deletions(-) diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json index e1d07bc58e3..f6982418d53 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json @@ -37,12 +37,12 @@ { "comment": "join with USING construct", "query": "select * from user join user_extra using(id)", - "plan": "can't handle JOIN USING without authoritative tables" + "plan": "VT03019: column id not found" }, { "comment": "join with USING construct with 3 tables", "query": "select user.id from user join user_extra using(id) join music using(id2)", - "plan": "can't handle JOIN USING without authoritative tables" + "plan": "VT03019: column id2 not found" }, { "comment": "natural left join", diff --git a/go/vt/vtgate/semantics/binder.go b/go/vt/vtgate/semantics/binder.go index 6269c8d46ae..0fc30260970 100644 --- a/go/vt/vtgate/semantics/binder.go +++ b/go/vt/vtgate/semantics/binder.go @@ -74,6 +74,32 @@ func (b *binder) up(cursor *sqlparser.Cursor) error { b.subqueryRef[node] = sq b.setSubQueryDependencies(node, currScope) + case *sqlparser.JoinTableExpr: + currScope := b.scoper.currentScope() + if len(node.Condition.Using) == 0 { + return nil + } + err := rewriteJoinUsing(b, node) + if err != nil { + return err + } + err = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { + column, isColumn := node.(*sqlparser.ColName) + if !isColumn { + return true, nil + } + deps, err := b.resolveColumn(column, currScope, false) + if err != nil { + return false, err + } + b.recursive[column] = deps.recursive + b.direct[column] = deps.direct + return true, nil + }, node.Condition.On) + if err != nil { + return err + } + node.Condition.Using = nil case *sqlparser.JoinCondition: currScope := b.scoper.currentScope() for _, ident := range node.Using { @@ -84,29 +110,6 @@ func (b *binder) up(cursor *sqlparser.Cursor) error { } currScope.joinUsing[ident.Lowered()] = deps.direct } - if len(node.Using) > 0 { - err := rewriteJoinUsing(currScope, node, b.org) - if err != nil { - return err - } - err = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { - column, isColumn := node.(*sqlparser.ColName) - if !isColumn { - return true, nil - } - deps, err := b.resolveColumn(column, currScope, false) - if err != nil { - return false, err - } - b.recursive[column] = deps.recursive - b.direct[column] = deps.direct - return true, nil - }, node.On) - if err != nil { - return err - } - node.Using = nil - } case *sqlparser.ColName: currentScope := b.scoper.currentScope() deps, err := b.resolveColumn(node, currentScope, false) diff --git a/go/vt/vtgate/semantics/early_rewriter.go b/go/vt/vtgate/semantics/early_rewriter.go index f0671488738..c652db7116e 100644 --- a/go/vt/vtgate/semantics/early_rewriter.go +++ b/go/vt/vtgate/semantics/early_rewriter.go @@ -17,8 +17,8 @@ limitations under the License. package semantics import ( + "fmt" "strconv" - "strings" "vitess.io/vitess/go/mysql/collations" "vitess.io/vitess/go/vt/vtgate/evalengine" @@ -344,23 +344,25 @@ func rewriteOrFalse(orExpr sqlparser.OrExpr) sqlparser.Expr { // // This function returns an error if it encounters a non-authoritative table or // if it cannot find a SELECT statement to add the WHERE predicate to. -func rewriteJoinUsing(current *scope, cond *sqlparser.JoinCondition, org originable) error { - predicates, err := buildJoinPredicates(current, cond.Using, org) +func rewriteJoinUsing(b *binder, join *sqlparser.JoinTableExpr) error { + predicates, err := buildJoinPredicates(b, join) if err != nil { return err } - cond.On = sqlparser.AndExpressions(predicates...) + if len(predicates) > 0 { + join.Condition.On = sqlparser.AndExpressions(predicates...) + join.Condition.Using = nil + } return nil } // buildJoinPredicates constructs the join predicates for a given set of USING columns. // It returns a slice of sqlparser.Expr, each representing a join predicate for the given columns. -func buildJoinPredicates(current *scope, using sqlparser.Columns, org originable) ([]sqlparser.Expr, error) { - joinUsing := current.prepareUsingMap() +func buildJoinPredicates(b *binder, join *sqlparser.JoinTableExpr) ([]sqlparser.Expr, error) { var predicates []sqlparser.Expr - for _, column := range using { - foundTables, err := findTablesWithColumn(current, joinUsing, org, column) + for _, column := range join.Condition.Using { + foundTables, err := findTablesWithColumn(b, join, column) if err != nil { return nil, err } @@ -371,42 +373,77 @@ func buildJoinPredicates(current *scope, using sqlparser.Columns, org originable return predicates, nil } -// findTablesWithColumn finds the tables with the specified column in the current scope. -func findTablesWithColumn(current *scope, joinUsing map[TableSet]map[string]TableSet, org originable, column sqlparser.IdentifierCI) ([]sqlparser.TableName, error) { - var foundTables []sqlparser.TableName - - for _, tbl := range current.tables { - if !tbl.authoritative() { - return nil, vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "can't handle JOIN USING without authoritative tables") +func findOnlyOneTableInfoThatHasColumn(b *binder, tbl sqlparser.TableExpr, column sqlparser.IdentifierCI) (TableInfo, error) { + switch tbl := tbl.(type) { + case *sqlparser.AliasedTableExpr: + ts := b.tc.tableSetFor(tbl) + tblInfo := b.tc.Tables[ts.TableOffset()] + for _, info := range tblInfo.getColumns() { + if column.EqualString(info.Name) { + return tblInfo, nil + } } - - currTable := tbl.getTableSet(org) - usingCols := joinUsing[currTable] - if usingCols == nil { - usingCols = map[string]TableSet{} + return nil, nil + case *sqlparser.JoinTableExpr: + tblInfoR, err := findOnlyOneTableInfoThatHasColumn(b, tbl.RightExpr, column) + if err != nil { + return nil, err } - - if hasColumnInTable(tbl, usingCols) { - tblName, err := tbl.Name() + tblInfoL, err := findOnlyOneTableInfoThatHasColumn(b, tbl.LeftExpr, column) + if err != nil { + return nil, err + } + if tblInfoL != nil && tblInfoR != nil { + return nil, vterrors.VT03021(column.String()) + } + if tblInfoR != nil { + return tblInfoR, nil + } + return tblInfoL, nil + case *sqlparser.ParenTableExpr: + var tblInfo TableInfo + for _, parenTable := range tbl.Exprs { + newTblInfo, err := findOnlyOneTableInfoThatHasColumn(b, parenTable, column) if err != nil { return nil, err } - foundTables = append(foundTables, tblName) + if tblInfo != nil && newTblInfo != nil { + return nil, vterrors.VT03021(column.String()) + } + if newTblInfo != nil { + tblInfo = newTblInfo + } } + return tblInfo, nil + default: + panic(fmt.Sprintf("unsupported TableExpr type in JOIN: %T", tbl)) } - - return foundTables, nil } -// hasColumnInTable checks if the specified table has the given column. -func hasColumnInTable(tbl TableInfo, usingCols map[string]TableSet) bool { - for _, col := range tbl.getColumns() { - _, found := usingCols[strings.ToLower(col.Name)] - if found { - return true - } +// findTablesWithColumn finds the tables with the specified column in the current scope. +func findTablesWithColumn(b *binder, join *sqlparser.JoinTableExpr, column sqlparser.IdentifierCI) ([]sqlparser.TableName, error) { + leftTableInfo, err := findOnlyOneTableInfoThatHasColumn(b, join.LeftExpr, column) + if err != nil { + return nil, err + } + + rightTableInfo, err := findOnlyOneTableInfoThatHasColumn(b, join.RightExpr, column) + if err != nil { + return nil, err + } + + if leftTableInfo == nil || rightTableInfo == nil { + return nil, ShardedError{Inner: vterrors.VT03019(column.String())} + } + leftTableName, err := leftTableInfo.Name() + if err != nil { + return nil, err + } + rightTableName, err := rightTableInfo.Name() + if err != nil { + return nil, err } - return false + return []sqlparser.TableName{leftTableName, rightTableName}, nil } // createComparisonPredicates creates a list of comparison predicates between the given column and foundTables. From 136e2d49d461adbec728d942d6db9133fc03541a Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Thu, 7 Sep 2023 16:34:36 -0400 Subject: [PATCH 05/10] move all join table expr rewriting to the earlyRewrite Signed-off-by: Florent Poinsard --- go/vt/vtgate/semantics/analyzer.go | 5 ++++ go/vt/vtgate/semantics/binder.go | 26 ------------------- go/vt/vtgate/semantics/early_rewriter.go | 32 ++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 26 deletions(-) diff --git a/go/vt/vtgate/semantics/analyzer.go b/go/vt/vtgate/semantics/analyzer.go index 5b560ec7075..6955f4bafcd 100644 --- a/go/vt/vtgate/semantics/analyzer.go +++ b/go/vt/vtgate/semantics/analyzer.go @@ -192,6 +192,11 @@ func (a *analyzer) analyzeUp(cursor *sqlparser.Cursor) bool { return false } + if err := a.rewriter.up(cursor); err != nil { + a.setError(err) + return true + } + a.leaveProjection(cursor) return a.shouldContinue() } diff --git a/go/vt/vtgate/semantics/binder.go b/go/vt/vtgate/semantics/binder.go index 0fc30260970..d467a97c130 100644 --- a/go/vt/vtgate/semantics/binder.go +++ b/go/vt/vtgate/semantics/binder.go @@ -74,32 +74,6 @@ func (b *binder) up(cursor *sqlparser.Cursor) error { b.subqueryRef[node] = sq b.setSubQueryDependencies(node, currScope) - case *sqlparser.JoinTableExpr: - currScope := b.scoper.currentScope() - if len(node.Condition.Using) == 0 { - return nil - } - err := rewriteJoinUsing(b, node) - if err != nil { - return err - } - err = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { - column, isColumn := node.(*sqlparser.ColName) - if !isColumn { - return true, nil - } - deps, err := b.resolveColumn(column, currScope, false) - if err != nil { - return false, err - } - b.recursive[column] = deps.recursive - b.direct[column] = deps.direct - return true, nil - }, node.Condition.On) - if err != nil { - return err - } - node.Condition.Using = nil case *sqlparser.JoinCondition: currScope := b.scoper.currentScope() for _, ident := range node.Using { diff --git a/go/vt/vtgate/semantics/early_rewriter.go b/go/vt/vtgate/semantics/early_rewriter.go index c652db7116e..93835dcf4c1 100644 --- a/go/vt/vtgate/semantics/early_rewriter.go +++ b/go/vt/vtgate/semantics/early_rewriter.go @@ -60,6 +60,38 @@ func (r *earlyRewriter) down(cursor *sqlparser.Cursor) error { return nil } +func (r *earlyRewriter) up(cursor *sqlparser.Cursor) error { + switch node := cursor.Node().(type) { + case *sqlparser.JoinTableExpr: + currScope := r.binder.scoper.currentScope() + if len(node.Condition.Using) == 0 { + return nil + } + err := rewriteJoinUsing(r.binder, node) + if err != nil { + return err + } + err = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { + column, isColumn := node.(*sqlparser.ColName) + if !isColumn { + return true, nil + } + deps, err := r.binder.resolveColumn(column, currScope, false) + if err != nil { + return false, err + } + r.binder.recursive[column] = deps.recursive + r.binder.direct[column] = deps.direct + return true, nil + }, node.Condition.On) + if err != nil { + return err + } + node.Condition.Using = nil + } + return nil +} + // handleWhereClause processes WHERE clauses, specifically the HAVING clause. func handleWhereClause(node *sqlparser.Where, parent sqlparser.SQLNode) { if node.Type != sqlparser.HavingClause { From 56f5c61cc399f0de1cf0115e3a67552947ef3da6 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Thu, 7 Sep 2023 17:11:15 -0400 Subject: [PATCH 06/10] Fix TestViewReferences test Signed-off-by: Florent Poinsard --- go/vt/schemadiff/schema_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/vt/schemadiff/schema_test.go b/go/vt/schemadiff/schema_test.go index 06139db3b8b..79bf44117e2 100644 --- a/go/vt/schemadiff/schema_test.go +++ b/go/vt/schemadiff/schema_test.go @@ -631,7 +631,7 @@ func TestViewReferences(t *testing.T) { "create table t2(id int primary key, n int, info int)", "create view v1 as select id, c as ch from t1 where id > 0", "create view v2 as select n as num, info from t2", - "create view v3 as select num, v1.id, ch from v1 join v2 using (id) where info > 5", + "create view v3 as select num, v1.id, ch from v1 join v2 on v1.id = v2.num where info > 5", }, }, { From f4c7a19df6b4fc18fd7c545a76b4c91b689ec10c Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Thu, 7 Sep 2023 18:28:17 -0400 Subject: [PATCH 07/10] allow column coming from more than one table as this is a ON condition and not USING Signed-off-by: Florent Poinsard --- go/vt/vtgate/semantics/early_rewriter.go | 36 +++++++++++++----------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/go/vt/vtgate/semantics/early_rewriter.go b/go/vt/vtgate/semantics/early_rewriter.go index 93835dcf4c1..2c73f0b9989 100644 --- a/go/vt/vtgate/semantics/early_rewriter.go +++ b/go/vt/vtgate/semantics/early_rewriter.go @@ -405,14 +405,14 @@ func buildJoinPredicates(b *binder, join *sqlparser.JoinTableExpr) ([]sqlparser. return predicates, nil } -func findOnlyOneTableInfoThatHasColumn(b *binder, tbl sqlparser.TableExpr, column sqlparser.IdentifierCI) (TableInfo, error) { +func findOnlyOneTableInfoThatHasColumn(b *binder, tbl sqlparser.TableExpr, column sqlparser.IdentifierCI) ([]TableInfo, error) { switch tbl := tbl.(type) { case *sqlparser.AliasedTableExpr: ts := b.tc.tableSetFor(tbl) tblInfo := b.tc.Tables[ts.TableOffset()] for _, info := range tblInfo.getColumns() { if column.EqualString(info.Name) { - return tblInfo, nil + return []TableInfo{tblInfo}, nil } } return nil, nil @@ -425,15 +425,10 @@ func findOnlyOneTableInfoThatHasColumn(b *binder, tbl sqlparser.TableExpr, colum if err != nil { return nil, err } - if tblInfoL != nil && tblInfoR != nil { - return nil, vterrors.VT03021(column.String()) - } - if tblInfoR != nil { - return tblInfoR, nil - } - return tblInfoL, nil + + return append(tblInfoL, tblInfoR...), nil case *sqlparser.ParenTableExpr: - var tblInfo TableInfo + var tblInfo []TableInfo for _, parenTable := range tbl.Exprs { newTblInfo, err := findOnlyOneTableInfoThatHasColumn(b, parenTable, column) if err != nil { @@ -467,15 +462,22 @@ func findTablesWithColumn(b *binder, join *sqlparser.JoinTableExpr, column sqlpa if leftTableInfo == nil || rightTableInfo == nil { return nil, ShardedError{Inner: vterrors.VT03019(column.String())} } - leftTableName, err := leftTableInfo.Name() - if err != nil { - return nil, err + var tableNames []sqlparser.TableName + for _, info := range leftTableInfo { + nm, err := info.Name() + if err != nil { + return nil, err + } + tableNames = append(tableNames, nm) } - rightTableName, err := rightTableInfo.Name() - if err != nil { - return nil, err + for _, info := range rightTableInfo { + nm, err := info.Name() + if err != nil { + return nil, err + } + tableNames = append(tableNames, nm) } - return []sqlparser.TableName{leftTableName, rightTableName}, nil + return tableNames, nil } // createComparisonPredicates creates a list of comparison predicates between the given column and foundTables. From b992007459f5d820da68971cbb946df5a58419a2 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Fri, 8 Sep 2023 09:08:56 +0200 Subject: [PATCH 08/10] minor refactoring Signed-off-by: Andres Taylor --- go/vt/vtgate/semantics/early_rewriter.go | 51 +++++++++---------- go/vt/vtgate/semantics/early_rewriter_test.go | 3 ++ 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/go/vt/vtgate/semantics/early_rewriter.go b/go/vt/vtgate/semantics/early_rewriter.go index 2c73f0b9989..33bad7daa6f 100644 --- a/go/vt/vtgate/semantics/early_rewriter.go +++ b/go/vt/vtgate/semantics/early_rewriter.go @@ -61,35 +61,30 @@ func (r *earlyRewriter) down(cursor *sqlparser.Cursor) error { } func (r *earlyRewriter) up(cursor *sqlparser.Cursor) error { - switch node := cursor.Node().(type) { - case *sqlparser.JoinTableExpr: - currScope := r.binder.scoper.currentScope() - if len(node.Condition.Using) == 0 { - return nil - } - err := rewriteJoinUsing(r.binder, node) - if err != nil { - return err - } - err = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { - column, isColumn := node.(*sqlparser.ColName) - if !isColumn { - return true, nil - } - deps, err := r.binder.resolveColumn(column, currScope, false) - if err != nil { - return false, err - } - r.binder.recursive[column] = deps.recursive - r.binder.direct[column] = deps.direct - return true, nil - }, node.Condition.On) - if err != nil { - return err - } - node.Condition.Using = nil + // this rewriting is done in the `up` phase, because we need the scope to have been + // filled in with the available tables + node, ok := cursor.Node().(*sqlparser.JoinTableExpr) + if !ok || len(node.Condition.Using) == 0 { + return nil } - return nil + + err := rewriteJoinUsing(r.binder, node) + if err != nil { + return err + } + + // since the binder has already been over the join, we need to invoke it again so it + // can bind columns to the right tables + sqlparser.Rewrite(node.Condition.On, nil, func(cursor *sqlparser.Cursor) bool { + innerErr := r.binder.up(cursor) + if innerErr == nil { + return true + } + + err = innerErr + return false + }) + return err } // handleWhereClause processes WHERE clauses, specifically the HAVING clause. diff --git a/go/vt/vtgate/semantics/early_rewriter_test.go b/go/vt/vtgate/semantics/early_rewriter_test.go index fe830e3406f..2846bfd9366 100644 --- a/go/vt/vtgate/semantics/early_rewriter_test.go +++ b/go/vt/vtgate/semantics/early_rewriter_test.go @@ -278,6 +278,9 @@ func TestRewriteJoinUsingColumns(t *testing.T) { }, { sql: "select 1 from t1 join t2 using (a), t1 as b join t3 on (a) where a = 42", expErr: "Column 'a' in field list is ambiguous", + }, { + sql: "select 1 from t1 left join t2 using (a) where a = 42", + expSQL: "select 1 from t1 left join t2 on t1.a = t2.a where t1.a = 42", }} for _, tcase := range tcases { t.Run(tcase.sql, func(t *testing.T) { From a07c5dfbc054ce55cd7fa6710c56ec34b532c4b5 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Fri, 8 Sep 2023 08:19:47 -0400 Subject: [PATCH 09/10] edit error message if the column has not been found Signed-off-by: Florent Poinsard --- go/vt/vtgate/planbuilder/testdata/unsupported_cases.json | 4 ++-- go/vt/vtgate/semantics/early_rewriter.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json index f6982418d53..89d1db0ffa1 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json @@ -37,12 +37,12 @@ { "comment": "join with USING construct", "query": "select * from user join user_extra using(id)", - "plan": "VT03019: column id not found" + "plan": "column id not found, can't handle JOIN USING without authoritative tables" }, { "comment": "join with USING construct with 3 tables", "query": "select user.id from user join user_extra using(id) join music using(id2)", - "plan": "VT03019: column id2 not found" + "plan": "column id2 not found, can't handle JOIN USING without authoritative tables" }, { "comment": "natural left join", diff --git a/go/vt/vtgate/semantics/early_rewriter.go b/go/vt/vtgate/semantics/early_rewriter.go index 33bad7daa6f..41252373c50 100644 --- a/go/vt/vtgate/semantics/early_rewriter.go +++ b/go/vt/vtgate/semantics/early_rewriter.go @@ -455,7 +455,7 @@ func findTablesWithColumn(b *binder, join *sqlparser.JoinTableExpr, column sqlpa } if leftTableInfo == nil || rightTableInfo == nil { - return nil, ShardedError{Inner: vterrors.VT03019(column.String())} + return nil, ShardedError{Inner: fmt.Errorf("column %s not found, can't handle JOIN USING without authoritative tables", column.String())} } var tableNames []sqlparser.TableName for _, info := range leftTableInfo { From 19b553618594740418d8c8998307c8be492f402a Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Fri, 8 Sep 2023 08:27:26 -0400 Subject: [PATCH 10/10] use predefined error messages when schema tracking is needed Signed-off-by: Florent Poinsard --- go/vt/vtgate/planbuilder/testdata/unsupported_cases.json | 4 ++-- go/vt/vtgate/semantics/early_rewriter.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json index 89d1db0ffa1..9919b600b23 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json @@ -37,12 +37,12 @@ { "comment": "join with USING construct", "query": "select * from user join user_extra using(id)", - "plan": "column id not found, can't handle JOIN USING without authoritative tables" + "plan": "VT09015: schema tracking required" }, { "comment": "join with USING construct with 3 tables", "query": "select user.id from user join user_extra using(id) join music using(id2)", - "plan": "column id2 not found, can't handle JOIN USING without authoritative tables" + "plan": "VT09015: schema tracking required" }, { "comment": "natural left join", diff --git a/go/vt/vtgate/semantics/early_rewriter.go b/go/vt/vtgate/semantics/early_rewriter.go index 41252373c50..ca1ebc6d2f4 100644 --- a/go/vt/vtgate/semantics/early_rewriter.go +++ b/go/vt/vtgate/semantics/early_rewriter.go @@ -455,7 +455,7 @@ func findTablesWithColumn(b *binder, join *sqlparser.JoinTableExpr, column sqlpa } if leftTableInfo == nil || rightTableInfo == nil { - return nil, ShardedError{Inner: fmt.Errorf("column %s not found, can't handle JOIN USING without authoritative tables", column.String())} + return nil, ShardedError{Inner: vterrors.VT09015()} } var tableNames []sqlparser.TableName for _, info := range leftTableInfo {