From bdfb4b97131e730da536233a03da0fcfca58a3fc Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Sat, 9 Jan 2021 08:08:51 +0100 Subject: [PATCH 01/13] moved out jointree transformations into its own file Signed-off-by: Andres Taylor --- .../planbuilder/jointree_transformers.go | 168 ++++++++++++++++++ go/vt/vtgate/planbuilder/route_planning.go | 157 ++-------------- .../vtgate/planbuilder/route_planning_test.go | 19 ++ 3 files changed, 203 insertions(+), 141 deletions(-) create mode 100644 go/vt/vtgate/planbuilder/jointree_transformers.go diff --git a/go/vt/vtgate/planbuilder/jointree_transformers.go b/go/vt/vtgate/planbuilder/jointree_transformers.go new file mode 100644 index 00000000000..239c659262c --- /dev/null +++ b/go/vt/vtgate/planbuilder/jointree_transformers.go @@ -0,0 +1,168 @@ +/* +Copyright 2021 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package planbuilder + +import ( + "sort" + "strings" + + "vitess.io/vitess/go/sqltypes" + vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" + "vitess.io/vitess/go/vt/vtgate/semantics" + "vitess.io/vitess/go/vt/vtgate/vindexes" + + "vitess.io/vitess/go/vt/vterrors" + + "vitess.io/vitess/go/vt/sqlparser" + "vitess.io/vitess/go/vt/vtgate/engine" +) + +func transformToLogicalPlan(tree joinTree, semTable *semantics.SemTable) (logicalPlan, error) { + switch n := tree.(type) { + case *routePlan: + return transformRoutePlan(n) + + case *joinPlan: + return transformJoinPlan(n, semTable) + } + + return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "BUG: unknown type encountered: %T", tree) +} + +func transformJoinPlan(n *joinPlan, semTable *semantics.SemTable) (*join2, error) { + lhsColList := extractColumnsNeededFromLHS(n, semTable, n.lhs.solves()) + + var lhsColExpr []*sqlparser.AliasedExpr + for _, col := range lhsColList { + lhsColExpr = append(lhsColExpr, &sqlparser.AliasedExpr{ + Expr: col, + }) + } + + lhs, err := transformToLogicalPlan(n.lhs, semTable) + if err != nil { + return nil, err + } + offset, err := pushProjection(lhsColExpr, lhs, semTable) + if err != nil { + return nil, err + } + + vars := map[string]int{} + + for _, col := range lhsColList { + vars[col.CompliantName("")] = offset + offset++ + } + + rhs, err := transformToLogicalPlan(n.rhs, semTable) + if err != nil { + return nil, err + } + + err = pushPredicate(n.predicates, rhs, semTable) + if err != nil { + return nil, err + } + + return &join2{ + Left: lhs, + Right: rhs, + Vars: vars, + }, nil +} + +func transformRoutePlan(n *routePlan) (*route, error) { + var tablesForSelect sqlparser.TableExprs + tableNameMap := map[string]interface{}{} + + sort.Sort(n.tables) + for _, t := range n.tables { + alias := sqlparser.AliasedTableExpr{ + Expr: sqlparser.TableName{ + Name: t.vtable.Name, + }, + Partitions: nil, + As: t.qtable.alias.As, + Hints: nil, + } + tablesForSelect = append(tablesForSelect, &alias) + tableNameMap[sqlparser.String(t.qtable.table.Name)] = nil + } + + predicates := n.Predicates() + var where *sqlparser.Where + if predicates != nil { + where = &sqlparser.Where{Expr: predicates, Type: sqlparser.WhereClause} + } + var values []sqltypes.PlanValue + if len(n.conditions) == 1 { + value, err := sqlparser.NewPlanValue(n.conditions[0].(*sqlparser.ComparisonExpr).Right) + if err != nil { + return nil, err + } + values = []sqltypes.PlanValue{value} + } + var singleColumn vindexes.SingleColumn + if n.vindex != nil { + singleColumn = n.vindex.(vindexes.SingleColumn) + } + + var tableNames []string + for name := range tableNameMap { + tableNames = append(tableNames, name) + } + sort.Strings(tableNames) + + return &route{ + eroute: &engine.Route{ + Opcode: n.routeOpCode, + TableName: strings.Join(tableNames, ", "), + Keyspace: n.keyspace, + Vindex: singleColumn, + Values: values, + }, + Select: &sqlparser.Select{ + From: tablesForSelect, + Where: where, + }, + solvedTables: n.solved, + }, nil +} + +func extractColumnsNeededFromLHS(n *joinPlan, semTable *semantics.SemTable, lhsSolves semantics.TableSet) []*sqlparser.ColName { + lhsColMap := map[*sqlparser.ColName]sqlparser.Argument{} + for _, predicate := range n.predicates { + sqlparser.Rewrite(predicate, func(cursor *sqlparser.Cursor) bool { + switch node := cursor.Node().(type) { + case *sqlparser.ColName: + if semTable.Dependencies(node).IsSolvedBy(lhsSolves) { + arg := sqlparser.NewArgument([]byte(":" + node.CompliantName(""))) + lhsColMap[node] = arg + cursor.Replace(arg) + } + } + return true + }, nil) + } + + var lhsColList []*sqlparser.ColName + for col := range lhsColMap { + lhsColList = append(lhsColList, col) + } + return lhsColList +} diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index bb9df3655b5..0800675e84c 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -18,9 +18,6 @@ package planbuilder import ( "sort" - "strings" - - "vitess.io/vitess/go/sqltypes" vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" "vitess.io/vitess/go/vt/vtgate/semantics" @@ -128,6 +125,10 @@ type ( // vindex and conditions is set if a vindex will be used for this route. vindex vindexes.Vindex conditions []sqlparser.Expr + + // here we store the possible vindexes we can use so that when we add predicates to the plan, + // we can quickly check if the new predicates enables any new vindex options + vindexPreds []vindexPlusPredicates } joinPlan struct { predicates []sqlparser.Expr @@ -136,7 +137,16 @@ type ( routeTables []*routeTable ) -// solves implements the joinTree interface +// clone returns a copy of the struct with copies of slices, +// so changing the the contents of them will not be reflected in the original +func (rp *routePlan) clone() *routePlan { + result := *rp + result.vindexPreds = make([]vindexPlusPredicates, len(rp.vindexPreds)) + copy(result.vindexPreds, rp.vindexPreds) + return &result +} + +// solves returns the tables that this plan is solving func (rp *routePlan) solves() semantics.TableSet { return rp.solved } @@ -149,8 +159,9 @@ func (*routePlan) cost() int { // vindexPlusPredicates is a struct used to store all the predicates that the vindex can be used to query type vindexPlusPredicates struct { vindex *vindexes.ColumnVindex - covered bool predicates []sqlparser.Expr + // Vindex is covered if all the columns in the vindex have an associated predicate + covered bool } func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { @@ -416,142 +427,6 @@ func createRoutePlan(table *queryTable, solves semantics.TableSet, vschema Conte return plan, nil } -func transformToLogicalPlan(tree joinTree, semTable *semantics.SemTable) (logicalPlan, error) { - switch n := tree.(type) { - case *routePlan: - return transformRoutePlan(n) - - case *joinPlan: - return transformJoinPlan(n, semTable) - } - - return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "BUG: unknown type encountered: %T", tree) -} - -func transformJoinPlan(n *joinPlan, semTable *semantics.SemTable) (*join2, error) { - lhsColList := extractColumnsNeededFromLHS(n, semTable, n.lhs.solves()) - - var lhsColExpr []*sqlparser.AliasedExpr - for _, col := range lhsColList { - lhsColExpr = append(lhsColExpr, &sqlparser.AliasedExpr{ - Expr: col, - }) - } - - lhs, err := transformToLogicalPlan(n.lhs, semTable) - if err != nil { - return nil, err - } - offset, err := pushProjection(lhsColExpr, lhs, semTable) - if err != nil { - return nil, err - } - - vars := map[string]int{} - - for _, col := range lhsColList { - vars[col.CompliantName("")] = offset - offset++ - } - - rhs, err := transformToLogicalPlan(n.rhs, semTable) - if err != nil { - return nil, err - } - - err = pushPredicate(n.predicates, rhs, semTable) - if err != nil { - return nil, err - } - - return &join2{ - Left: lhs, - Right: rhs, - Vars: vars, - }, nil -} - -func extractColumnsNeededFromLHS(n *joinPlan, semTable *semantics.SemTable, lhsSolves semantics.TableSet) []*sqlparser.ColName { - lhsColMap := map[*sqlparser.ColName]sqlparser.Argument{} - for _, predicate := range n.predicates { - sqlparser.Rewrite(predicate, func(cursor *sqlparser.Cursor) bool { - switch node := cursor.Node().(type) { - case *sqlparser.ColName: - if semTable.Dependencies(node).IsSolvedBy(lhsSolves) { - arg := sqlparser.NewArgument([]byte(":" + node.CompliantName(""))) - lhsColMap[node] = arg - cursor.Replace(arg) - } - } - return true - }, nil) - } - - var lhsColList []*sqlparser.ColName - for col := range lhsColMap { - lhsColList = append(lhsColList, col) - } - return lhsColList -} - -func transformRoutePlan(n *routePlan) (*route, error) { - var tablesForSelect sqlparser.TableExprs - tableNameMap := map[string]interface{}{} - - sort.Sort(n.tables) - for _, t := range n.tables { - alias := sqlparser.AliasedTableExpr{ - Expr: sqlparser.TableName{ - Name: t.vtable.Name, - }, - Partitions: nil, - As: t.qtable.alias.As, - Hints: nil, - } - tablesForSelect = append(tablesForSelect, &alias) - tableNameMap[sqlparser.String(t.qtable.table.Name)] = nil - } - - predicates := n.Predicates() - var where *sqlparser.Where - if predicates != nil { - where = &sqlparser.Where{Expr: predicates, Type: sqlparser.WhereClause} - } - var values []sqltypes.PlanValue - if len(n.conditions) == 1 { - value, err := sqlparser.NewPlanValue(n.conditions[0].(*sqlparser.ComparisonExpr).Right) - if err != nil { - return nil, err - } - values = []sqltypes.PlanValue{value} - } - var singleColumn vindexes.SingleColumn - if n.vindex != nil { - singleColumn = n.vindex.(vindexes.SingleColumn) - } - - var tableNames []string - for name := range tableNameMap { - tableNames = append(tableNames, name) - } - sort.Strings(tableNames) - - return &route{ - eroute: &engine.Route{ - Opcode: n.routeOpCode, - TableName: strings.Join(tableNames, ", "), - Keyspace: n.keyspace, - Vindex: singleColumn, - Values: values, - }, - Select: &sqlparser.Select{ - From: tablesForSelect, - Where: where, - }, - solvedTables: n.solved, - }, nil -} - func findColumnVindex(a *routePlan, exp sqlparser.Expr, sem *semantics.SemTable) vindexes.SingleColumn { left, isCol := exp.(*sqlparser.ColName) if !isCol { diff --git a/go/vt/vtgate/planbuilder/route_planning_test.go b/go/vt/vtgate/planbuilder/route_planning_test.go index f3335d9d1e5..fe7dc39224b 100644 --- a/go/vt/vtgate/planbuilder/route_planning_test.go +++ b/go/vt/vtgate/planbuilder/route_planning_test.go @@ -104,3 +104,22 @@ func TestMergeJoins(t *testing.T) { }) } } + +func TestClone(t *testing.T) { + original := &routePlan{ + routeOpCode: engine.SelectEqualUnique, + vindexPreds: []vindexPlusPredicates{{ + covered: false, + }}, + } + + clone := original.clone() + + clone.routeOpCode = engine.SelectDBA + assert.Equal(t, clone.routeOpCode, engine.SelectDBA) + assert.Equal(t, original.routeOpCode, engine.SelectEqualUnique) + + clone.vindexPreds[0].covered = true + assert.True(t, clone.vindexPreds[0].covered) + assert.False(t, original.vindexPreds[0].covered) +} From 6b44402fae8766952bb7b938f4628e3db6c212f5 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Sat, 9 Jan 2021 17:23:50 +0100 Subject: [PATCH 02/13] start of making it possible to push predicates on the joinTree Signed-off-by: Andres Taylor --- go/vt/vtgate/planbuilder/route_planning.go | 189 +++++++++++++----- .../vtgate/planbuilder/route_planning_test.go | 9 +- 2 files changed, 146 insertions(+), 52 deletions(-) diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index 0800675e84c..8ae31f8ab17 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -17,6 +17,7 @@ limitations under the License. package planbuilder import ( + "fmt" "sort" vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" @@ -105,6 +106,9 @@ type ( // cost is simply the number of routes in the joinTree cost() int + + // creates a copy of the joinTree that can be updated without changing the original + clone() joinTree } routeTable struct { qtable *queryTable @@ -139,7 +143,7 @@ type ( // clone returns a copy of the struct with copies of slices, // so changing the the contents of them will not be reflected in the original -func (rp *routePlan) clone() *routePlan { +func (rp *routePlan) clone() joinTree { result := *rp result.vindexPreds = make([]vindexPlusPredicates, len(rp.vindexPreds)) copy(result.vindexPreds, rp.vindexPreds) @@ -152,7 +156,25 @@ func (rp *routePlan) solves() semantics.TableSet { } // cost implements the joinTree interface -func (*routePlan) cost() int { +func (rp *routePlan) cost() int { + switch rp.routeOpCode { + case + engine.SelectDBA, + engine.SelectEqualUnique, + engine.SelectNext, + engine.SelectNone, + engine.SelectReference, + engine.SelectUnsharded: + return 1 + case engine.SelectEqual: + return 5 + case engine.SelectIN: + return 10 + case engine.SelectMultiEqual: + return 10 + case engine.SelectScatter: + return 20 + } return 1 } @@ -164,23 +186,16 @@ type vindexPlusPredicates struct { covered bool } +// addPredicate clones this routePlan and returns a new one with these predicates added to it. if the predicates can help, +// they will improve the routeOpCode func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { - if len(rp.tables) != 1 { - return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "addPredicate should only be called when the route has a single table") - } - - var vindexPreds []*vindexPlusPredicates - - // Add all the column vindexes to the list of vindexPlusPredicates - for _, columnVindex := range rp.tables[0].vtable.ColumnVindexes { - vindexPreds = append(vindexPreds, &vindexPlusPredicates{vindex: columnVindex}) - } - + newVindexFound := false for _, filter := range predicates { switch node := filter.(type) { case *sqlparser.ComparisonExpr: switch node.Operator { case sqlparser.EqualOp: + // here we are searching for predicates in the form n.col = XYZ if sqlparser.IsNull(node.Left) || sqlparser.IsNull(node.Right) { // we are looking at ANDed predicates in the WHERE clause. // since we know that nothing returns true when compared to NULL, @@ -189,7 +204,11 @@ func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { return nil } // TODO(Manan,Andres): Remove the predicates that are repeated eg. Id=1 AND Id=1 - for _, v := range vindexPreds { + for _, v := range rp.vindexPreds { + if v.covered { + // already covered by an earlier predicate + continue + } column := node.Left.(*sqlparser.ColName) for _, col := range v.vindex.Columns { // If the column for the predicate matches any column in the vindex add it to the list @@ -197,6 +216,7 @@ func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { v.predicates = append(v.predicates, node) // Vindex is covered if all the columns in the vindex have a associated predicate v.covered = len(v.predicates) == len(v.vindex.Columns) + newVindexFound = true } } } @@ -204,8 +224,21 @@ func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { } } + // if we didn't open up any new vindex options, no need to enter here + if newVindexFound { + rp.pickBestAvailableVindex() + } + + // any predicates that cover more than a single table need to be added here + rp.extraPredicates = append(rp.extraPredicates, predicates...) + + return nil +} + +// pickBestAvailableVindex goes over the available vindexes for this route and picks the best one available. +func (rp *routePlan) pickBestAvailableVindex() { //TODO (Manan,Andres): Improve cost metric for vindexes - for _, v := range vindexPreds { + for _, v := range rp.vindexPreds { if !v.covered { continue } @@ -222,7 +255,6 @@ func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { rp.routeOpCode = engine.SelectEqualUnique } } - return nil } // Predicates takes all known predicates for this route and ANDs them together @@ -255,17 +287,69 @@ func (jp *joinPlan) solves() semantics.TableSet { func (jp *joinPlan) cost() int { return jp.lhs.cost() + jp.rhs.cost() } +func (jp *joinPlan) clone() joinTree { + result := &joinPlan{ + predicates: make([]sqlparser.Expr, len(jp.predicates)), + lhs: jp.lhs.clone(), + rhs: jp.rhs.clone(), + } + copy(result.predicates, jp.predicates) + return result +} -func createJoin(lhs, rhs joinTree, joinPredicates []sqlparser.Expr, semTable *semantics.SemTable) joinTree { - newPlan := tryMerge(lhs, rhs, joinPredicates, semTable) - if newPlan == nil { - newPlan = &joinPlan{ - lhs: lhs, - rhs: rhs, - predicates: joinPredicates, +func pushPredicate2(exprs []sqlparser.Expr, tree joinTree, semTable *semantics.SemTable) (joinTree, error) { + switch node := tree.(type) { + case *routePlan: + plan := node.clone().(*routePlan) + err := plan.addPredicate(exprs...) + if err != nil { + return nil, err + } + return plan, nil + + case *joinPlan: + // we need to figure out which predicates go where + var lhsPreds, rhsPreds []sqlparser.Expr + lhsSolves := node.lhs.solves() + rhsSolves := node.rhs.solves() + for _, expr := range exprs { + // TODO: we should not do this. + // we should instead do the same as go/vt/vtgate/planbuilder/jointree_transformers.go:46 + deps := semTable.Dependencies(expr) + switch { + case deps.IsSolvedBy(lhsSolves): + lhsPreds = append(lhsPreds, expr) + case deps.IsSolvedBy(rhsSolves): + rhsPreds = append(rhsPreds, expr) + default: + return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "unknown dependencies for %s", sqlparser.String(expr)) + } + } + lhsPlan, err := pushPredicate2(lhsPreds, node.lhs, semTable) + if err != nil { + return nil, err + } + rhsPlan, err := pushPredicate2(rhsPreds, node.rhs, semTable) + if err != nil { + return nil, err } + return &joinPlan{ + predicates: exprs, + lhs: lhsPlan, + rhs: rhsPlan, + }, nil + default: + panic(fmt.Sprintf("BUG: unknown type %T", node)) } - return newPlan +} + +func mergeOrJoin(lhs, rhs joinTree, joinPredicates []sqlparser.Expr, semTable *semantics.SemTable) (joinTree, error) { + newPlan := tryMerge(lhs, rhs, joinPredicates, semTable) + if newPlan != nil { + return newPlan, nil + } + + return pushPredicate2(joinPredicates, &joinPlan{lhs: lhs, rhs: rhs}, semTable) } type ( @@ -297,7 +381,10 @@ func greedySolve(qg *queryGraph, semTable *semantics.SemTable, vschema ContextVS crossJoinsOK := false for len(plans) > 1 { - bestPlan, lIdx, rIdx := findBestJoin(qg, semTable, plans, planCache, crossJoinsOK) + bestPlan, lIdx, rIdx, err := findBestJoin(qg, semTable, plans, planCache, crossJoinsOK) + if err != nil { + return nil, err + } if bestPlan != nil { // if we found a best plan, we'll replace the two plans that were joined with the join plan created plans = removeAt(plans, rIdx) @@ -314,14 +401,19 @@ func greedySolve(qg *queryGraph, semTable *semantics.SemTable, vschema ContextVS return plans[0], nil } -func (cm cacheMap) getJoinFor(lhs, rhs joinTree, joinPredicates []sqlparser.Expr, semTable *semantics.SemTable) joinTree { +func (cm cacheMap) getJoinFor(lhs, rhs joinTree, joinPredicates []sqlparser.Expr, semTable *semantics.SemTable) (joinTree, error) { solves := tableSetPair{left: lhs.solves(), right: rhs.solves()} - plan := cm[solves] - if plan == nil { - plan = createJoin(lhs, rhs, joinPredicates, semTable) - cm[solves] = plan + cachedPlan := cm[solves] + if cachedPlan != nil { + return cachedPlan, nil } - return plan + + join, err := mergeOrJoin(lhs, rhs, joinPredicates, semTable) + if err != nil { + return nil, err + } + cm[solves] = join + return join, nil } func findBestJoin( @@ -330,10 +422,7 @@ func findBestJoin( plans []joinTree, planCache cacheMap, crossJoinsOK bool, -) (joinTree, int, int) { - var lIdx, rIdx int - var bestPlan joinTree - +) (bestPlan joinTree, lIdx int, rIdx int, err error) { for i, lhs := range plans { for j, rhs := range plans { if i == j { @@ -346,8 +435,10 @@ func findBestJoin( // cartesian product, which is almost always a bad idea continue } - plan := planCache.getJoinFor(lhs, rhs, joinPredicates, semTable) - + plan, err := planCache.getJoinFor(lhs, rhs, joinPredicates, semTable) + if err != nil { + return nil, 0, 0, err + } if bestPlan == nil || plan.cost() < bestPlan.cost() { bestPlan = plan // remember which plans we based on, so we can remove them later @@ -356,7 +447,7 @@ func findBestJoin( } } } - return bestPlan, lIdx, rIdx + return bestPlan, lIdx, rIdx, nil } func leftToRightSolve(qg *queryGraph, semTable *semantics.SemTable, vschema ContextVSchema) (joinTree, error) { @@ -373,13 +464,17 @@ func leftToRightSolve(qg *queryGraph, semTable *semantics.SemTable, vschema Cont } var acc joinTree + var err error for _, plan := range plans { if acc == nil { acc = plan continue } joinPredicates := qg.getPredicates(acc.solves(), plan.solves()) - acc = createJoin(acc, plan, joinPredicates, semTable) + acc, err = mergeOrJoin(acc, plan, joinPredicates, semTable) + if err != nil { + return nil, err + } } return acc, nil @@ -403,6 +498,10 @@ func createRoutePlan(table *queryTable, solves semantics.TableSet, vschema Conte keyspace: vschemaTable.Keyspace, } + for _, columnVindex := range vschemaTable.ColumnVindexes { + plan.vindexPreds = append(plan.vindexPreds, vindexPlusPredicates{vindex: columnVindex}) + } + switch { case vschemaTable.Type == vindexes.TypeSequence: plan.routeOpCode = engine.SelectNext @@ -502,7 +601,8 @@ func tryMerge(a, b joinTree, joinPredicates []sqlparser.Expr, semTable *semantic extraPredicates: append( append(aRoute.extraPredicates, bRoute.extraPredicates...), joinPredicates...), - keyspace: aRoute.keyspace, + keyspace: aRoute.keyspace, + vindexPreds: append(aRoute.vindexPreds, bRoute.vindexPreds...), } switch aRoute.routeOpCode { @@ -522,14 +622,7 @@ func tryMerge(a, b joinTree, joinPredicates []sqlparser.Expr, semTable *semantic if !canMerge { return nil } - if aRoute.routeOpCode == engine.SelectEqualUnique { - r.vindex = aRoute.vindex - r.conditions = aRoute.conditions - } else if bRoute.routeOpCode == engine.SelectEqualUnique { - r.routeOpCode = bRoute.routeOpCode - r.vindex = bRoute.vindex - r.conditions = bRoute.conditions - } + r.pickBestAvailableVindex() } return r diff --git a/go/vt/vtgate/planbuilder/route_planning_test.go b/go/vt/vtgate/planbuilder/route_planning_test.go index fe7dc39224b..aaff9c75260 100644 --- a/go/vt/vtgate/planbuilder/route_planning_test.go +++ b/go/vt/vtgate/planbuilder/route_planning_test.go @@ -115,11 +115,12 @@ func TestClone(t *testing.T) { clone := original.clone() - clone.routeOpCode = engine.SelectDBA - assert.Equal(t, clone.routeOpCode, engine.SelectDBA) + clonedRP := clone.(*routePlan) + clonedRP.routeOpCode = engine.SelectDBA + assert.Equal(t, clonedRP.routeOpCode, engine.SelectDBA) assert.Equal(t, original.routeOpCode, engine.SelectEqualUnique) - clone.vindexPreds[0].covered = true - assert.True(t, clone.vindexPreds[0].covered) + clonedRP.vindexPreds[0].covered = true + assert.True(t, clonedRP.vindexPreds[0].covered) assert.False(t, original.vindexPreds[0].covered) } From 9fedb9039234229a32657a43eebfdcdc65227345 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Sun, 10 Jan 2021 12:26:15 +0100 Subject: [PATCH 03/13] Added Clone() method to sqlparser.Expr Signed-off-by: Andres Taylor --- go/vt/sqlparser/ast.go | 1 + go/vt/sqlparser/ast_funcs.go | 257 +++++++++++++++++++++++++++++++++++ 2 files changed, 258 insertions(+) diff --git a/go/vt/sqlparser/ast.go b/go/vt/sqlparser/ast.go index c508c526c8e..d8d6f634e5d 100644 --- a/go/vt/sqlparser/ast.go +++ b/go/vt/sqlparser/ast.go @@ -1469,6 +1469,7 @@ type ( Expr interface { iExpr() SQLNode + Clone() Expr } // AndExpr represents an AND expression. diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index 76acef968fd..6588d50a300 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -1283,3 +1283,260 @@ const ( // DoubleAt represnts @@ DoubleAt ) + +func (node *Subquery) Clone() Expr { + if node == nil { + return nil + } + panic(1) +} + +func (node *AndExpr) Clone() Expr { + if node == nil { + return nil + } + return &AndExpr{ + Left: node.Left.Clone(), + Right: node.Right.Clone(), + } +} + +func (node *OrExpr) Clone() Expr { + if node == nil { + return nil + } + return &OrExpr{ + Left: node.Left.Clone(), + Right: node.Right.Clone(), + } +} + +func (node *XorExpr) Clone() Expr { + if node == nil { + return nil + } + return &XorExpr{ + Left: node.Left.Clone(), + Right: node.Right.Clone(), + } +} + +func (node *NotExpr) Clone() Expr { + if node == nil { + return nil + } + return &NotExpr{ + Expr: node.Clone(), + } +} + +func (node *ComparisonExpr) Clone() Expr { + if node == nil { + return nil + } + return &ComparisonExpr{ + Operator: node.Operator, + Left: node.Left.Clone(), + Right: node.Right.Clone(), + Escape: node.Escape.Clone(), + } +} +func (node *RangeCond) Clone() Expr { + if node == nil { + return nil + } + return &RangeCond{ + Operator: node.Operator, + Left: node.Left.Clone(), + From: node.From.Clone(), + To: node.To.Clone(), + } +} +func (node *IsExpr) Clone() Expr { + if node == nil { + return nil + } + return &IsExpr{ + Operator: node.Operator, + Expr: node.Expr.Clone(), + } +} +func (node *ExistsExpr) Clone() Expr { + if node == nil { + return nil + } + return &ExistsExpr{ + Subquery: node.Subquery.Clone().(*Subquery), + } +} +func (node *Literal) Clone() Expr { + if node == nil { + return nil + } + return &Literal{} +} +func (node Argument) Clone() Expr { + if node == nil { + return nil + } + cpy := make(Argument, len(node)) + copy(cpy, node) + return cpy +} +func (node *NullVal) Clone() Expr { + if node == nil { + return nil + } + return &NullVal{} +} +func (node BoolVal) Clone() Expr { + return node +} +func (node *ColName) Clone() Expr { + if node == nil { + return nil + } + return &ColName{ + Metadata: node.Metadata, + Name: node.Name, + Qualifier: node.Qualifier, + } +} +func (node ValTuple) Clone() Expr { + if node == nil { + return nil + } + cpy := make(ValTuple, len(node)) + copy(cpy, node) + return cpy +} +func (node ListArg) Clone() Expr { + if node == nil { + return nil + } + cpy := make(ListArg, len(node)) + copy(cpy, node) + return cpy +} +func (node *BinaryExpr) Clone() Expr { + if node == nil { + return nil + } + return &BinaryExpr{ + Operator: node.Operator, + Left: node.Left.Clone(), + Right: node.Right.Clone(), + } +} +func (node *UnaryExpr) Clone() Expr { + if node == nil { + return nil + } + return &UnaryExpr{ + Operator: node.Operator, + Expr: node.Expr.Clone(), + } +} +func (node *IntervalExpr) Clone() Expr { + if node == nil { + return nil + } + return &IntervalExpr{ + Expr: node.Expr.Clone(), + Unit: node.Unit, + } +} +func (node *CollateExpr) Clone() Expr { + if node == nil { + return nil + } + return &CollateExpr{ + Expr: node.Expr.Clone(), + Charset: node.Charset, + } +} +func (node *FuncExpr) Clone() Expr { + if node == nil { + return nil + } + panic(1) +} +func (node *TimestampFuncExpr) Clone() Expr { + if node == nil { + return nil + } + return &TimestampFuncExpr{ + Name: node.Name, + Expr1: node.Expr1.Clone(), + Expr2: node.Expr2.Clone(), + Unit: node.Unit, + } +} +func (node *CurTimeFuncExpr) Clone() Expr { + if node == nil { + return nil + } + return &CurTimeFuncExpr{ + Name: node.Name, + Fsp: node.Fsp.Clone(), + } +} +func (node *CaseExpr) Clone() Expr { + if node == nil { + return nil + } + panic(1) +} +func (node *ValuesFuncExpr) Clone() Expr { + if node == nil { + return nil + } + return &ValuesFuncExpr{ + Name: node.Name.Clone().(*ColName), + } +} +func (node *ConvertExpr) Clone() Expr { + if node == nil { + return nil + } + panic(1) +} +func (node *SubstrExpr) Clone() Expr { + if node == nil { + return nil + } + return &SubstrExpr{ + Name: node.Name, + StrVal: node.StrVal.Clone().(*Literal), + From: node.From.Clone(), + To: node.To.Clone(), + } +} +func (node *ConvertUsingExpr) Clone() Expr { + if node == nil { + return nil + } + return &ConvertUsingExpr{ + Expr: node.Expr.Clone(), + Type: node.Type, + } +} +func (node *MatchExpr) Clone() Expr { + if node == nil { + return nil + } + panic(1) +} +func (node *GroupConcatExpr) Clone() Expr { + if node == nil { + return nil + } + + panic(1) +} +func (node *Default) Clone() Expr { + if node == nil { + return nil + } + return &Default{ColName: node.ColName} +} From 22973c1706ed5ed26c4c42031d4da800cf72be89 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Sun, 10 Jan 2021 12:43:53 +0100 Subject: [PATCH 04/13] clean up predicate adding to joinTrees Signed-off-by: Andres Taylor --- go/vt/vtgate/planbuilder/plan_test.go | 4 +-- go/vt/vtgate/planbuilder/route_planning.go | 31 +++++++++---------- .../vtgate/planbuilder/route_planning_test.go | 2 +- 3 files changed, 18 insertions(+), 19 deletions(-) diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index d3d0affb694..45e7619fe1b 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -396,7 +396,7 @@ func testFile(t *testing.T, filename, tempDir string, vschema *vschemaWrapper, c expected := &strings.Builder{} fail := checkAllTests for tcase := range iterateExecFile(filename) { - t.Run(tcase.comments, func(t *testing.T) { + t.Run(fmt.Sprintf("%d V3: %s", tcase.lineno, tcase.comments), func(t *testing.T) { vschema.version = V3 plan, err := TestBuilder(tcase.input, vschema) out := getPlanOrErrorOutput(err, plan) @@ -431,7 +431,7 @@ func testFile(t *testing.T, filename, tempDir string, vschema *vschemaWrapper, c // with this last expectation, it is an error if the V4 planner // produces the same plan as the V3 planner does if !empty || checkAllTests { - t.Run("V4: "+tcase.comments, func(t *testing.T) { + t.Run(fmt.Sprintf("%d V4: %s", tcase.lineno, tcase.comments), func(t *testing.T) { if out != tcase.output2ndPlanner { fail = true t.Errorf("V4 - %s:%d\nDiff:\n%s\n[%s] \n[%s]", filename, tcase.lineno, cmp.Diff(tcase.output2ndPlanner, out), tcase.output, out) diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index 8ae31f8ab17..22bb048c95d 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -123,8 +123,8 @@ type ( // the tables also contain any predicates that only depend on that particular table tables routeTables - // extraPredicates are the predicates that depend on multiple tables - extraPredicates []sqlparser.Expr + // predicates are the predicates evaluated by this plan + predicates []sqlparser.Expr // vindex and conditions is set if a vindex will be used for this route. vindex vindexes.Vindex @@ -132,7 +132,7 @@ type ( // here we store the possible vindexes we can use so that when we add predicates to the plan, // we can quickly check if the new predicates enables any new vindex options - vindexPreds []vindexPlusPredicates + vindexPreds []*vindexPlusPredicates } joinPlan struct { predicates []sqlparser.Expr @@ -145,8 +145,12 @@ type ( // so changing the the contents of them will not be reflected in the original func (rp *routePlan) clone() joinTree { result := *rp - result.vindexPreds = make([]vindexPlusPredicates, len(rp.vindexPreds)) - copy(result.vindexPreds, rp.vindexPreds) + result.vindexPreds = make([]*vindexPlusPredicates, len(rp.vindexPreds)) + for i, pred := range rp.vindexPreds { + // we do this to create a copy of the struct + p := *pred + result.vindexPreds[i] = &p + } return &result } @@ -216,7 +220,7 @@ func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { v.predicates = append(v.predicates, node) // Vindex is covered if all the columns in the vindex have a associated predicate v.covered = len(v.predicates) == len(v.vindex.Columns) - newVindexFound = true + newVindexFound = newVindexFound || v.covered } } } @@ -230,7 +234,7 @@ func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { } // any predicates that cover more than a single table need to be added here - rp.extraPredicates = append(rp.extraPredicates, predicates...) + rp.predicates = append(rp.predicates, predicates...) return nil } @@ -270,12 +274,7 @@ func (rp *routePlan) Predicates() sqlparser.Expr { Right: e, } } - for _, t := range rp.tables { - for _, predicate := range t.qtable.predicates { - add(predicate) - } - } - for _, p := range rp.extraPredicates { + for _, p := range rp.predicates { add(p) } return result @@ -499,7 +498,7 @@ func createRoutePlan(table *queryTable, solves semantics.TableSet, vschema Conte } for _, columnVindex := range vschemaTable.ColumnVindexes { - plan.vindexPreds = append(plan.vindexPreds, vindexPlusPredicates{vindex: columnVindex}) + plan.vindexPreds = append(plan.vindexPreds, &vindexPlusPredicates{vindex: columnVindex}) } switch { @@ -598,8 +597,8 @@ func tryMerge(a, b joinTree, joinPredicates []sqlparser.Expr, semTable *semantic routeOpCode: aRoute.routeOpCode, solved: newTabletSet, tables: append(aRoute.tables, bRoute.tables...), - extraPredicates: append( - append(aRoute.extraPredicates, bRoute.extraPredicates...), + predicates: append( + append(aRoute.predicates, bRoute.predicates...), joinPredicates...), keyspace: aRoute.keyspace, vindexPreds: append(aRoute.vindexPreds, bRoute.vindexPreds...), diff --git a/go/vt/vtgate/planbuilder/route_planning_test.go b/go/vt/vtgate/planbuilder/route_planning_test.go index aaff9c75260..535aecd0f2a 100644 --- a/go/vt/vtgate/planbuilder/route_planning_test.go +++ b/go/vt/vtgate/planbuilder/route_planning_test.go @@ -108,7 +108,7 @@ func TestMergeJoins(t *testing.T) { func TestClone(t *testing.T) { original := &routePlan{ routeOpCode: engine.SelectEqualUnique, - vindexPreds: []vindexPlusPredicates{{ + vindexPreds: []*vindexPlusPredicates{{ covered: false, }}, } From fca46a716f89855686cd40123e27312c7bc83541 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Sun, 10 Jan 2021 12:50:22 +0100 Subject: [PATCH 05/13] updated test expectations Signed-off-by: Andres Taylor --- .../planbuilder/testdata/filter_cases.txt | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt index 126602ac3d7..d05474045ea 100644 --- a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt @@ -36,6 +36,19 @@ } } { + "QueryType": "SELECT", + "Original": "select id from user where someColumn = null", + "Instructions": { + "OperatorType": "Route", + "Variant": "SelectNone", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select id from user where 1 != 1", + "Query": "select id from user", + "Table": "user" + } } # Single table unique vindex route @@ -1601,6 +1614,19 @@ } } { + "QueryType": "SELECT", + "Original": "select id from music where id = null", + "Instructions": { + "OperatorType": "Route", + "Variant": "SelectNone", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select id from music where 1 != 1", + "Query": "select id from music", + "Table": "music" + } } # SELECT with IS NULL @@ -1663,6 +1689,19 @@ } } { + "QueryType": "SELECT", + "Original": "select id from music where user_id = 4 and id = null", + "Instructions": { + "OperatorType": "Route", + "Variant": "SelectNone", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select id from music where 1 != 1", + "Query": "select id from music", + "Table": "music" + } } # Single table with unique vindex match and IN (null) From 2c8669548601d19e6af0111c490ec2ed36a8f397 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Sat, 16 Jan 2021 12:35:20 +0100 Subject: [PATCH 06/13] last touch ups of AxB vs BxA Signed-off-by: Andres Taylor --- go/vt/sqlparser/ast_funcs.go | 72 ++++----- .../planbuilder/jointree_transformers.go | 70 ++------- go/vt/vtgate/planbuilder/route_planning.go | 147 +++++++++++++----- 3 files changed, 158 insertions(+), 131 deletions(-) diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index 849eb7144ed..feb5ad66717 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -1298,6 +1298,13 @@ const ( DoubleAt ) +func nilOrClone(in Expr) Expr { + if in == nil { + return nil + } + return in.Clone() +} + // Clone implements the Expr interface func (node *Subquery) Clone() Expr { if node == nil { @@ -1312,8 +1319,8 @@ func (node *AndExpr) Clone() Expr { return nil } return &AndExpr{ - Left: node.Left.Clone(), - Right: node.Right.Clone(), + Left: nilOrClone(node.Left), + Right: nilOrClone(node.Right), } } @@ -1323,8 +1330,8 @@ func (node *OrExpr) Clone() Expr { return nil } return &OrExpr{ - Left: node.Left.Clone(), - Right: node.Right.Clone(), + Left: nilOrClone(node.Left), + Right: nilOrClone(node.Right), } } @@ -1334,8 +1341,8 @@ func (node *XorExpr) Clone() Expr { return nil } return &XorExpr{ - Left: node.Left.Clone(), - Right: node.Right.Clone(), + Left: nilOrClone(node.Left), + Right: nilOrClone(node.Right), } } @@ -1345,7 +1352,7 @@ func (node *NotExpr) Clone() Expr { return nil } return &NotExpr{ - Expr: node.Clone(), + Expr: nilOrClone(node), } } @@ -1356,9 +1363,9 @@ func (node *ComparisonExpr) Clone() Expr { } return &ComparisonExpr{ Operator: node.Operator, - Left: node.Left.Clone(), - Right: node.Right.Clone(), - Escape: node.Escape.Clone(), + Left: nilOrClone(node.Left), + Right: nilOrClone(node.Right), + Escape: nilOrClone(node.Escape), } } @@ -1369,9 +1376,9 @@ func (node *RangeCond) Clone() Expr { } return &RangeCond{ Operator: node.Operator, - Left: node.Left.Clone(), - From: node.From.Clone(), - To: node.To.Clone(), + Left: nilOrClone(node.Left), + From: nilOrClone(node.From), + To: nilOrClone(node.To), } } @@ -1382,7 +1389,7 @@ func (node *IsExpr) Clone() Expr { } return &IsExpr{ Operator: node.Operator, - Expr: node.Expr.Clone(), + Expr: nilOrClone(node.Expr), } } @@ -1392,7 +1399,7 @@ func (node *ExistsExpr) Clone() Expr { return nil } return &ExistsExpr{ - Subquery: node.Subquery.Clone().(*Subquery), + Subquery: nilOrClone(node.Subquery).(*Subquery), } } @@ -1429,14 +1436,7 @@ func (node BoolVal) Clone() Expr { // Clone implements the Expr interface func (node *ColName) Clone() Expr { - if node == nil { - return nil - } - return &ColName{ - Metadata: node.Metadata, - Name: node.Name, - Qualifier: node.Qualifier, - } + return node } // Clone implements the Expr interface @@ -1466,8 +1466,8 @@ func (node *BinaryExpr) Clone() Expr { } return &BinaryExpr{ Operator: node.Operator, - Left: node.Left.Clone(), - Right: node.Right.Clone(), + Left: nilOrClone(node.Left), + Right: nilOrClone(node.Right), } } @@ -1478,7 +1478,7 @@ func (node *UnaryExpr) Clone() Expr { } return &UnaryExpr{ Operator: node.Operator, - Expr: node.Expr.Clone(), + Expr: nilOrClone(node.Expr), } } @@ -1488,7 +1488,7 @@ func (node *IntervalExpr) Clone() Expr { return nil } return &IntervalExpr{ - Expr: node.Expr.Clone(), + Expr: nilOrClone(node.Expr), Unit: node.Unit, } } @@ -1499,7 +1499,7 @@ func (node *CollateExpr) Clone() Expr { return nil } return &CollateExpr{ - Expr: node.Expr.Clone(), + Expr: nilOrClone(node.Expr), Charset: node.Charset, } } @@ -1519,8 +1519,8 @@ func (node *TimestampFuncExpr) Clone() Expr { } return &TimestampFuncExpr{ Name: node.Name, - Expr1: node.Expr1.Clone(), - Expr2: node.Expr2.Clone(), + Expr1: nilOrClone(node.Expr1), + Expr2: nilOrClone(node.Expr2), Unit: node.Unit, } } @@ -1532,7 +1532,7 @@ func (node *CurTimeFuncExpr) Clone() Expr { } return &CurTimeFuncExpr{ Name: node.Name, - Fsp: node.Fsp.Clone(), + Fsp: nilOrClone(node.Fsp), } } @@ -1550,7 +1550,7 @@ func (node *ValuesFuncExpr) Clone() Expr { return nil } return &ValuesFuncExpr{ - Name: node.Name.Clone().(*ColName), + Name: nilOrClone(node.Name).(*ColName), } } @@ -1569,9 +1569,9 @@ func (node *SubstrExpr) Clone() Expr { } return &SubstrExpr{ Name: node.Name, - StrVal: node.StrVal.Clone().(*Literal), - From: node.From.Clone(), - To: node.To.Clone(), + StrVal: nilOrClone(node.StrVal).(*Literal), + From: nilOrClone(node.From), + To: nilOrClone(node.To), } } @@ -1581,7 +1581,7 @@ func (node *ConvertUsingExpr) Clone() Expr { return nil } return &ConvertUsingExpr{ - Expr: node.Expr.Clone(), + Expr: nilOrClone(node.Expr), Type: node.Type, } } diff --git a/go/vt/vtgate/planbuilder/jointree_transformers.go b/go/vt/vtgate/planbuilder/jointree_transformers.go index 1402d5c21f1..6a5659634f5 100644 --- a/go/vt/vtgate/planbuilder/jointree_transformers.go +++ b/go/vt/vtgate/planbuilder/jointree_transformers.go @@ -22,13 +22,12 @@ import ( "vitess.io/vitess/go/sqltypes" vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" + "vitess.io/vitess/go/vt/sqlparser" + "vitess.io/vitess/go/vt/vtgate/engine" "vitess.io/vitess/go/vt/vtgate/semantics" "vitess.io/vitess/go/vt/vtgate/vindexes" "vitess.io/vitess/go/vt/vterrors" - - "vitess.io/vitess/go/vt/sqlparser" - "vitess.io/vitess/go/vt/vtgate/engine" ) func transformToLogicalPlan(tree joinTree, semTable *semantics.SemTable) (logicalPlan, error) { @@ -43,46 +42,20 @@ func transformToLogicalPlan(tree joinTree, semTable *semantics.SemTable) (logica return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "BUG: unknown type encountered: %T", tree) } -func transformJoinPlan(n *joinPlan, semTable *semantics.SemTable) (*joinV4, error) { - lhsColList := extractColumnsNeededFromLHS(n, semTable, n.lhs.tables()) - - var lhsColExpr []*sqlparser.AliasedExpr - for _, col := range lhsColList { - lhsColExpr = append(lhsColExpr, &sqlparser.AliasedExpr{ - Expr: col, - }) - } - +func transformJoinPlan(n *joinPlan, semTable *semantics.SemTable) (logicalPlan, error) { lhs, err := transformToLogicalPlan(n.lhs, semTable) if err != nil { return nil, err } - offset, err := pushProjection(lhsColExpr, lhs, semTable) - if err != nil { - return nil, err - } - - vars := map[string]int{} - - for _, col := range lhsColList { - vars[col.CompliantName("")] = offset - offset++ - } - rhs, err := transformToLogicalPlan(n.rhs, semTable) if err != nil { return nil, err } - - err = pushPredicate(n.predicates, rhs, semTable) - if err != nil { - return nil, err - } - return &joinV4{ Left: lhs, Right: rhs, - Vars: vars, + Cols: n.columns, + Vars: n.vars, }, nil } @@ -122,6 +95,11 @@ func transformRoutePlan(n *routePlan) (*route, error) { singleColumn = n.vindex.(vindexes.SingleColumn) } + var expressions sqlparser.SelectExprs + for _, col := range n.columns { + expressions = append(expressions, &sqlparser.AliasedExpr{Expr: col}) + } + var tableNames []string for name := range tableNameMap { tableNames = append(tableNames, name) @@ -137,32 +115,10 @@ func transformRoutePlan(n *routePlan) (*route, error) { Values: values, }, Select: &sqlparser.Select{ - From: tablesForSelect, - Where: where, + SelectExprs: expressions, + From: tablesForSelect, + Where: where, }, tables: n.solved, }, nil } - -func extractColumnsNeededFromLHS(n *joinPlan, semTable *semantics.SemTable, lhsSolves semantics.TableSet) []*sqlparser.ColName { - lhsColMap := map[*sqlparser.ColName]sqlparser.Argument{} - for _, predicate := range n.predicates { - sqlparser.Rewrite(predicate, func(cursor *sqlparser.Cursor) bool { - switch node := cursor.Node().(type) { - case *sqlparser.ColName: - if semTable.Dependencies(node).IsSolvedBy(lhsSolves) { - arg := sqlparser.NewArgument([]byte(":" + node.CompliantName(""))) - lhsColMap[node] = arg - cursor.Replace(arg) - } - } - return true - }, nil) - } - - var lhsColList []*sqlparser.ColName - for col := range lhsColMap { - lhsColList = append(lhsColList, col) - } - return lhsColList -} diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index 9bbfece72b5..0feb0f60cfa 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -109,6 +109,8 @@ type ( // creates a copy of the joinTree that can be updated without changing the original clone() joinTree + + pushOutputColumns([]*sqlparser.ColName, *semantics.SemTable) int } routeTable struct { qtable *queryTable @@ -133,14 +135,25 @@ type ( // here we store the possible vindexes we can use so that when we add predicates to the plan, // we can quickly check if the new predicates enables any new vindex options vindexPreds []*vindexPlusPredicates + + // columns needed to feed other plans + columns []*sqlparser.ColName } joinPlan struct { - predicates []sqlparser.Expr - lhs, rhs joinTree + // columns needed to feed other plans + columns []int + + // arguments that need to be copied from the LHS/RHS + vars map[string]int + + lhs, rhs joinTree } routeTables []*routeTable ) +var _ joinTree = (*routePlan)(nil) +var _ joinTree = (*joinPlan)(nil) + // clone returns a copy of the struct with copies of slices, // so changing the the contents of them will not be reflected in the original func (rp *routePlan) clone() joinTree { @@ -213,14 +226,19 @@ func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { // already covered by an earlier predicate continue } - column := node.Left.(*sqlparser.ColName) - for _, col := range v.vindex.Columns { - // If the column for the predicate matches any column in the vindex add it to the list - if column.Name.Equal(col) { - v.predicates = append(v.predicates, node) - // Vindex is covered if all the columns in the vindex have a associated predicate - v.covered = len(v.predicates) == len(v.vindex.Columns) - newVindexFound = newVindexFound || v.covered + column, ok := node.Left.(*sqlparser.ColName) + if !ok { + column, ok = node.Right.(*sqlparser.ColName) + } + if ok { + for _, col := range v.vindex.Columns { + // If the column for the predicate matches any column in the vindex add it to the list + if column.Name.Equal(col) { + v.predicates = append(v.predicates, node) + // Vindex is covered if all the columns in the vindex have a associated predicate + v.covered = len(v.predicates) == len(v.vindex.Columns) + newVindexFound = newVindexFound || v.covered + } } } } @@ -280,22 +298,56 @@ func (rp *routePlan) Predicates() sqlparser.Expr { return result } +func (rp *routePlan) pushOutputColumns(col []*sqlparser.ColName, _ *semantics.SemTable) int { + newCol := len(rp.columns) + rp.columns = append(rp.columns, col...) + return newCol +} + func (jp *joinPlan) tables() semantics.TableSet { return jp.lhs.tables() | jp.rhs.tables() } + func (jp *joinPlan) cost() int { return jp.lhs.cost() + jp.rhs.cost() } + func (jp *joinPlan) clone() joinTree { result := &joinPlan{ - predicates: make([]sqlparser.Expr, len(jp.predicates)), - lhs: jp.lhs.clone(), - rhs: jp.rhs.clone(), + lhs: jp.lhs.clone(), + rhs: jp.rhs.clone(), } - copy(result.predicates, jp.predicates) return result } +func (jp *joinPlan) pushOutputColumns(columns []*sqlparser.ColName, semTable *semantics.SemTable) int { + resultIdx := len(jp.columns) + var toTheLeft []bool + var lhs, rhs []*sqlparser.ColName + for _, col := range columns { + if semTable.Dependencies(col).IsSolvedBy(jp.lhs.tables()) { + lhs = append(lhs, col) + toTheLeft = append(toTheLeft, true) + } else { + rhs = append(rhs, col) + toTheLeft = append(toTheLeft, false) + } + } + lhsOffset := jp.lhs.pushOutputColumns(lhs, semTable) + rhsOffset := -jp.rhs.pushOutputColumns(rhs, semTable) + + for _, left := range toTheLeft { + if left { + jp.columns = append(jp.columns, lhsOffset) + lhsOffset++ + } else { + jp.columns = append(jp.columns, rhsOffset) + rhsOffset-- + } + } + return resultIdx +} + func pushPredicate2(exprs []sqlparser.Expr, tree joinTree, semTable *semantics.SemTable) (joinTree, error) { switch node := tree.(type) { case *routePlan: @@ -307,48 +359,61 @@ func pushPredicate2(exprs []sqlparser.Expr, tree joinTree, semTable *semantics.S return plan, nil case *joinPlan: - // we need to figure out which predicates go where - var lhsPreds, rhsPreds []sqlparser.Expr + // we break up the predicates so that colnames from the LHS are replaced by arguments + var rhsPreds []sqlparser.Expr + var lhsColumns []*sqlparser.ColName lhsSolves := node.lhs.tables() - rhsSolves := node.rhs.tables() for _, expr := range exprs { - // TODO: we should not do this. - // we should instead do the same as go/vt/vtgate/planbuilder/jointree_transformers.go:46 - deps := semTable.Dependencies(expr) - switch { - case deps.IsSolvedBy(lhsSolves): - lhsPreds = append(lhsPreds, expr) - case deps.IsSolvedBy(rhsSolves): - rhsPreds = append(rhsPreds, expr) - default: - return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "unknown dependencies for %s", sqlparser.String(expr)) + cols, predicate, err := breakPredicateInLHSandRHS(expr, semTable, lhsSolves) + if err != nil { + return nil, err } + lhsColumns = append(lhsColumns, cols...) + rhsPreds = append(rhsPreds, predicate) } - lhsPlan, err := pushPredicate2(lhsPreds, node.lhs, semTable) - if err != nil { - return nil, err - } + node.pushOutputColumns(lhsColumns, semTable) rhsPlan, err := pushPredicate2(rhsPreds, node.rhs, semTable) if err != nil { return nil, err } return &joinPlan{ - predicates: exprs, - lhs: lhsPlan, - rhs: rhsPlan, + lhs: node.lhs, + rhs: rhsPlan, }, nil default: panic(fmt.Sprintf("BUG: unknown type %T", node)) } } +func breakPredicateInLHSandRHS(expr sqlparser.Expr, semTable *semantics.SemTable, lhs semantics.TableSet) (columns []*sqlparser.ColName, predicate sqlparser.Expr, err error) { + predicate = expr.Clone() + sqlparser.Rewrite(predicate, nil, func(cursor *sqlparser.Cursor) bool { + switch node := cursor.Node().(type) { + case *sqlparser.ColName: + deps := semTable.Dependencies(node) + if deps == 0 { + err = vterrors.Errorf(vtrpcpb.Code_INTERNAL, "unknown column. has the AST been copied?") + return false + } + if deps.IsSolvedBy(lhs) { + columns = append(columns, node) + arg := sqlparser.NewArgument([]byte(":" + node.CompliantName(""))) + cursor.Replace(arg) + } + } + return true + }) + return +} + func mergeOrJoin(lhs, rhs joinTree, joinPredicates []sqlparser.Expr, semTable *semantics.SemTable) (joinTree, error) { newPlan := tryMerge(lhs, rhs, joinPredicates, semTable) if newPlan != nil { return newPlan, nil } - return pushPredicate2(joinPredicates, &joinPlan{lhs: lhs, rhs: rhs}, semTable) + tree := &joinPlan{lhs: lhs.clone(), rhs: rhs.clone()} + return pushPredicate2(joinPredicates, tree, semTable) } type ( @@ -377,10 +442,16 @@ func greedySolve(qg *queryGraph, semTable *semantics.SemTable, vschema ContextVS if err != nil { return nil, err } + // if we found a best plan, we'll replace the two plans that were joined with the join plan created if bestTree != nil { - // if we found a best plan, we'll replace the two joinTrees that were joined with the join plan created - joinTrees = removeAt(joinTrees, rIdx) - joinTrees = removeAt(joinTrees, lIdx) + // we need to remove the larger of the two plans first + if rIdx > lIdx { + joinTrees = removeAt(joinTrees, rIdx) + joinTrees = removeAt(joinTrees, lIdx) + } else { + joinTrees = removeAt(joinTrees, lIdx) + joinTrees = removeAt(joinTrees, rIdx) + } joinTrees = append(joinTrees, bestTree) } else { // we will only fail to find a join plan when there are only cross joins left From f38b2afd1085572d5fcac45bb7cc0acba33c5e95 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Sat, 16 Jan 2021 14:51:14 +0100 Subject: [PATCH 07/13] clean up predicates Signed-off-by: Andres Taylor --- go/vt/sqlparser/ast_funcs.go | 18 ++++++++ .../planbuilder/jointree_transformers.go | 6 ++- go/vt/vtgate/planbuilder/route.go | 10 ++++ .../planbuilder/testdata/filter_cases.txt | 36 +++++++++++++++ .../planbuilder/testdata/from_cases.txt | 46 +++++++++++-------- .../planbuilder/testdata/large_cases.txt | 28 +++++------ 6 files changed, 111 insertions(+), 33 deletions(-) diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index feb5ad66717..2375f16c85f 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -1610,3 +1610,21 @@ func (node *Default) Clone() Expr { } return &Default{ColName: node.ColName} } + +// OtherSideOfColumnExpression returns the side of the comparison that is not ColName expression +func (node *ComparisonExpr) OtherSideOfColumnExpression() (Expr, error) { + _, lok := node.Left.(*ColName) + _, rok := node.Right.(*ColName) + if lok && rok { + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "both sides are columns") + } + if !lok && !rok { + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "neither side is a column") + } + + if lok { + return node.Right, nil + } + + return node.Left, nil +} diff --git a/go/vt/vtgate/planbuilder/jointree_transformers.go b/go/vt/vtgate/planbuilder/jointree_transformers.go index 6a5659634f5..5eb52e7c3b4 100644 --- a/go/vt/vtgate/planbuilder/jointree_transformers.go +++ b/go/vt/vtgate/planbuilder/jointree_transformers.go @@ -84,7 +84,11 @@ func transformRoutePlan(n *routePlan) (*route, error) { } var values []sqltypes.PlanValue if len(n.conditions) == 1 { - value, err := sqlparser.NewPlanValue(n.conditions[0].(*sqlparser.ComparisonExpr).Right) + expr, err := n.conditions[0].(*sqlparser.ComparisonExpr).OtherSideOfColumnExpression() + if err != nil { + return nil, err + } + value, err := sqlparser.NewPlanValue(expr) if err != nil { return nil, err } diff --git a/go/vt/vtgate/planbuilder/route.go b/go/vt/vtgate/planbuilder/route.go index 2a357845ec5..6998a16b313 100644 --- a/go/vt/vtgate/planbuilder/route.go +++ b/go/vt/vtgate/planbuilder/route.go @@ -237,6 +237,16 @@ func (rb *route) prepareTheAST() { }, } } + case *sqlparser.ComparisonExpr: + // 42 = colName -> colName = 42 + b := node.Operator == sqlparser.EqualOp + value := sqlparser.IsValue(node.Left) + name := sqlparser.IsColName(node.Right) + if b && + value && + name { + node.Left, node.Right = node.Right, node.Left + } } return true, nil }, rb.Select) diff --git a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt index d05474045ea..c6d0cd61301 100644 --- a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt @@ -1012,6 +1012,42 @@ } } { + "QueryType": "SELECT", + "Original": "select unsharded.id from user join unsharded where unsharded.id = user.id", + "Instructions": { + "OperatorType": "Join", + "Variant": "Join", + "JoinColumnIndexes": "-2", + "TableName": "unsharded_user", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "SelectUnsharded", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "FieldQuery": "select unsharded.id, unsharded.id from unsharded where 1 != 1", + "Query": "select unsharded.id, unsharded.id from unsharded", + "Table": "unsharded" + }, + { + "OperatorType": "Route", + "Variant": "SelectEqualUnique", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select 1 from user where 1 != 1", + "Query": "select 1 from user where user.id = :unsharded_id", + "Table": "user", + "Values": [ + ":unsharded_id" + ], + "Vindex": "user_index" + } + ] + } } # routing rules: choose the redirected table diff --git a/go/vt/vtgate/planbuilder/testdata/from_cases.txt b/go/vt/vtgate/planbuilder/testdata/from_cases.txt index 9305de7fe24..db447c9493b 100644 --- a/go/vt/vtgate/planbuilder/testdata/from_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/from_cases.txt @@ -1163,6 +1163,8 @@ ] } } +{ +} # sharded join, non-vindex col "select user.col from user join user_extra on user.id = user_extra.col" @@ -1206,8 +1208,8 @@ "Instructions": { "OperatorType": "Join", "Variant": "Join", - "JoinColumnIndexes": "-2", - "TableName": "user_user_extra", + "JoinColumnIndexes": "1", + "TableName": "user_extra_user", "Inputs": [ { "OperatorType": "Route", @@ -1216,20 +1218,24 @@ "Name": "user", "Sharded": true }, - "FieldQuery": "select user.id, user.col from user where 1 != 1", - "Query": "select user.id, user.col from user", - "Table": "user" + "FieldQuery": "select user_extra.col from user_extra where 1 != 1", + "Query": "select user_extra.col from user_extra", + "Table": "user_extra" }, { "OperatorType": "Route", - "Variant": "SelectScatter", + "Variant": "SelectEqualUnique", "Keyspace": { "Name": "user", "Sharded": true }, - "FieldQuery": "select 1 from user_extra where 1 != 1", - "Query": "select 1 from user_extra where user_extra.col = :user_id", - "Table": "user_extra" + "FieldQuery": "select user.col from user where 1 != 1", + "Query": "select user.col from user where user.id = :user_extra_col", + "Table": "user", + "Values": [ + ":user_extra_col" + ], + "Vindex": "user_index" } ] } @@ -2409,8 +2415,8 @@ "Instructions": { "OperatorType": "Join", "Variant": "Join", - "JoinColumnIndexes": "-2", - "TableName": "user_user_extra", + "JoinColumnIndexes": "1", + "TableName": "user_extra_user", "Inputs": [ { "OperatorType": "Route", @@ -2419,20 +2425,24 @@ "Name": "user", "Sharded": true }, - "FieldQuery": "select user.id, user.id from user where 1 != 1", - "Query": "select user.id, user.id from user", - "Table": "user" + "FieldQuery": "select user_extra.id from user_extra where 1 != 1", + "Query": "select user_extra.id from user_extra", + "Table": "user_extra" }, { "OperatorType": "Route", - "Variant": "SelectScatter", + "Variant": "SelectEqualUnique", "Keyspace": { "Name": "user", "Sharded": true }, - "FieldQuery": "select 1 from user_extra where 1 != 1", - "Query": "select 1 from user_extra where user_extra.id = :user_id", - "Table": "user_extra" + "FieldQuery": "select user.id from user where 1 != 1", + "Query": "select user.id from user where user.id = :user_extra_id", + "Table": "user", + "Values": [ + ":user_extra_id" + ], + "Vindex": "user_index" } ] } diff --git a/go/vt/vtgate/planbuilder/testdata/large_cases.txt b/go/vt/vtgate/planbuilder/testdata/large_cases.txt index 3ec24ac86c6..064a60a4792 100644 --- a/go/vt/vtgate/planbuilder/testdata/large_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/large_cases.txt @@ -178,24 +178,24 @@ "OperatorType": "Join", "Variant": "Join", "JoinColumnIndexes": "1", - "TableName": "unsharded, unsharded_a, unsharded_auto, unsharded_b_user, user_extra, user_metadata_music, music_extra", + "TableName": "music, music_extra_user, user_extra, user_metadata_unsharded, unsharded_a, unsharded_auto, unsharded_b", "Inputs": [ { "OperatorType": "Route", - "Variant": "SelectUnsharded", + "Variant": "SelectScatter", "Keyspace": { - "Name": "main", - "Sharded": false + "Name": "user", + "Sharded": true }, - "FieldQuery": "select 1 from unsharded, unsharded_a, unsharded_b, unsharded_auto where 1 != 1", - "Query": "select 1 from unsharded, unsharded_a, unsharded_b, unsharded_auto where unsharded.x = unsharded_a.y", - "Table": "unsharded, unsharded_a, unsharded_auto, unsharded_b" + "FieldQuery": "select 1 from music, music_extra where 1 != 1", + "Query": "select 1 from music, music_extra where music.id = music_extra.music_id", + "Table": "music, music_extra" }, { "OperatorType": "Join", "Variant": "Join", "JoinColumnIndexes": "-1", - "TableName": "user, user_extra, user_metadata_music, music_extra", + "TableName": "user, user_extra, user_metadata_unsharded, unsharded_a, unsharded_auto, unsharded_b", "Inputs": [ { "OperatorType": "Route", @@ -210,14 +210,14 @@ }, { "OperatorType": "Route", - "Variant": "SelectScatter", + "Variant": "SelectUnsharded", "Keyspace": { - "Name": "user", - "Sharded": true + "Name": "main", + "Sharded": false }, - "FieldQuery": "select 1 from music, music_extra where 1 != 1", - "Query": "select 1 from music, music_extra where music.id = music_extra.music_id", - "Table": "music, music_extra" + "FieldQuery": "select 1 from unsharded, unsharded_a, unsharded_b, unsharded_auto where 1 != 1", + "Query": "select 1 from unsharded, unsharded_a, unsharded_b, unsharded_auto where unsharded.x = unsharded_a.y", + "Table": "unsharded, unsharded_a, unsharded_auto, unsharded_b" } ] } From a46dbdb0bde1a8c29748ff204e77c18e3360ddbb Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Sat, 16 Jan 2021 17:01:01 +0100 Subject: [PATCH 08/13] Only use vindex for valid values When picking the vindex to use, we must also check that the value it is reading from can be used by vtgate for this purpose - the PlanValue must support the expression type. Signed-off-by: Andres Taylor --- go/vt/sqlparser/ast_funcs.go | 18 ----------- .../planbuilder/jointree_transformers.go | 16 ++-------- go/vt/vtgate/planbuilder/route_planning.go | 30 ++++++++++++++----- .../planbuilder/testdata/filter_cases.txt | 21 +++++++++++++ .../planbuilder/testdata/select_cases.txt | 2 ++ 5 files changed, 47 insertions(+), 40 deletions(-) diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index 2375f16c85f..feb5ad66717 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -1610,21 +1610,3 @@ func (node *Default) Clone() Expr { } return &Default{ColName: node.ColName} } - -// OtherSideOfColumnExpression returns the side of the comparison that is not ColName expression -func (node *ComparisonExpr) OtherSideOfColumnExpression() (Expr, error) { - _, lok := node.Left.(*ColName) - _, rok := node.Right.(*ColName) - if lok && rok { - return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "both sides are columns") - } - if !lok && !rok { - return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "neither side is a column") - } - - if lok { - return node.Right, nil - } - - return node.Left, nil -} diff --git a/go/vt/vtgate/planbuilder/jointree_transformers.go b/go/vt/vtgate/planbuilder/jointree_transformers.go index 5eb52e7c3b4..9235eafef25 100644 --- a/go/vt/vtgate/planbuilder/jointree_transformers.go +++ b/go/vt/vtgate/planbuilder/jointree_transformers.go @@ -20,7 +20,6 @@ import ( "sort" "strings" - "vitess.io/vitess/go/sqltypes" vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" "vitess.io/vitess/go/vt/sqlparser" "vitess.io/vitess/go/vt/vtgate/engine" @@ -82,18 +81,7 @@ func transformRoutePlan(n *routePlan) (*route, error) { if predicates != nil { where = &sqlparser.Where{Expr: predicates, Type: sqlparser.WhereClause} } - var values []sqltypes.PlanValue - if len(n.conditions) == 1 { - expr, err := n.conditions[0].(*sqlparser.ComparisonExpr).OtherSideOfColumnExpression() - if err != nil { - return nil, err - } - value, err := sqlparser.NewPlanValue(expr) - if err != nil { - return nil, err - } - values = []sqltypes.PlanValue{value} - } + var singleColumn vindexes.SingleColumn if n.vindex != nil { singleColumn = n.vindex.(vindexes.SingleColumn) @@ -116,7 +104,7 @@ func transformRoutePlan(n *routePlan) (*route, error) { TableName: strings.Join(tableNames, ", "), Keyspace: n.keyspace, Vindex: singleColumn, - Values: values, + Values: n.vindexValues, }, Select: &sqlparser.Select{ SelectExprs: expressions, diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index 0feb0f60cfa..f2c5e7e65be 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -19,6 +19,9 @@ package planbuilder import ( "fmt" "sort" + "strings" + + "vitess.io/vitess/go/sqltypes" vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" "vitess.io/vitess/go/vt/vtgate/semantics" @@ -128,9 +131,9 @@ type ( // predicates are the predicates evaluated by this plan predicates []sqlparser.Expr - // vindex and conditions is set if a vindex will be used for this route. - vindex vindexes.Vindex - conditions []sqlparser.Expr + // vindex and vindexValues is set if a vindex will be used for this route. + vindex vindexes.Vindex + vindexValues []sqltypes.PlanValue // here we store the possible vindexes we can use so that when we add predicates to the plan, // we can quickly check if the new predicates enables any new vindex options @@ -197,8 +200,8 @@ func (rp *routePlan) cost() int { // vindexPlusPredicates is a struct used to store all the predicates that the vindex can be used to query type vindexPlusPredicates struct { - vindex *vindexes.ColumnVindex - predicates []sqlparser.Expr + vindex *vindexes.ColumnVindex + values []sqltypes.PlanValue // Vindex is covered if all the columns in the vindex have an associated predicate covered bool } @@ -227,16 +230,27 @@ func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { continue } column, ok := node.Left.(*sqlparser.ColName) + other := node.Right if !ok { column, ok = node.Right.(*sqlparser.ColName) + other = node.Left + } + value, err := sqlparser.NewPlanValue(other) + if err != nil { + // if we are unable to create a PlanValue, we can't use a vindex, but we don't have to fail + if strings.Contains(err.Error(), "expression is too complex") { + continue + } + // something else went wrong, return the error + return err } if ok { for _, col := range v.vindex.Columns { // If the column for the predicate matches any column in the vindex add it to the list if column.Name.Equal(col) { - v.predicates = append(v.predicates, node) + v.values = append(v.values, value) // Vindex is covered if all the columns in the vindex have a associated predicate - v.covered = len(v.predicates) == len(v.vindex.Columns) + v.covered = len(v.values) == len(v.vindex.Columns) newVindexFound = newVindexFound || v.covered } } @@ -267,7 +281,7 @@ func (rp *routePlan) pickBestAvailableVindex() { // Choose the minimum cost vindex from the ones which are covered if rp.vindex == nil || v.vindex.Vindex.Cost() < rp.vindex.Cost() { rp.vindex = v.vindex.Vindex - rp.conditions = v.predicates + rp.vindexValues = v.values } } diff --git a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt index c6d0cd61301..8069e112714 100644 --- a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt @@ -92,6 +92,8 @@ "Table": "user" } } +{ +} # Single table multiple unique vindex match "select id from music where id = 5 and user_id = 4" @@ -1607,6 +1609,25 @@ "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" + } +} # database() call in where clause. "select id from user where database()" diff --git a/go/vt/vtgate/planbuilder/testdata/select_cases.txt b/go/vt/vtgate/planbuilder/testdata/select_cases.txt index ead579b8b07..28039055a06 100644 --- a/go/vt/vtgate/planbuilder/testdata/select_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/select_cases.txt @@ -851,6 +851,8 @@ "Table": "user" } } +{ +} # sharded limit offset "select user_id from music order by user_id limit 10, 20" From 19c38037616bb2485591b5cc52083c71d5bd687e Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 25 Jan 2021 13:39:20 +0100 Subject: [PATCH 09/13] nicer panic messages Signed-off-by: Andres Taylor --- go/vt/sqlparser/ast_funcs.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index 970812863e1..bdd00bce729 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -1312,7 +1312,7 @@ func (node *Subquery) Clone() Expr { if node == nil { return nil } - panic(1) + panic("Subquery cloning not supported") } // Clone implements the Expr interface @@ -1511,7 +1511,7 @@ func (node *FuncExpr) Clone() Expr { if node == nil { return nil } - panic(1) + panic("FuncExpr cloning not supported") } // Clone implements the Expr interface @@ -1543,7 +1543,7 @@ func (node *CaseExpr) Clone() Expr { if node == nil { return nil } - panic(1) + panic("CaseExpr cloning not supported") } // Clone implements the Expr interface @@ -1561,7 +1561,7 @@ func (node *ConvertExpr) Clone() Expr { if node == nil { return nil } - panic(1) + panic("ConvertExpr cloning not supported") } // Clone implements the Expr interface @@ -1593,7 +1593,7 @@ func (node *MatchExpr) Clone() Expr { if node == nil { return nil } - panic(1) + panic("MatchExpr cloning not supported") } // Clone implements the Expr interface @@ -1601,8 +1601,7 @@ func (node *GroupConcatExpr) Clone() Expr { if node == nil { return nil } - - panic(1) + panic("GroupConcatExpr cloning not supported") } // Clone implements the Expr interface From dfc53d5dd91cf8e4dc987f3141cefa77b0d4a5d3 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 25 Jan 2021 13:43:59 +0100 Subject: [PATCH 10/13] tweaked planner costs and added TODO Signed-off-by: Andres Taylor --- go/vt/vtgate/planbuilder/route_planning.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index 45ab6c5207f..5752703515a 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -203,13 +203,15 @@ func (rp *routePlan) tables() semantics.TableSet { // cost implements the joinTree interface func (rp *routePlan) cost() int { switch rp.routeOpCode { - case + case // these op codes will never be compared with each other - they are assigned by a rule and not a comparison engine.SelectDBA, - engine.SelectEqualUnique, engine.SelectNext, engine.SelectNone, engine.SelectReference, engine.SelectUnsharded: + return 0 + // TODO revisit these costs when more of the gen4 planner is done + case engine.SelectEqualUnique: return 1 case engine.SelectEqual: return 5 @@ -298,7 +300,6 @@ func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { // pickBestAvailableVindex goes over the available vindexes for this route and picks the best one available. func (rp *routePlan) pickBestAvailableVindex() { - //TODO (Manan,Andres): Improve cost metric for vindexes for _, v := range rp.vindexPreds { if !v.covered { continue From b062f536607a36f2e240475d60536c0709553d39 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 25 Jan 2021 13:51:26 +0100 Subject: [PATCH 11/13] add predicates no matter the opcode Signed-off-by: Andres Taylor --- go/vt/vtgate/planbuilder/route_planning.go | 32 ++++++++++++------- .../planbuilder/testdata/filter_cases.txt | 6 ++-- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index 5752703515a..7003fe1b85b 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -236,6 +236,23 @@ type vindexPlusPredicates struct { // addPredicate clones this routePlan and returns a new one with these predicates added to it. if the predicates can help, // they will improve the routeOpCode func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { + newVindexFound, err := rp.searchForNewVindexes(predicates) + if err != nil { + return err + } + + // if we didn't open up any new vindex options, no need to enter here + if newVindexFound { + rp.pickBestAvailableVindex() + } + + // any predicates that cover more than a single table need to be added here + rp.predicates = append(rp.predicates, predicates...) + + return nil +} + +func (rp *routePlan) searchForNewVindexes(predicates []sqlparser.Expr) (bool, error) { newVindexFound := false for _, filter := range predicates { switch node := filter.(type) { @@ -248,7 +265,7 @@ func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { // since we know that nothing returns true when compared to NULL, // so we can safely bail out here rp.routeOpCode = engine.SelectNone - return nil + return false, nil } // TODO(Manan,Andres): Remove the predicates that are repeated eg. Id=1 AND Id=1 for _, v := range rp.vindexPreds { @@ -269,7 +286,7 @@ func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { continue } // something else went wrong, return the error - return err + return false, err } if ok { for _, col := range v.vindex.Columns { @@ -286,16 +303,7 @@ func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error { } } } - - // if we didn't open up any new vindex options, no need to enter here - if newVindexFound { - rp.pickBestAvailableVindex() - } - - // any predicates that cover more than a single table need to be added here - rp.predicates = append(rp.predicates, predicates...) - - return nil + return newVindexFound, nil } // pickBestAvailableVindex goes over the available vindexes for this route and picks the best one available. diff --git a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt index a8d2c26d272..9625327dd41 100644 --- a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt @@ -45,7 +45,7 @@ Gen4 plan same as above "Sharded": true }, "FieldQuery": "select id from user where 1 != 1", - "Query": "select id from user", + "Query": "select id from user where someColumn = null", "Table": "user" } } @@ -1664,7 +1664,7 @@ Gen4 plan same as above "Sharded": true }, "FieldQuery": "select id from music where 1 != 1", - "Query": "select id from music", + "Query": "select id from music where id = null", "Table": "music" } } @@ -1738,7 +1738,7 @@ Gen4 plan same as above "Sharded": true }, "FieldQuery": "select id from music where 1 != 1", - "Query": "select id from music", + "Query": "select id from music where user_id = 4 and id = null", "Table": "music" } } From 7febcf64f3c0bdb9948aea22296c9ed041126324 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 25 Jan 2021 14:44:07 +0100 Subject: [PATCH 12/13] small optimisation that makes big difference Before optimisation: BenchmarkPlanner/large_cases.txt-v4-16 70786 167749 ns/op 74315 B/op 1618 allocs/op BenchmarkPlanner/large_cases.txt-v4-16 70998 168734 ns/op 74303 B/op 1618 allocs/op After: BenchmarkPlanner/large_cases.txt-v4-16 76375 156411 ns/op 67752 B/op 1376 allocs/op BenchmarkPlanner/large_cases.txt-v4-16 76207 157553 ns/op 67816 B/op 1376 allocs/op Signed-off-by: Andres Taylor --- go/vt/vtgate/planbuilder/querygraph_test.go | 1 - go/vt/vtgate/planbuilder/route_planning_test.go | 2 +- go/vt/vtgate/semantics/semantic_state.go | 12 +++++++++++- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/go/vt/vtgate/planbuilder/querygraph_test.go b/go/vt/vtgate/planbuilder/querygraph_test.go index 71c8c06989f..43717a4a465 100644 --- a/go/vt/vtgate/planbuilder/querygraph_test.go +++ b/go/vt/vtgate/planbuilder/querygraph_test.go @@ -123,7 +123,6 @@ func TestQueryGraph(t *testing.T) { require.NoError(t, err) qgraph, err := createQGFromSelect(tree.(*sqlparser.Select), semTable) require.NoError(t, err) - fmt.Println(qgraph.testString()) assert.Equal(t, tc.output, qgraph.testString()) utils.MustMatch(t, tc.output, qgraph.testString(), "incorrect query graph") }) diff --git a/go/vt/vtgate/planbuilder/route_planning_test.go b/go/vt/vtgate/planbuilder/route_planning_test.go index 535aecd0f2a..4209225bd66 100644 --- a/go/vt/vtgate/planbuilder/route_planning_test.go +++ b/go/vt/vtgate/planbuilder/route_planning_test.go @@ -99,7 +99,7 @@ func TestMergeJoins(t *testing.T) { }} for i, tc := range tests { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - result := tryMerge(tc.l, tc.r, tc.predicates, &semantics.SemTable{}) + result := tryMerge(tc.l, tc.r, tc.predicates, semantics.NewSemTable()) assert.Equal(t, tc.expected, result) }) } diff --git a/go/vt/vtgate/semantics/semantic_state.go b/go/vt/vtgate/semantics/semantic_state.go index 6e770a5c188..f30bb023641 100644 --- a/go/vt/vtgate/semantics/semantic_state.go +++ b/go/vt/vtgate/semantics/semantic_state.go @@ -42,6 +42,11 @@ type ( } ) +// NewSemTable creates a new empty SemTable +func NewSemTable() *SemTable { + return &SemTable{exprDependencies: map[sqlparser.Expr]TableSet{}} +} + // TableSetFor returns the bitmask for this particular tableshoe func (st *SemTable) TableSetFor(t table) TableSet { for idx, t2 := range st.Tables { @@ -54,7 +59,10 @@ func (st *SemTable) TableSetFor(t table) TableSet { // Dependencies return the table dependencies of the expression. func (st *SemTable) Dependencies(expr sqlparser.Expr) TableSet { - var deps TableSet + deps, found := st.exprDependencies[expr] + if found { + return deps + } _ = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { colName, ok := node.(*sqlparser.ColName) @@ -65,6 +73,8 @@ func (st *SemTable) Dependencies(expr sqlparser.Expr) TableSet { return true, nil }, expr) + st.exprDependencies[expr] = deps + return deps } From 7fa31bee70965a88ece071e63a728b0a25ae1443 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Wed, 27 Jan 2021 07:02:00 +0100 Subject: [PATCH 13/13] optimise bitmask operation Signed-off-by: Andres Taylor --- go/vt/vtgate/semantics/semantic_state.go | 12 +++++++----- go/vt/vtgate/semantics/tabletset_test.go | 12 +++++++++++- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/go/vt/vtgate/semantics/semantic_state.go b/go/vt/vtgate/semantics/semantic_state.go index f30bb023641..d6430082ff9 100644 --- a/go/vt/vtgate/semantics/semantic_state.go +++ b/go/vt/vtgate/semantics/semantic_state.go @@ -122,11 +122,13 @@ func (ts TableSet) NumberOfTables() int { // Constituents returns an slice with all the // individual tables in their own TableSet identifier func (ts TableSet) Constituents() (result []TableSet) { - for i := 0; i < 64; i++ { - i2 := TableSet(1 << i) - if ts&i2 == i2 { - result = append(result, i2) - } + mask := ts + + for mask > 0 { + maskLeft := mask & (mask - 1) + constituent := mask ^ maskLeft + mask = maskLeft + result = append(result, constituent) } return } diff --git a/go/vt/vtgate/semantics/tabletset_test.go b/go/vt/vtgate/semantics/tabletset_test.go index c3d1da5b95c..8ff41c57ecb 100644 --- a/go/vt/vtgate/semantics/tabletset_test.go +++ b/go/vt/vtgate/semantics/tabletset_test.go @@ -29,15 +29,25 @@ const ( F3 ) -func TestTableSet(t *testing.T) { +func TestTableSet_IsOverlapping(t *testing.T) { assert.True(t, (F1 | F2).IsOverlapping(F1|F2)) assert.True(t, F1.IsOverlapping(F1|F2)) assert.True(t, (F1 | F2).IsOverlapping(F1)) assert.False(t, F3.IsOverlapping(F1|F2)) assert.False(t, (F1 | F2).IsOverlapping(F3)) +} +func TestTableSet_IsSolvedBy(t *testing.T) { assert.True(t, F1.IsSolvedBy(F1|F2)) assert.False(t, (F1 | F2).IsSolvedBy(F1)) assert.False(t, F3.IsSolvedBy(F1|F2)) assert.False(t, (F1 | F2).IsSolvedBy(F3)) } + +func TestTableSet_Constituents(t *testing.T) { + assert.Equal(t, []TableSet{F1, F2, F3}, (F1 | F2 | F3).Constituents()) + assert.Equal(t, []TableSet{F1, F2}, (F1 | F2).Constituents()) + assert.Equal(t, []TableSet{F1, F3}, (F1 | F3).Constituents()) + assert.Equal(t, []TableSet{F2, F3}, (F2 | F3).Constituents()) + assert.Empty(t, TableSet(0).Constituents()) +}