From 15afd047c8b8bbebaa3cecb2b5f4d27658e2ff6b Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Thu, 24 Jun 2021 07:24:57 +0200 Subject: [PATCH 01/12] Marked some plan tests as Gen4 plan same as above Signed-off-by: Florent Poinsard --- go/vt/vtgate/planbuilder/testdata/other_admin_cases.txt | 3 +++ go/vt/vtgate/planbuilder/testdata/other_read_cases.txt | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/go/vt/vtgate/planbuilder/testdata/other_admin_cases.txt b/go/vt/vtgate/planbuilder/testdata/other_admin_cases.txt index 583ee3d0424..e5f965ee1b6 100644 --- a/go/vt/vtgate/planbuilder/testdata/other_admin_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/other_admin_cases.txt @@ -14,6 +14,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # Optimize statement "optimize table t1" @@ -31,6 +32,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # DO statement "DO 1" @@ -48,3 +50,4 @@ "SingleShardOnly": true } } +Gen4 plan same as above diff --git a/go/vt/vtgate/planbuilder/testdata/other_read_cases.txt b/go/vt/vtgate/planbuilder/testdata/other_read_cases.txt index 835377526b4..4b2a5bfae9c 100644 --- a/go/vt/vtgate/planbuilder/testdata/other_read_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/other_read_cases.txt @@ -14,6 +14,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # Explain Vitess statement "explain format=vitess select * from user" @@ -24,6 +25,7 @@ "OperatorType": "Rows" } } +Gen4 plan same as above # Analyze statement "analyze table t1" @@ -41,6 +43,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # Describe statement "describe select * from t" @@ -58,6 +61,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # Desc statement "desc select * from t" @@ -75,3 +79,4 @@ "SingleShardOnly": true } } +Gen4 plan same as above From a90d88bbb1edf8e289ff004304a2339c793078a8 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Thu, 24 Jun 2021 07:33:50 +0200 Subject: [PATCH 02/12] Stop semantic analysis on first error Signed-off-by: Florent Poinsard --- go/vt/vtgate/semantics/analyzer.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/go/vt/vtgate/semantics/analyzer.go b/go/vt/vtgate/semantics/analyzer.go index 8b4993dc1e3..e85692be1bb 100644 --- a/go/vt/vtgate/semantics/analyzer.go +++ b/go/vt/vtgate/semantics/analyzer.go @@ -63,6 +63,11 @@ func Analyze(statement sqlparser.Statement, currentDb string, si SchemaInformati // analyzeDown pushes new scopes when we encounter sub queries, // and resolves the table a column is using func (a *analyzer) analyzeDown(cursor *sqlparser.Cursor) bool { + // If we have an error we keep on going down the tree without checking for anything else + // this way we can abort when we come back up. + if !a.shouldContinue() { + return true + } current := a.currentScope() n := cursor.Node() switch node := n.(type) { @@ -224,6 +229,9 @@ func (a *analyzer) analyze(statement sqlparser.Statement) error { } func (a *analyzer) analyzeUp(cursor *sqlparser.Cursor) bool { + if !a.shouldContinue() { + return false + } switch cursor.Node().(type) { case *sqlparser.Union, *sqlparser.Select: a.popScope() From fe89658574503b06b1e8420f85a518ea520adf6e Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Thu, 24 Jun 2021 08:20:53 +0200 Subject: [PATCH 03/12] Plan test now allowing and checking Gen4's errors Signed-off-by: Florent Poinsard --- go/vt/sqlparser/ast.go | 21 ++++++++++--------- go/vt/vtgate/planbuilder/plan_test.go | 14 +++++++++---- .../ddl_cases_no_default_keyspace.txt | 1 + 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/go/vt/sqlparser/ast.go b/go/vt/sqlparser/ast.go index 35160dbd3c2..ee4682a04e7 100644 --- a/go/vt/sqlparser/ast.go +++ b/go/vt/sqlparser/ast.go @@ -201,16 +201,17 @@ type ( Distinct bool StraightJoinHint bool SQLCalcFoundRows bool - From []TableExpr - Comments Comments - SelectExprs SelectExprs - Where *Where - GroupBy GroupBy - Having *Where - OrderBy OrderBy - Limit *Limit - Lock Lock - Into *SelectInto + // The From field must be the first AST element of this struct so the rewriter sees it first + From []TableExpr + Comments Comments + SelectExprs SelectExprs + Where *Where + GroupBy GroupBy + Having *Where + OrderBy OrderBy + Limit *Limit + Lock Lock + Into *SelectInto } // SelectInto is a struct that represent the INTO part of a select query diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index bb4978f424b..7d771dd412a 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -150,7 +150,10 @@ func init() { vindexes.Register("costly", newCostlyIndex) } -const samePlanMarker = "Gen4 plan same as above\n" +const ( + samePlanMarker = "Gen4 plan same as above\n" + gen4ErrorPrefix = "Gen4 error: " +) func TestPlan(t *testing.T) { vschemaWrapper := &vschemaWrapper{ @@ -607,9 +610,11 @@ func iterateExecFile(name string) (testCaseIterator chan testCase) { if err != nil && err != io.EOF { panic(fmt.Sprintf("error reading file %s line# %d: %s", name, lineno, err.Error())) } - if len(binput) > 0 && string(binput) == samePlanMarker { + nextLine := string(binput) + switch { + case nextLine == samePlanMarker: output2Planner = output - } else if len(binput) > 0 && (binput[0] == '"' || binput[0] == '{') { + case strings.HasPrefix(nextLine, "{"): output2Planner = append(output2Planner, binput...) for { l, err := r.ReadBytes('\n') @@ -627,8 +632,9 @@ func iterateExecFile(name string) (testCaseIterator chan testCase) { break } } + case strings.HasPrefix(nextLine, gen4ErrorPrefix): + output2Planner = []byte(nextLine[len(gen4ErrorPrefix) : len(nextLine)-1]) } - testCaseIterator <- testCase{ file: name, lineno: lineno, diff --git a/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.txt b/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.txt index 50afcc55b80..8850fd8799f 100644 --- a/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.txt +++ b/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.txt @@ -225,6 +225,7 @@ Gen4 plan same as above "Query": "create view view_a as select col from `user` join user_extra on `user`.id = user_extra.user_id" } } +Gen4 error: Column 'col' in field list is ambiguous # create view with join that can be solved in each shard separately "create view user.view_a as select user.id from user join user_extra on user.id = user_extra.user_id" From 9763beef0d53aa3b5bc785dd1465cb7e23736766 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Thu, 24 Jun 2021 10:05:11 +0200 Subject: [PATCH 04/12] Support for Composite IN in Gen4 Signed-off-by: Florent Poinsard --- go/vt/vtgate/planbuilder/jointree.go | 66 +++++++++++++++++-- .../planbuilder/jointree_transformers.go | 5 +- .../planbuilder/testdata/filter_cases.txt | 5 ++ 3 files changed, 69 insertions(+), 7 deletions(-) diff --git a/go/vt/vtgate/planbuilder/jointree.go b/go/vt/vtgate/planbuilder/jointree.go index 49295e96c40..3e26b2f1dad 100644 --- a/go/vt/vtgate/planbuilder/jointree.go +++ b/go/vt/vtgate/planbuilder/jointree.go @@ -355,11 +355,7 @@ func (rp *routePlan) planEqualOp(node *sqlparser.ComparisonExpr) (bool, error) { return rp.haveMatchingVindex(node, column, *val, equalOrEqualUnique, justTheVindex), err } -func (rp *routePlan) planInOp(node *sqlparser.ComparisonExpr) (bool, error) { - column, ok := node.Left.(*sqlparser.ColName) - if !ok { - return false, nil - } +func (rp *routePlan) planSimpleInOp(node *sqlparser.ComparisonExpr, left *sqlparser.ColName) (bool, error) { value, err := sqlparser.NewPlanValue(node.Right) if err != nil { // if we are unable to create a PlanValue, we can't use a vindex, but we don't have to fail @@ -377,7 +373,54 @@ func (rp *routePlan) planInOp(node *sqlparser.ComparisonExpr) (bool, error) { } } opcode := func(*vindexes.ColumnVindex) engine.RouteOpcode { return engine.SelectIN } - return rp.haveMatchingVindex(node, column, value, opcode, justTheVindex), err + return rp.haveMatchingVindex(node, left, value, opcode, justTheVindex), err +} + +func (rp *routePlan) planCompositeInOp(node *sqlparser.ComparisonExpr, left sqlparser.ValTuple) (bool, error) { + right, rightIsValTuple := node.Right.(sqlparser.ValTuple) + if !rightIsValTuple { + return false, nil + } + + for i, expr := range left { + if col, isColName := expr.(*sqlparser.ColName); isColName { + // check if left col is a vindex + if !rp.hasVindex(col) { + continue + } + + rightVals := make(sqlparser.ValTuple, len(right)) + for j, currRight := range right { + switch currRight := currRight.(type) { + case sqlparser.ValTuple: + rightVals[j] = currRight[i] + default: + return false, nil + } + } + newPlanValues, err := makePlanValue(rightVals) + if newPlanValues == nil || err != nil { + return false, err + } + + opcode := func(*vindexes.ColumnVindex) engine.RouteOpcode { return engine.SelectMultiEqual } + newVindex := rp.haveMatchingVindex(node, col, *newPlanValues, opcode, justTheVindex) + if newVindex { + return true, nil + } + } + } + return false, nil +} + +func (rp *routePlan) planInOp(node *sqlparser.ComparisonExpr) (bool, error) { + switch left := node.Left.(type) { + case *sqlparser.ColName: + return rp.planSimpleInOp(node, left) + case sqlparser.ValTuple: + return rp.planCompositeInOp(node, left) + } + return false, nil } func (rp *routePlan) planLikeOp(node *sqlparser.ComparisonExpr) (bool, error) { @@ -434,6 +477,17 @@ func makePlanValue(n sqlparser.Expr) (*sqltypes.PlanValue, error) { return &value, nil } +func (rp routePlan) hasVindex(column *sqlparser.ColName) bool { + for _, v := range rp.vindexPreds { + for _, col := range v.colVindex.Columns { + if column.Name.Equal(col) { + return true + } + } + } + return false +} + func (rp *routePlan) haveMatchingVindex( node sqlparser.Expr, column *sqlparser.ColName, diff --git a/go/vt/vtgate/planbuilder/jointree_transformers.go b/go/vt/vtgate/planbuilder/jointree_transformers.go index b2f1b93d384..d828cc0971c 100644 --- a/go/vt/vtgate/planbuilder/jointree_transformers.go +++ b/go/vt/vtgate/planbuilder/jointree_transformers.go @@ -83,7 +83,10 @@ func transformRoutePlan(n *routePlan) (*route, error) { switch predicate := predicate.(type) { case *sqlparser.ComparisonExpr: if predicate.Operator == sqlparser.InOp { - predicate.Right = sqlparser.ListArg(engine.ListVarName) + switch predicate.Left.(type) { + case *sqlparser.ColName: + predicate.Right = sqlparser.ListArg(engine.ListVarName) + } } } } diff --git a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt index 4be2688c686..0831743f0f4 100644 --- a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt @@ -174,6 +174,7 @@ Gen4 plan same as above "Vindex": "name_user_map" } } +Gen4 plan same as above # Composite IN clause, swapped columns "select id from user where (col, name) in (('aa', 'bb'), ('cc', 'dd'))" @@ -199,6 +200,7 @@ Gen4 plan same as above "Vindex": "name_user_map" } } +Gen4 plan same as above # Composite IN clause, choose cost within tuple "select id from user where (costly, name) in (('aa', 'bb'), ('cc', 'dd'))" @@ -249,6 +251,7 @@ Gen4 plan same as above "Vindex": "name_user_map" } } +Gen4 plan same as above # Composite IN clause, choose cost "select id from user where (col, costly) in (('aa', 'bb')) and (col, name) in (('cc', 'dd'))" @@ -273,6 +276,7 @@ Gen4 plan same as above "Vindex": "name_user_map" } } +Gen4 plan same as above # Composite IN clause vs equality "select id from user where (col, name) in (('aa', 'bb')) and id = 5" @@ -371,6 +375,7 @@ Gen4 plan same as above "Vindex": "name_user_map" } } +Gen4 plan same as above # Composite IN: tuple inside tuple, mismiatched values "select id from user where ((col1, name), col2) in (('aa', 'bb', 'cc'), (('dd', 'ee'), 'ff'))" From d80bb3d1a9a4e6cd78db2ef7dccde2567cb77b4e Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Thu, 24 Jun 2021 10:20:54 +0200 Subject: [PATCH 05/12] Choose cheapest vindex on composite in Signed-off-by: Florent Poinsard --- go/vt/vtgate/planbuilder/jointree.go | 7 +++---- go/vt/vtgate/planbuilder/testdata/filter_cases.txt | 2 ++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/go/vt/vtgate/planbuilder/jointree.go b/go/vt/vtgate/planbuilder/jointree.go index 3e26b2f1dad..47b0156265b 100644 --- a/go/vt/vtgate/planbuilder/jointree.go +++ b/go/vt/vtgate/planbuilder/jointree.go @@ -382,6 +382,7 @@ func (rp *routePlan) planCompositeInOp(node *sqlparser.ComparisonExpr, left sqlp return false, nil } + foundVindex := false for i, expr := range left { if col, isColName := expr.(*sqlparser.ColName); isColName { // check if left col is a vindex @@ -405,12 +406,10 @@ func (rp *routePlan) planCompositeInOp(node *sqlparser.ComparisonExpr, left sqlp opcode := func(*vindexes.ColumnVindex) engine.RouteOpcode { return engine.SelectMultiEqual } newVindex := rp.haveMatchingVindex(node, col, *newPlanValues, opcode, justTheVindex) - if newVindex { - return true, nil - } + foundVindex = newVindex || foundVindex } } - return false, nil + return foundVindex, nil } func (rp *routePlan) planInOp(node *sqlparser.ComparisonExpr) (bool, error) { diff --git a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt index 0831743f0f4..a053ec7848a 100644 --- a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt @@ -226,6 +226,7 @@ Gen4 plan same as above "Vindex": "name_user_map" } } +Gen4 plan same as above # Composite IN clause, choose cost within tuple, swapped "select id from user where (name, costly) in (('aa', 'bb'), ('cc', 'dd'))" @@ -325,6 +326,7 @@ Gen4 plan same as above "Vindex": "name_user_map" } } +Gen4 plan same as above # Composite IN: tuple inside tuple "select id from user where ((col1, name), col2) in ((('aa', 'bb'), 'cc'), (('dd', 'ee'), 'ff'))" From b2f4d9c607f28c74039db1bb4ed6d631125765fb Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Thu, 24 Jun 2021 10:42:05 +0200 Subject: [PATCH 06/12] Handle tuples inside tuples for IN planning Signed-off-by: Florent Poinsard --- go/vt/vtgate/planbuilder/jointree.go | 25 ++++++++++++++++--- go/vt/vtgate/planbuilder/route.go | 11 +++++++- .../planbuilder/testdata/filter_cases.txt | 1 + 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/go/vt/vtgate/planbuilder/jointree.go b/go/vt/vtgate/planbuilder/jointree.go index 47b0156265b..e7dc33332c2 100644 --- a/go/vt/vtgate/planbuilder/jointree.go +++ b/go/vt/vtgate/planbuilder/jointree.go @@ -381,12 +381,25 @@ func (rp *routePlan) planCompositeInOp(node *sqlparser.ComparisonExpr, left sqlp if !rightIsValTuple { return false, nil } + return rp.planCompositeInOpRecursive(node, left, right, nil) +} +func (rp *routePlan) planCompositeInOpRecursive(node *sqlparser.ComparisonExpr, left, right sqlparser.ValTuple, coordinates []int) (bool, error) { foundVindex := false + cindex := len(coordinates) + coordinates = append(coordinates, 0) for i, expr := range left { - if col, isColName := expr.(*sqlparser.ColName); isColName { + coordinates[cindex] = i + switch expr := expr.(type) { + case sqlparser.ValTuple: + ok, err := rp.planCompositeInOpRecursive(node, expr, right, coordinates) + if err != nil { + return false, err + } + return ok || foundVindex, nil + case *sqlparser.ColName: // check if left col is a vindex - if !rp.hasVindex(col) { + if !rp.hasVindex(expr) { continue } @@ -394,7 +407,11 @@ func (rp *routePlan) planCompositeInOp(node *sqlparser.ComparisonExpr, left sqlp for j, currRight := range right { switch currRight := currRight.(type) { case sqlparser.ValTuple: - rightVals[j] = currRight[i] + val := tupleAccess(currRight, coordinates) + if val == nil { + return false, nil + } + rightVals[j] = val default: return false, nil } @@ -405,7 +422,7 @@ func (rp *routePlan) planCompositeInOp(node *sqlparser.ComparisonExpr, left sqlp } opcode := func(*vindexes.ColumnVindex) engine.RouteOpcode { return engine.SelectMultiEqual } - newVindex := rp.haveMatchingVindex(node, col, *newPlanValues, opcode, justTheVindex) + newVindex := rp.haveMatchingVindex(node, expr, *newPlanValues, opcode, justTheVindex) foundVindex = newVindex || foundVindex } } diff --git a/go/vt/vtgate/planbuilder/route.go b/go/vt/vtgate/planbuilder/route.go index 504b73c6827..a93a4e1ef70 100644 --- a/go/vt/vtgate/planbuilder/route.go +++ b/go/vt/vtgate/planbuilder/route.go @@ -727,7 +727,16 @@ func (rb *route) computeCompositeINPlan(pb *primitiveBuilder, comparison *sqlpar // iterateCompositeIN recursively walks the LHS tuple of the IN clause looking // for column names. For those that match a vindex, it builds a multi-value plan // using the corresponding values in the RHS. It returns the best of the plans built. -func (rb *route) iterateCompositeIN(pb *primitiveBuilder, comparison *sqlparser.ComparisonExpr, coordinates []int, tuple sqlparser.ValTuple) (opcode engine.RouteOpcode, vindex vindexes.SingleColumn, values sqlparser.Expr) { +// (1, (2, (col, 3))) IN ((1,(2,(43,3))), ) +// 1 = [0] +// 2 = [1,0] +// col = [1,1,0] +func (rb *route) iterateCompositeIN( + pb *primitiveBuilder, + comparison *sqlparser.ComparisonExpr, + coordinates []int, + tuple sqlparser.ValTuple, +) (opcode engine.RouteOpcode, vindex vindexes.SingleColumn, values sqlparser.Expr) { opcode = engine.SelectScatter cindex := len(coordinates) diff --git a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt index a053ec7848a..1a80b3efb2f 100644 --- a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt @@ -352,6 +352,7 @@ Gen4 plan same as above "Vindex": "name_user_map" } } +Gen4 plan same as above # Composite IN: tuple inside tuple, but no match in tuple "select id from user where (name, (col1, col2)) in (('aa', ('bb', 'cc')), ('dd', ('ee', 'ff')))" From 85ae5954dad79670c3e10f9f521816eba5602557 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 24 Jun 2021 11:22:13 +0200 Subject: [PATCH 07/12] fail subqueries and use all available predicates Signed-off-by: Andres Taylor --- go/vt/vtgate/planbuilder/jointree.go | 38 +++++--- go/vt/vtgate/planbuilder/route_planning.go | 2 +- .../planbuilder/testdata/filter_cases.txt | 97 ++++++++++++++++++- go/vt/vtgate/semantics/analyzer.go | 2 + 4 files changed, 126 insertions(+), 13 deletions(-) diff --git a/go/vt/vtgate/planbuilder/jointree.go b/go/vt/vtgate/planbuilder/jointree.go index e7dc33332c2..ade239ea610 100644 --- a/go/vt/vtgate/planbuilder/jointree.go +++ b/go/vt/vtgate/planbuilder/jointree.go @@ -141,20 +141,31 @@ func (p parenTables) tableID() semantics.TableSet { } // visit will traverse the route tables, going inside parenTables and visiting all routeTables -func (p parenTables) visit(f func(tbl *routeTable) error) error { - for _, r := range p { - switch r := r.(type) { - case *routeTable: - err := f(r) - if err != nil { - return err - } - case parenTables: - err := r.visit(f) +func visitTables(r relation, f func(tbl *routeTable) error) error { + switch r := r.(type) { + case *routeTable: + err := f(r) + if err != nil { + return err + } + case parenTables: + for _, r := range r { + err := visitTables(r, f) if err != nil { return err } } + return nil + case *leJoin: + err := visitTables(r.lhs, f) + if err != nil { + return err + } + err = visitTables(r.rhs, f) + if err != nil { + return err + } + return nil } return nil } @@ -552,7 +563,12 @@ func (rp *routePlan) pickBestAvailableVindex() { // Predicates takes all known predicates for this route and ANDs them together func (rp *routePlan) Predicates() sqlparser.Expr { - return sqlparser.AndExpressions(rp.predicates...) + predicates := rp.predicates + _ = visitTables(rp.tables, func(tbl *routeTable) error { + predicates = append(predicates, tbl.qtable.Predicates...) + return nil + }) + return sqlparser.AndExpressions(predicates...) } func (rp *routePlan) pushOutputColumns(col []*sqlparser.ColName, _ *semantics.SemTable) []int { diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index be4e7feebe9..7b8e81e12a8 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -625,7 +625,7 @@ func findColumnVindex(a *routePlan, exp sqlparser.Expr, sem *semantics.SemTable) var singCol vindexes.SingleColumn - _ = a.tables.visit(func(table *routeTable) error { + _ = visitTables(a.tables, func(table *routeTable) error { if leftDep.IsSolvedBy(table.qtable.TableID) { for _, vindex := range table.vtable.ColumnVindexes { sC, isSingle := vindex.Vindex.(vindexes.SingleColumn) diff --git a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt index 1a80b3efb2f..613b661f063 100644 --- a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt @@ -564,6 +564,25 @@ Gen4 plan same as above "Vindex": "user_index" } } +{ + "QueryType": "SELECT", + "Original": "select user_extra.id from user join user_extra on user.id = user_extra.user_id where user.id = 5", + "Instructions": { + "OperatorType": "Route", + "Variant": "SelectEqualUnique", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select user_extra.id from `user`, user_extra where 1 != 1", + "Query": "select user_extra.id from `user`, user_extra where `user`.id = 5 and `user`.id = user_extra.user_id", + "Table": "`user`, user_extra", + "Values": [ + 5 + ], + "Vindex": "user_index" + } +} # Multi-table unique vindex constraint on right table "select user_extra.id from user join user_extra on user.id = user_extra.user_id where user_extra.user_id = 5" @@ -608,6 +627,7 @@ Gen4 plan same as above "Vindex": "user_index" } } +Gen4 plan same as above # Multi-table unique vindex constraint on left-joined right table "select user_extra.id from user left join user_extra on user.id = user_extra.user_id where user_extra.user_id = 5" @@ -717,6 +737,48 @@ Gen4 plan same as above ] } } +{ + "QueryType": "SELECT", + "Original": "select user_extra.id from user join user_extra on user.col = user_extra.col where user.id = 5 and user_extra.user_id = 5", + "Instructions": { + "OperatorType": "Join", + "Variant": "Join", + "JoinColumnIndexes": "1", + "TableName": "`user`_user_extra", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "SelectEqualUnique", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select `user`.col from `user` where 1 != 1", + "Query": "select `user`.col from `user` where `user`.id = 5", + "Table": "`user`", + "Values": [ + 5 + ], + "Vindex": "user_index" + }, + { + "OperatorType": "Route", + "Variant": "SelectEqualUnique", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select user_extra.id from user_extra where 1 != 1", + "Query": "select user_extra.id from user_extra where user_extra.user_id = 5 and user_extra.col = :user_col", + "Table": "user_extra", + "Values": [ + 5 + ], + "Vindex": "user_index" + } + ] + } +} # Multi-route with cross-route constraint "select user_extra.id from user join user_extra on user.col = user_extra.col where user_extra.user_id = user.col" @@ -796,6 +858,40 @@ Gen4 plan same as above ] } } +{ + "QueryType": "SELECT", + "Original": "select user_extra.id from user join user_extra on user.col = user_extra.col where 1 = 1", + "Instructions": { + "OperatorType": "Join", + "Variant": "Join", + "JoinColumnIndexes": "1", + "TableName": "`user`_user_extra", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "SelectScatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select `user`.col from `user` where 1 != 1", + "Query": "select `user`.col from `user` where 1 = 1", + "Table": "`user`" + }, + { + "OperatorType": "Route", + "Variant": "SelectScatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select user_extra.id from user_extra where 1 != 1", + "Query": "select user_extra.id from user_extra where 1 = 1 and user_extra.col = :user_col", + "Table": "user_extra" + } + ] + } +} # Route with multiple route constraints, SelectIN is the best constraint. "select id from user where user.col = 5 and user.id in (1, 2)" @@ -1288,7 +1384,6 @@ Gen4 plan same as above # unresolved symbol in inner subquery. "select id from user where id = :a and user.col in (select user_extra.col from user_extra where user_extra.user_id = :a and foo.id = 1)" "symbol foo.id not found" -Gen4 plan same as above # outer and inner subquery route by same outermost column value "select id2 from user uu where id in (select id from user where id = uu.id and user.col in (select user_extra.col from user_extra where user_extra.user_id = uu.id))" diff --git a/go/vt/vtgate/semantics/analyzer.go b/go/vt/vtgate/semantics/analyzer.go index e85692be1bb..374e3069169 100644 --- a/go/vt/vtgate/semantics/analyzer.go +++ b/go/vt/vtgate/semantics/analyzer.go @@ -79,6 +79,8 @@ func (a *analyzer) analyzeDown(cursor *sqlparser.Cursor) bool { a.selectScope[node] = a.currentScope() case *sqlparser.DerivedTable: a.err = Gen4NotSupportedF("derived tables") + case *sqlparser.Subquery: + a.err = Gen4NotSupportedF("subquery") case sqlparser.TableExpr: _, isSelect := cursor.Parent().(*sqlparser.Select) if isSelect { From 55b408063567c9a1cf91890c9d7e228636217108 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 24 Jun 2021 11:57:31 +0200 Subject: [PATCH 08/12] dont try to handle query comments Signed-off-by: Andres Taylor --- go/vt/vtgate/planbuilder/route_planning.go | 5 +++++ .../planbuilder/testdata/filter_cases.txt | 19 +++++++++++++++++++ .../planbuilder/testdata/select_cases.txt | 1 - 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index 7b8e81e12a8..9ddfcfb3fe3 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -46,6 +46,11 @@ func gen4Planner(_ string) func(sqlparser.Statement, *sqlparser.ReservedVars, Co } func newBuildSelectPlan(sel *sqlparser.Select, vschema ContextVSchema) (engine.Primitive, error) { + + directives := sqlparser.ExtractCommentDirectives(sel.Comments) + if len(directives) > 0 { + return nil, semantics.Gen4NotSupportedF("comment directives") + } keyspace, err := vschema.DefaultKeyspace() if err != nil { return nil, err diff --git a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt index 613b661f063..c7642bf5369 100644 --- a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt @@ -605,6 +605,25 @@ Gen4 plan same as above "Vindex": "user_index" } } +{ + "QueryType": "SELECT", + "Original": "select user_extra.id from user join user_extra on user.id = user_extra.user_id where user_extra.user_id = 5", + "Instructions": { + "OperatorType": "Route", + "Variant": "SelectEqualUnique", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select user_extra.id from `user`, user_extra where 1 != 1", + "Query": "select user_extra.id from `user`, user_extra where user_extra.user_id = 5 and `user`.id = user_extra.user_id", + "Table": "`user`, user_extra", + "Values": [ + 5 + ], + "Vindex": "user_index" + } +} # Multi-table unique vindex constraint on left table of left join "select user_extra.id from user left join user_extra on user.id = user_extra.user_id where user.id = 5" diff --git a/go/vt/vtgate/planbuilder/testdata/select_cases.txt b/go/vt/vtgate/planbuilder/testdata/select_cases.txt index ea44d9954a3..46650433aeb 100644 --- a/go/vt/vtgate/planbuilder/testdata/select_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/select_cases.txt @@ -1356,7 +1356,6 @@ Gen4 plan same as above "Table": "unsharded" } } -Gen4 plan same as above # testing SingleRow Projection "select 42" From 7e3d163a58b75036aec05e4507a5b921de69976d Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 24 Jun 2021 12:05:12 +0200 Subject: [PATCH 09/12] handle non-select queries Signed-off-by: Andres Taylor --- go/vt/vtgate/planbuilder/plan_test.go | 6 +++- .../testdata/alterVschema_cases.txt | 9 +++++ .../planbuilder/testdata/call_cases.txt | 4 +++ .../vtgate/planbuilder/testdata/ddl_cases.txt | 28 +++++++++++++++ .../planbuilder/testdata/flush_cases.txt | 3 ++ .../planbuilder/testdata/migration_cases.txt | 5 +++ .../planbuilder/testdata/show_cases.txt | 34 +++++++++++++++++++ 7 files changed, 88 insertions(+), 1 deletion(-) diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index 7d771dd412a..bd78144b732 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -238,7 +238,11 @@ func TestWithDefaultKeyspaceFromFile(t *testing.T) { // We are testing this separately so we can set a default keyspace testOutputTempDir, err := ioutil.TempDir("", "plan_test") require.NoError(t, err) - defer os.RemoveAll(testOutputTempDir) + defer func() { + if !t.Failed() { + _ = os.RemoveAll(testOutputTempDir) + } + }() vschema := &vschemaWrapper{ v: loadSchema(t, "schema_test.json"), keyspace: &vindexes.Keyspace{ diff --git a/go/vt/vtgate/planbuilder/testdata/alterVschema_cases.txt b/go/vt/vtgate/planbuilder/testdata/alterVschema_cases.txt index 7fe1469ddfd..9c3b03523e8 100644 --- a/go/vt/vtgate/planbuilder/testdata/alterVschema_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/alterVschema_cases.txt @@ -12,6 +12,7 @@ "query": "alter vschema create vindex hash_vdx using hash" } } +Gen4 plan same as above # Create vindex with qualifier "alter vschema create vindex user.hash_vdx using hash" @@ -27,6 +28,7 @@ "query": "alter vschema create vindex `user`.hash_vdx using hash" } } +Gen4 plan same as above # Drop vindex "alter vschema drop vindex hash_vdx" @@ -42,6 +44,7 @@ "query": "alter vschema drop vindex hash_vdx" } } +Gen4 plan same as above # Add table "alter vschema add table a" @@ -57,6 +60,7 @@ "query": "alter vschema add table a" } } +Gen4 plan same as above # Add sequence "alter vschema add sequence a_seq" @@ -72,6 +76,7 @@ "query": "alter vschema add sequence a_seq" } } +Gen4 plan same as above # Add auto_increment with qualifier "alter vschema on user.a add auto_increment id using a_seq" @@ -87,6 +92,7 @@ "query": "alter vschema on `user`.a add auto_increment id using a_seq" } } +Gen4 plan same as above # Drop table "alter vschema drop table a" @@ -102,6 +108,7 @@ "query": "alter vschema drop table a" } } +Gen4 plan same as above # Add Vindex "alter vschema on a add vindex hash (id)" @@ -117,6 +124,7 @@ "query": "alter vschema on a add vindex hash (id)" } } +Gen4 plan same as above # Drop Vindex "alter vschema on a drop vindex hash" @@ -132,3 +140,4 @@ "query": "alter vschema on a drop vindex hash" } } +Gen4 plan same as above diff --git a/go/vt/vtgate/planbuilder/testdata/call_cases.txt b/go/vt/vtgate/planbuilder/testdata/call_cases.txt index b70783a63cb..eb9e0277c84 100644 --- a/go/vt/vtgate/planbuilder/testdata/call_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/call_cases.txt @@ -13,6 +13,7 @@ "Query": "call proc()" } } +Gen4 plan same as above # call qualified keyspace "call main.proc()" @@ -29,10 +30,12 @@ "Query": "call proc()" } } +Gen4 plan same as above # CALL not allowed on sharded keyspaces "call user.proc()" "CALL is not supported for sharded database" +Gen4 plan same as above # CALL with expressions and parameters "call proc(1, 'foo', @var)" @@ -49,3 +52,4 @@ "Query": "call proc(1, 'foo', :__vtudvvar)" } } +Gen4 plan same as above diff --git a/go/vt/vtgate/planbuilder/testdata/ddl_cases.txt b/go/vt/vtgate/planbuilder/testdata/ddl_cases.txt index b7b14e4ce37..accb0316f37 100644 --- a/go/vt/vtgate/planbuilder/testdata/ddl_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/ddl_cases.txt @@ -12,6 +12,7 @@ "Query": "create table t1 (\n\tid bigint,\n\tprimary key (id)\n)" } } +Gen4 plan same as above # simple create table with keyspace "create table user.t1(id bigint, primary key(id))" @@ -27,6 +28,7 @@ "Query": "create table t1 (\n\tid bigint,\n\tprimary key (id)\n)" } } +Gen4 plan same as above # DDL "create table a(id int)" @@ -42,10 +44,12 @@ "Query": "create table a (\n\tid int\n)" } } +Gen4 plan same as above # simple create table with table qualifier that does not exists "create table a.b(id int)" "Unknown database 'a' in vschema" +Gen4 plan same as above #Alter table "alter table a ADD id int" @@ -61,6 +65,7 @@ "Query": "alter table a add column id int" } } +Gen4 plan same as above #Alter table with qualifier "alter table user.user ADD id int" @@ -76,6 +81,7 @@ "Query": "alter table `user` add column id int" } } +Gen4 plan same as above #Alter table with qualifier and table not in vschema "alter table user.a ADD id int" @@ -91,10 +97,12 @@ "Query": "alter table a add column id int" } } +Gen4 plan same as above #Alter table with unknown qualifier "alter table a.b ADD id int" "Unknown database 'a' in vschema" +Gen4 plan same as above # create db foo "create database foo" @@ -109,10 +117,12 @@ } } } +Gen4 plan same as above # create db main "create database main" "Can't create database 'main'; database exists" +Gen4 plan same as above # create db if not exists main "create database if not exists main" @@ -123,18 +133,22 @@ "OperatorType": "Rows" } } +Gen4 plan same as above # alter db foo "alter database foo collate utf8" "Can't alter database 'foo'; unknown database" +Gen4 plan same as above # alter db main "alter database main collate utf8" "alter database is not supported" +Gen4 plan same as above # drop db foo "drop database foo" "Can't drop database 'foo'; database doesn't exists" +Gen4 plan same as above # drop db main "drop database main" @@ -149,6 +163,7 @@ } } } +Gen4 plan same as above # drop db if exists main "drop database if exists main" @@ -163,6 +178,7 @@ } } } +Gen4 plan same as above # drop db if exists foo "drop schema if exists foo" @@ -173,6 +189,7 @@ "OperatorType": "Rows" } } +Gen4 plan same as above # DDL with qualifier "create index a on user.user(id)" @@ -188,6 +205,7 @@ "Query": "alter table `user` add index a (id)" } } +Gen4 plan same as above # DDL with qualifier for a table not in vschema of an unsharded keyspace "create index a on main.unknown(id)" @@ -203,6 +221,7 @@ "Query": "alter table unknown add index a (id)" } } +Gen4 plan same as above # create view with subquery in unsharded keyspace "create view view_a as select * from (select col1, col2 from unsharded where id = 1 union select col1, col2 from unsharded where id = 3) a" @@ -248,6 +267,7 @@ "Query": "create view view_a as (select id from unsharded) union (select id from unsharded_auto) order by id asc limit 5" } } +Gen4 plan same as above # create view with subquery in unsharded keyspace with multiple UNION clauses "create view view_a as select id from unsharded union select id from unsharded_auto union select id from unsharded_auto where id in (132)" @@ -263,6 +283,7 @@ "Query": "create view view_a as select id from unsharded union select id from unsharded_auto union select id from unsharded_auto where id in (132)" } } +Gen4 plan same as above # create view with subquery in unsharded keyspace with UNION clauses in subqueries "create view view_a as (select id from unsharded union select id from unsharded_auto) union (select id from unsharded_auto union select name from unsharded)" @@ -278,6 +299,7 @@ "Query": "create view view_a as (select id from unsharded union select id from unsharded_auto) union (select id from unsharded_auto union select `name` from unsharded)" } } +Gen4 plan same as above # Alter View "alter view user.user_extra as select* from user.user" @@ -308,6 +330,7 @@ "Query": "drop table unsharded_a" } } +Gen4 plan same as above # Drop view "drop view main.a" @@ -323,6 +346,7 @@ "Query": "drop view a" } } +Gen4 plan same as above # Truncate table with qualifier "truncate user.user_extra" @@ -338,6 +362,7 @@ "Query": "truncate table user_extra" } } +Gen4 plan same as above # Rename table "rename table a to main.b" @@ -353,6 +378,7 @@ "Query": "rename table a to b" } } +Gen4 plan same as above # CREATE temp TABLE "create temporary table a(id int)" @@ -369,6 +395,7 @@ "TempTable": true } } +Gen4 plan same as above # DROP temp TABLE "drop temporary table a" @@ -385,3 +412,4 @@ "TempTable": true } } +Gen4 plan same as above diff --git a/go/vt/vtgate/planbuilder/testdata/flush_cases.txt b/go/vt/vtgate/planbuilder/testdata/flush_cases.txt index c56ffd08c4a..a2d1f0ef319 100644 --- a/go/vt/vtgate/planbuilder/testdata/flush_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/flush_cases.txt @@ -13,6 +13,7 @@ "Query": "flush tables unsharded, music" } } +Gen4 plan same as above # Flush statement with no tables "flush local tables with read lock" @@ -29,6 +30,7 @@ "Query": "flush local tables with read lock" } } +Gen4 plan same as above # Flush statement with flush options "flush no_write_to_binlog hosts, logs" @@ -45,3 +47,4 @@ "Query": "flush local hosts, logs" } } +Gen4 plan same as above diff --git a/go/vt/vtgate/planbuilder/testdata/migration_cases.txt b/go/vt/vtgate/planbuilder/testdata/migration_cases.txt index 3fd7671e3c0..9f645bae590 100644 --- a/go/vt/vtgate/planbuilder/testdata/migration_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/migration_cases.txt @@ -12,6 +12,7 @@ "query": "revert vitess_migration 'abc'" } } +Gen4 plan same as above # retry migration "alter vitess_migration 'abc' retry" @@ -28,6 +29,7 @@ "Query": "alter vitess_migration 'abc' retry" } } +Gen4 plan same as above # complete migration "alter vitess_migration 'abc' complete" @@ -44,6 +46,7 @@ "Query": "alter vitess_migration 'abc' complete" } } +Gen4 plan same as above # cancel migration "alter vitess_migration 'abc' cancel" @@ -60,6 +63,7 @@ "Query": "alter vitess_migration 'abc' cancel" } } +Gen4 plan same as above # cancel all migrations "alter vitess_migration cancel all" @@ -76,3 +80,4 @@ "Query": "alter vitess_migration cancel all" } } +Gen4 plan same as above diff --git a/go/vt/vtgate/planbuilder/testdata/show_cases.txt b/go/vt/vtgate/planbuilder/testdata/show_cases.txt index f0ab459c4da..4b79891df17 100644 --- a/go/vt/vtgate/planbuilder/testdata/show_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/show_cases.txt @@ -14,6 +14,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # Show Table status with a keyspace name "SHOW table StatUs from main" @@ -31,6 +32,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # Show Table status with a keyspace name using IN "SHOW table StatUs In main" @@ -48,6 +50,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # Show Table status with a keyspace name with a condition "SHOW table StatUs In user WHERE Rows > 70" @@ -65,6 +68,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # Show Table status with a Like condition "SHOW table StatUs LIKe '%a'" @@ -82,6 +86,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show columns from user keyspace "show full columns from user.user_extra" @@ -99,6 +104,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show columns from main keyspace "show full columns from unsharded" @@ -116,6 +122,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show columns pass as dbname in from clause supersedes the qualifier "show full columns from user.unsharded from main" @@ -133,14 +140,17 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show columns fails as table does not exists in user keyspace "show full columns from unsharded from user" "table unsharded not found" +Gen4 plan same as above # show columns fails as table does not exists in user keyspace "show full columns from user.unsharded" "table unsharded not found" +Gen4 plan same as above # show charset "show charset" @@ -151,6 +161,7 @@ "OperatorType": "Rows" } } +Gen4 plan same as above # show function "show function status" @@ -168,6 +179,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show privileges "show privileges" @@ -185,6 +197,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show procedure status "show procedure status" @@ -202,6 +215,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show variables "show variables" @@ -224,6 +238,7 @@ ] } } +Gen4 plan same as above # show global variables "show global variables" @@ -246,6 +261,7 @@ ] } } +Gen4 plan same as above # show databases "show databases" @@ -256,6 +272,7 @@ "OperatorType": "Rows" } } +Gen4 plan same as above # show create database "show create database user" @@ -273,6 +290,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show create database system_schema "show create database mysql" @@ -290,6 +308,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show create procedure "show create procedure proc" @@ -307,6 +326,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show create procedure from system_schema "show create procedure information_schema.proc" @@ -324,6 +344,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show create table on table present in sharded but as unsharded is selected it goes to unsharded keyspace "show create table user_extra" @@ -341,6 +362,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show create table with qualifier "show create table user.user_extra" @@ -358,6 +380,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show create table with unsharded as default keyspace "show create table unknown" @@ -375,10 +398,12 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show create table with table not present with qualifier "show create table user.unknown" "table unknown not found" +Gen4 plan same as above # show create table from system_schema "show create table information_schema.tables" @@ -396,6 +421,7 @@ "SingleShardOnly": true } } +Gen4 plan same as above # show tables "show tables" @@ -424,6 +450,7 @@ ] } } +Gen4 plan same as above # show tables from db "show tables from user" @@ -452,6 +479,7 @@ ] } } +Gen4 plan same as above # show tables from system schema "show tables from performance_schema" @@ -480,6 +508,7 @@ ] } } +Gen4 plan same as above # show migrations with db and like "show vitess_migrations from user like '%format'" @@ -496,6 +525,7 @@ "Query": "SELECT * FROM _vt.schema_migrations where migration_uuid LIKE '%format' OR migration_context LIKE '%format' OR migration_status LIKE '%format'" } } +Gen4 plan same as above # show migrations with db and where "show vitess_migrations from user where id = 5" @@ -512,6 +542,7 @@ "Query": "SELECT * FROM _vt.schema_migrations where id = 5" } } +Gen4 plan same as above # show vgtid "show global vgtid_executed" @@ -536,6 +567,7 @@ ] } } +Gen4 plan same as above # show gtid "show global gtid_executed from user" @@ -553,6 +585,7 @@ "ShardNameNeeded": true } } +Gen4 plan same as above # show warnings "show warnings" @@ -563,3 +596,4 @@ "OperatorType": "SHOW WARNINGS" } } +Gen4 plan same as above From 7600d81deafe6d9172769758d59287ece76909a0 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Thu, 24 Jun 2021 12:55:23 +0200 Subject: [PATCH 10/12] Proper unsupported error for unsupported queries Signed-off-by: Florent Poinsard --- go/vt/vtgate/planbuilder/join.go | 4 +--- go/vt/vtgate/planbuilder/testdata/symtab_cases.txt | 2 ++ .../planbuilder/testdata/unsupported_cases.txt | 5 +++++ go/vt/vtgate/semantics/analyzer.go | 13 +++++++++---- 4 files changed, 17 insertions(+), 7 deletions(-) diff --git a/go/vt/vtgate/planbuilder/join.go b/go/vt/vtgate/planbuilder/join.go index 3da977b2c1c..30f79b69060 100644 --- a/go/vt/vtgate/planbuilder/join.go +++ b/go/vt/vtgate/planbuilder/join.go @@ -17,8 +17,6 @@ limitations under the License. package planbuilder import ( - "errors" - "vitess.io/vitess/go/vt/vtgate/semantics" vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" @@ -100,7 +98,7 @@ func newJoin(lpb, rpb *primitiveBuilder, ajoin *sqlparser.JoinTableExpr, reserve return err } case ajoin.Condition.Using != nil: - return errors.New("unsupported: join with USING(column_list) clause for complex queries") + return vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: join with USING(column_list) clause for complex queries") } } lpb.plan = &join{ diff --git a/go/vt/vtgate/planbuilder/testdata/symtab_cases.txt b/go/vt/vtgate/planbuilder/testdata/symtab_cases.txt index 24811cbc907..bb40820a797 100644 --- a/go/vt/vtgate/planbuilder/testdata/symtab_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/symtab_cases.txt @@ -36,7 +36,9 @@ ] } } +Gen4 plan same as above # predef1 is in both user and unsharded. So, it's ambiguous. "select predef1, predef3 from user join unsharded on predef1 = predef3" "symbol predef1 not found" +Gen4 error: Column 'predef1' in field list is ambiguous diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt index 472f937a5b5..30b06c4843f 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt @@ -40,22 +40,27 @@ Gen4 plan same as above # natural join "select * from user natural join user_extra" "unsupported: natural join" +Gen4 plan same as above # join with USING construct "select * from user join user_extra using(id)" "unsupported: join with USING(column_list) clause for complex queries" +Gen4 plan same as above # join with USING construct with 3 tables "select user.id from user join user_extra using(id) join music using(id2)" "unsupported: join with USING(column_list) clause for complex queries" +Gen4 plan same as above # natural left join "select * from user natural left join user_extra" "unsupported: natural left join" +Gen4 plan same as above # natural right join "select * from user natural right join user_extra" "unsupported: natural right join" +Gen4 plan same as above # left join with expressions "select user.id, user_extra.col+1 from user left join user_extra on user.col = user_extra.col" diff --git a/go/vt/vtgate/semantics/analyzer.go b/go/vt/vtgate/semantics/analyzer.go index 374e3069169..c3e7e952841 100644 --- a/go/vt/vtgate/semantics/analyzer.go +++ b/go/vt/vtgate/semantics/analyzer.go @@ -89,10 +89,15 @@ func (a *analyzer) analyzeDown(cursor *sqlparser.Cursor) bool { switch node := node.(type) { case *sqlparser.AliasedTableExpr: a.err = a.bindTable(node, node.Expr) - } - - // we don't need to push new scope for sub queries since we do that for SELECT and UNION + case *sqlparser.JoinTableExpr: + if node.Condition.Using != nil { + a.err = vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: join with USING(column_list) clause for complex queries") + } + if node.Join == sqlparser.NaturalJoinType || node.Join == sqlparser.NaturalRightJoinType || node.Join == sqlparser.NaturalLeftJoinType { + a.err = vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: "+node.Join.ToString()) + } + } case *sqlparser.Union: a.push(newScope(current)) case *sqlparser.ColName: @@ -163,7 +168,7 @@ func (a *analyzer) resolveUnQualifiedColumn(current *scope, expr *sqlparser.ColN var tblInfo *TableInfo for _, tbl := range current.tables { - if tbl.Table == nil || !tbl.Table.ColumnListAuthoritative { + if tbl.Table == nil { 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 { From f6e672ecace118470cd975d412e713eb2fae6f48 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Thu, 24 Jun 2021 14:44:47 +0200 Subject: [PATCH 11/12] Fixing ambiguous columns in analyzer and disallow projection on outer joins Co-authored-by: Andres Taylor Signed-off-by: Florent Poinsard --- go/vt/vtgate/planbuilder/project.go | 6 +- go/vt/vtgate/planbuilder/route_planning.go | 6 +- go/vt/vtgate/planbuilder/selectGen4.go | 21 ++++-- .../ddl_cases_no_default_keyspace.txt | 2 +- .../testdata/unsupported_cases.txt | 2 + go/vt/vtgate/semantics/analyzer.go | 71 +++++++++++++------ go/vt/vtgate/semantics/analyzer_test.go | 41 ++++++++++- go/vt/vtgate/semantics/semantic_state.go | 5 +- 8 files changed, 119 insertions(+), 35 deletions(-) diff --git a/go/vt/vtgate/planbuilder/project.go b/go/vt/vtgate/planbuilder/project.go index 71c1889ee84..3a1989cfee4 100644 --- a/go/vt/vtgate/planbuilder/project.go +++ b/go/vt/vtgate/planbuilder/project.go @@ -20,7 +20,7 @@ import ( "errors" querypb "vitess.io/vitess/go/vt/proto/query" - "vitess.io/vitess/go/vt/proto/vtrpc" + vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" "vitess.io/vitess/go/vt/sqlparser" "vitess.io/vitess/go/vt/vterrors" "vitess.io/vitess/go/vt/vtgate/engine" @@ -46,7 +46,7 @@ func planProjection(pb *primitiveBuilder, in logicalPlan, expr *sqlparser.Aliase } else { // Pushing of non-trivial expressions not allowed for RHS of left joins. if _, ok := expr.Expr.(*sqlparser.ColName); !ok && node.ejoin.Opcode == engine.LeftJoin { - return nil, nil, 0, errors.New("unsupported: cross-shard left join and column expressions") + return nil, nil, 0, vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: cross-shard left join and column expressions") } newRight, col, colNumber, err := planProjection(pb, node.Right, expr, origin) @@ -167,5 +167,5 @@ func planProjection(pb *primitiveBuilder, in logicalPlan, expr *sqlparser.Aliase return node, rc, len(node.resultColumns) - 1, nil } - return nil, nil, 0, vterrors.Errorf(vtrpc.Code_UNIMPLEMENTED, "[BUG] unreachable %T.projection", in) + return nil, nil, 0, vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "[BUG] unreachable %T.projection", in) } diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index 9ddfcfb3fe3..dc687c1b828 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -261,6 +261,10 @@ func planLimit(limit *sqlparser.Limit, plan logicalPlan) (logicalPlan, error) { func planHorizon(sel *sqlparser.Select, plan logicalPlan, semTable *semantics.SemTable) (logicalPlan, error) { rb, ok := plan.(*route) + if !ok && semTable.ProjectionErr != nil { + return nil, semTable.ProjectionErr + } + if ok && rb.isSingleShard() { createSingleShardRoutePlan(sel, rb) return plan, nil @@ -275,7 +279,7 @@ func planHorizon(sel *sqlparser.Select, plan logicalPlan, semTable *semantics.Se return nil, err } for _, e := range qp.selectExprs { - if _, err := pushProjection(e, plan, semTable); err != nil { + if _, err := pushProjection(e, plan, semTable, true); err != nil { return nil, err } } diff --git a/go/vt/vtgate/planbuilder/selectGen4.go b/go/vt/vtgate/planbuilder/selectGen4.go index aa31244a48d..9e803d13895 100644 --- a/go/vt/vtgate/planbuilder/selectGen4.go +++ b/go/vt/vtgate/planbuilder/selectGen4.go @@ -28,9 +28,18 @@ import ( "vitess.io/vitess/go/vt/vtgate/engine" ) -func pushProjection(expr *sqlparser.AliasedExpr, plan logicalPlan, semTable *semantics.SemTable) (int, error) { +func pushProjection(expr *sqlparser.AliasedExpr, plan logicalPlan, semTable *semantics.SemTable, inner bool) (int, error) { switch node := plan.(type) { case *route: + value, err := makePlanValue(expr.Expr) + if err != nil { + return 0, err + } + _, isColName := expr.Expr.(*sqlparser.ColName) + badExpr := value == nil && !isColName + if !inner && badExpr { + return 0, vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: cross-shard left join and column expressions") + } sel := node.Select.(*sqlparser.Select) i := checkIfAlreadyExists(expr, sel) if i != -1 { @@ -47,13 +56,13 @@ func pushProjection(expr *sqlparser.AliasedExpr, plan logicalPlan, semTable *sem deps := semTable.Dependencies(expr.Expr) switch { case deps.IsSolvedBy(lhsSolves): - offset, err := pushProjection(expr, node.Left, semTable) + offset, err := pushProjection(expr, node.Left, semTable, inner) if err != nil { return 0, err } node.Cols = append(node.Cols, -(offset + 1)) case deps.IsSolvedBy(rhsSolves): - offset, err := pushProjection(expr, node.Right, semTable) + offset, err := pushProjection(expr, node.Right, semTable, inner && node.Opcode != engine.LeftJoin) if err != nil { return 0, err } @@ -94,7 +103,7 @@ func planAggregations(qp *queryProjection, plan logicalPlan, semTable *semantics eaggr: eaggr, } for _, e := range qp.aggrExprs { - offset, err := pushProjection(e, plan, semTable) + offset, err := pushProjection(e, plan, semTable, true) if err != nil { return nil, err } @@ -123,7 +132,7 @@ func planOrderBy(qp *queryProjection, plan logicalPlan, semTable *semantics.SemT Expr: order.Expr, } var err error - offset, err = pushProjection(expr, plan, semTable) + offset, err = pushProjection(expr, plan, semTable, true) if err != nil { return nil, err } @@ -157,7 +166,7 @@ func planOrderBy(qp *queryProjection, plan logicalPlan, semTable *semantics.SemT }, }, } - weightStringOffset, err = pushProjection(expr, plan, semTable) + weightStringOffset, err = pushProjection(expr, plan, semTable, true) if err != nil { return nil, err } diff --git a/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.txt b/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.txt index 8850fd8799f..3a8f86a92cf 100644 --- a/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.txt +++ b/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.txt @@ -225,7 +225,7 @@ Gen4 plan same as above "Query": "create view view_a as select col from `user` join user_extra on `user`.id = user_extra.user_id" } } -Gen4 error: Column 'col' in field list is ambiguous +Gen4 plan same as above # create view with join that can be solved in each shard separately "create view user.view_a as select user.id from user join user_extra on user.id = user_extra.user_id" diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt index 30b06c4843f..6c942619184 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt @@ -65,10 +65,12 @@ Gen4 plan same as above # left join with expressions "select user.id, user_extra.col+1 from user left join user_extra on user.col = user_extra.col" "unsupported: cross-shard left join and column expressions" +Gen4 plan same as above # left join with expressions, with three-way join (different code path) "select user.id, user_extra.col+1 from user left join user_extra on user.col = user_extra.col join user_extra e" "unsupported: cross-shard left join and column expressions" +Gen4 plan same as above # left join where clauses "select user.id from user left join user_extra on user.col = user_extra.col where user_extra.col = 5" diff --git a/go/vt/vtgate/semantics/analyzer.go b/go/vt/vtgate/semantics/analyzer.go index c3e7e952841..0f0c7b49815 100644 --- a/go/vt/vtgate/semantics/analyzer.go +++ b/go/vt/vtgate/semantics/analyzer.go @@ -29,13 +29,15 @@ type ( analyzer struct { si SchemaInformation - Tables []*TableInfo - scopes []*scope - exprDeps map[sqlparser.Expr]TableSet - err error - currentDb string + Tables []*TableInfo + scopes []*scope + exprDeps map[sqlparser.Expr]TableSet + err error + currentDb string + inProjection []bool selectScope map[*sqlparser.Select]*scope + projErr error } ) @@ -57,7 +59,15 @@ func Analyze(statement sqlparser.Statement, currentDb string, si SchemaInformati if err != nil { return nil, err } - return &SemTable{exprDependencies: analyzer.exprDeps, Tables: analyzer.Tables, selectScope: analyzer.selectScope}, nil + return &SemTable{exprDependencies: analyzer.exprDeps, Tables: analyzer.Tables, selectScope: analyzer.selectScope, ProjectionErr: analyzer.projErr}, nil +} + +func (a *analyzer) setError(err error) { + if len(a.inProjection) > 0 && vterrors.ErrState(err) == vterrors.NonUniqError { + a.projErr = err + } else { + a.err = err + } } // analyzeDown pushes new scopes when we encounter sub queries, @@ -71,30 +81,34 @@ func (a *analyzer) analyzeDown(cursor *sqlparser.Cursor) bool { current := a.currentScope() n := cursor.Node() switch node := n.(type) { + case sqlparser.SelectExprs: + if isParentSelect(cursor) { + a.inProjection = append(a.inProjection, true) + fmt.Println(len(a.inProjection)) + } case *sqlparser.Select: if node.Having != nil { - a.err = Gen4NotSupportedF("HAVING") + a.setError(Gen4NotSupportedF("HAVING")) } a.push(newScope(current)) a.selectScope[node] = a.currentScope() case *sqlparser.DerivedTable: - a.err = Gen4NotSupportedF("derived tables") + a.setError(Gen4NotSupportedF("derived tables")) case *sqlparser.Subquery: - a.err = Gen4NotSupportedF("subquery") + a.setError(Gen4NotSupportedF("subquery")) case sqlparser.TableExpr: - _, isSelect := cursor.Parent().(*sqlparser.Select) - if isSelect { + if isParentSelect(cursor) { a.push(newScope(nil)) } switch node := node.(type) { case *sqlparser.AliasedTableExpr: - a.err = a.bindTable(node, node.Expr) + a.setError(a.bindTable(node, node.Expr)) case *sqlparser.JoinTableExpr: if node.Condition.Using != nil { - a.err = vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: join with USING(column_list) clause for complex queries") + a.setError(vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: join with USING(column_list) clause for complex queries")) } if node.Join == sqlparser.NaturalJoinType || node.Join == sqlparser.NaturalRightJoinType || node.Join == sqlparser.NaturalLeftJoinType { - a.err = vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: "+node.Join.ToString()) + a.setError(vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: "+node.Join.ToString())) } } @@ -103,7 +117,7 @@ func (a *analyzer) analyzeDown(cursor *sqlparser.Cursor) bool { case *sqlparser.ColName: t, err := a.resolveColumn(node, current) if err != nil { - a.err = err + a.setError(err) } else { a.exprDeps[node] = t } @@ -111,13 +125,13 @@ func (a *analyzer) analyzeDown(cursor *sqlparser.Cursor) bool { if node.Distinct { err := vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "syntax error: %s", sqlparser.String(node)) if len(node.Exprs) != 1 { - a.err = err + a.setError(err) } else if _, ok := node.Exprs[0].(*sqlparser.AliasedExpr); !ok { - a.err = err + a.setError(err) } } if sqlparser.IsLockingFunc(node) { - a.err = Gen4NotSupportedF("locking functions") + a.setError(Gen4NotSupportedF("locking functions")) } } @@ -127,6 +141,11 @@ func (a *analyzer) analyzeDown(cursor *sqlparser.Cursor) bool { return true } +func isParentSelect(cursor *sqlparser.Cursor) bool { + _, isSelect := cursor.Parent().(*sqlparser.Select) + return isSelect +} + func (a *analyzer) resolveColumn(colName *sqlparser.ColName, current *scope) (TableSet, error) { var t *TableInfo var err error @@ -138,6 +157,9 @@ func (a *analyzer) resolveColumn(colName *sqlparser.ColName, current *scope) (Ta if err != nil { return 0, err } + if t == nil { + return 0, vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.NonUniqError, fmt.Sprintf("Column '%s' in field list is ambiguous", sqlparser.String(colName))) + } return a.tableSetFor(t.ASTNode), nil } @@ -240,11 +262,14 @@ func (a *analyzer) analyzeUp(cursor *sqlparser.Cursor) bool { return false } switch cursor.Node().(type) { + case sqlparser.SelectExprs: + if isParentSelect(cursor) { + a.popProjection() + } case *sqlparser.Union, *sqlparser.Select: a.popScope() case sqlparser.TableExpr: - _, isSelect := cursor.Parent().(*sqlparser.Select) - if isSelect { + if isParentSelect(cursor) { curScope := a.currentScope() a.popScope() earlierScope := a.currentScope() @@ -252,7 +277,7 @@ func (a *analyzer) analyzeUp(cursor *sqlparser.Cursor) bool { for _, table := range curScope.tables { err := earlierScope.addTable(table) if err != nil { - a.err = err + a.setError(err) break } } @@ -261,6 +286,10 @@ func (a *analyzer) analyzeUp(cursor *sqlparser.Cursor) bool { return a.shouldContinue() } +func (a *analyzer) popProjection() { + a.inProjection = a.inProjection[:len(a.inProjection)-1] +} + func (a *analyzer) shouldContinue() bool { return a.err == nil } diff --git a/go/vt/vtgate/semantics/analyzer_test.go b/go/vt/vtgate/semantics/analyzer_test.go index afc2d74d992..ee1c2cad29b 100644 --- a/go/vt/vtgate/semantics/analyzer_test.go +++ b/go/vt/vtgate/semantics/analyzer_test.go @@ -346,12 +346,12 @@ func TestUnknownColumnMap2(t *testing.T) { { name: "non authoritative columns - one authoritative and one not", schema: map[string]*vindexes.Table{"a": &nonAuthoritativeTblA, "b": &authoritativeTblB}, - err: true, + err: false, }, { name: "non authoritative columns - one authoritative and one not", schema: map[string]*vindexes.Table{"a": &authoritativeTblA, "b": &nonAuthoritativeTblB}, - err: true, + err: false, }, { name: "authoritative columns", @@ -364,6 +364,43 @@ func TestUnknownColumnMap2(t *testing.T) { err: true, }, } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + si := &FakeSI{Tables: test.schema} + tbl, err := Analyze(parse, "", si) + require.NoError(t, err) + + if test.err { + require.Error(t, tbl.ProjectionErr) + } else { + require.NoError(t, tbl.ProjectionErr) + } + }) + } +} + +func TestUnknownPredicate(t *testing.T) { + query := "select 1 from a, b where col = 1" + authoritativeTblA := &vindexes.Table{ + Name: sqlparser.NewTableIdent("a"), + } + authoritativeTblB := &vindexes.Table{ + Name: sqlparser.NewTableIdent("b"), + } + + parse, _ := sqlparser.Parse(query) + + tests := []struct { + name string + schema map[string]*vindexes.Table + err bool + }{ + { + name: "no info about tables", + schema: map[string]*vindexes.Table{"a": authoritativeTblA, "b": authoritativeTblB}, + err: true, + }, + } for _, test := range tests { t.Run(test.name, func(t *testing.T) { si := &FakeSI{Tables: test.schema} diff --git a/go/vt/vtgate/semantics/semantic_state.go b/go/vt/vtgate/semantics/semantic_state.go index 648ad473138..5694f80607b 100644 --- a/go/vt/vtgate/semantics/semantic_state.go +++ b/go/vt/vtgate/semantics/semantic_state.go @@ -41,7 +41,10 @@ type ( // SemTable contains semantic analysis information about the query. SemTable struct { - Tables []*TableInfo + Tables []*TableInfo + // ProjectionErr stores the error that we got during the semantic analysis of the SelectExprs. + // This is only a real error if we are unable to plan the query as a single route + ProjectionErr error exprDependencies map[sqlparser.Expr]TableSet selectScope map[*sqlparser.Select]*scope } From e62a676d09199d1b1aa3d3c4fc6672e3113cba46 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Mon, 28 Jun 2021 08:06:32 +0200 Subject: [PATCH 12/12] Removed non-required comments Signed-off-by: Florent Poinsard --- go/vt/vtgate/planbuilder/route.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/go/vt/vtgate/planbuilder/route.go b/go/vt/vtgate/planbuilder/route.go index a93a4e1ef70..418d083e8f9 100644 --- a/go/vt/vtgate/planbuilder/route.go +++ b/go/vt/vtgate/planbuilder/route.go @@ -727,10 +727,6 @@ func (rb *route) computeCompositeINPlan(pb *primitiveBuilder, comparison *sqlpar // iterateCompositeIN recursively walks the LHS tuple of the IN clause looking // for column names. For those that match a vindex, it builds a multi-value plan // using the corresponding values in the RHS. It returns the best of the plans built. -// (1, (2, (col, 3))) IN ((1,(2,(43,3))), ) -// 1 = [0] -// 2 = [1,0] -// col = [1,1,0] func (rb *route) iterateCompositeIN( pb *primitiveBuilder, comparison *sqlparser.ComparisonExpr,