From 2c3adcf339290ecc83b78637ad177cf8b6ed5f60 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 11 Oct 2021 19:16:18 +0530 Subject: [PATCH 1/4] gen4: support added for ordering on vindex function Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/horizon_planning.go | 18 ++++++++++++------ .../planbuilder/testdata/memory_sort_cases.txt | 1 + 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/go/vt/vtgate/planbuilder/horizon_planning.go b/go/vt/vtgate/planbuilder/horizon_planning.go index 490bb5acbb4..794c7894bc6 100644 --- a/go/vt/vtgate/planbuilder/horizon_planning.go +++ b/go/vt/vtgate/planbuilder/horizon_planning.go @@ -259,11 +259,12 @@ func pushProjection(expr *sqlparser.AliasedExpr, plan logicalPlan, semTable *sem } return 0, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "cannot push projections in ordered aggregates") case *vindexFunc: + colsBefore := len(node.eVindexFunc.Cols) i, err := node.SupplyProjection(expr, reuseCol) if err != nil { return 0, false, err } - return i, true, nil + return i /* col added */, len(node.eVindexFunc.Cols) > colsBefore, nil case *limit: return pushProjection(expr, node.input, semTable, inner, reuseCol) case *distinct: @@ -629,9 +630,10 @@ func (hp *horizonPlanning) planOrderBy(ctx *planningContext, orderExprs []abstra plan.input = newUnderlyingPlan return plan, nil case *simpleProjection: - return hp.createMemorySortPlan(ctx, plan, orderExprs) + return hp.createMemorySortPlan(ctx, plan, orderExprs, true) case *vindexFunc: - return nil, semantics.Gen4NotSupportedF("unsupported: ordering on vindex func") + // This is evaluated at VTGate only, so weight_string function cannot be used. + return hp.createMemorySortPlan(ctx, plan, orderExprs /* useWeightStr */, false) default: return nil, vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "ordering on complex query %T", plan) } @@ -760,7 +762,7 @@ func (hp *horizonPlanning) planOrderByForJoin(ctx *planningContext, orderExprs [ plan.Left = newLeft return plan, nil } - sortPlan, err := hp.createMemorySortPlan(ctx, plan, orderExprs) + sortPlan, err := hp.createMemorySortPlan(ctx, plan, orderExprs, true) if err != nil { return nil, err } @@ -807,7 +809,7 @@ func findExprInOrderedAggr(plan *orderedAggregate, order abstract.OrderBy) (int, return 0, 0, false } -func (hp *horizonPlanning) createMemorySortPlan(ctx *planningContext, plan logicalPlan, orderExprs []abstract.OrderBy) (logicalPlan, error) { +func (hp *horizonPlanning) createMemorySortPlan(ctx *planningContext, plan logicalPlan, orderExprs []abstract.OrderBy, useWeightStr bool) (logicalPlan, error) { primitive := &engine.MemorySort{} ms := &memorySort{ resultsBuilder: resultsBuilder{ @@ -819,7 +821,11 @@ func (hp *horizonPlanning) createMemorySortPlan(ctx *planningContext, plan logic } for _, order := range orderExprs { - offset, weightStringOffset, added, err := wrapAndPushExpr(order.Inner.Expr, order.WeightStrExpr, plan, ctx.semTable) + wsExpr := order.WeightStrExpr + if !useWeightStr { + wsExpr = nil + } + offset, weightStringOffset, added, err := wrapAndPushExpr(order.Inner.Expr, wsExpr, plan, ctx.semTable) if err != nil { return nil, err } diff --git a/go/vt/vtgate/planbuilder/testdata/memory_sort_cases.txt b/go/vt/vtgate/planbuilder/testdata/memory_sort_cases.txt index 761e3689b83..d1a32f7f0ea 100644 --- a/go/vt/vtgate/planbuilder/testdata/memory_sort_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/memory_sort_cases.txt @@ -706,6 +706,7 @@ Gen4 plan same as above ] } } +Gen4 plan same as above # unary expression "select a from user order by binary a desc" From 91917fa5284c08d76e4805b5ccb734f2ad51a128 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Tue, 12 Oct 2021 19:51:29 +0530 Subject: [PATCH 2/4] convert straight_join to join and log as planner warning Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/abstract/operator.go | 2 -- go/vt/vtgate/planbuilder/builder.go | 3 +++ go/vt/vtgate/planbuilder/gen4_planner.go | 4 ++++ go/vt/vtgate/planbuilder/plan_test.go | 3 +++ .../vtgate/planbuilder/testdata/from_cases.txt | 17 ++++++++++++++++- go/vt/vtgate/semantics/analyzer.go | 4 ++++ go/vt/vtgate/semantics/early_rewriter.go | 10 ++++++++-- go/vt/vtgate/semantics/semantic_state.go | 2 ++ go/vt/vtgate/vcursor_impl.go | 14 +++++++++++++- 9 files changed, 53 insertions(+), 6 deletions(-) diff --git a/go/vt/vtgate/planbuilder/abstract/operator.go b/go/vt/vtgate/planbuilder/abstract/operator.go index 8f2cfac35a8..001459ac039 100644 --- a/go/vt/vtgate/planbuilder/abstract/operator.go +++ b/go/vt/vtgate/planbuilder/abstract/operator.go @@ -109,8 +109,6 @@ func getOperatorFromTableExpr(tableExpr sqlparser.TableExpr, semTable *semantics lhs, rhs = rhs, lhs } return &Join{LHS: lhs, RHS: rhs, LeftJoin: true, Predicate: tableExpr.Condition.On}, nil - case sqlparser.StraightJoinType: - return nil, semantics.Gen4NotSupportedF(tableExpr.Join.ToString()) default: return nil, vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: %s", tableExpr.Join.ToString()) } diff --git a/go/vt/vtgate/planbuilder/builder.go b/go/vt/vtgate/planbuilder/builder.go index b9f62bb7469..468face549c 100644 --- a/go/vt/vtgate/planbuilder/builder.go +++ b/go/vt/vtgate/planbuilder/builder.go @@ -63,6 +63,9 @@ type ContextVSchema interface { // that could become a problem if they move to a sharded keyspace WarnUnshardedOnly(format string, params ...interface{}) + // PlannerWarning records warning created during planning. + PlannerWarning(message string) + // ForeignKeyMode returns the foreign_key flag value ForeignKeyMode() string } diff --git a/go/vt/vtgate/planbuilder/gen4_planner.go b/go/vt/vtgate/planbuilder/gen4_planner.go index d990bf3bba2..cf8a026c91f 100644 --- a/go/vt/vtgate/planbuilder/gen4_planner.go +++ b/go/vt/vtgate/planbuilder/gen4_planner.go @@ -78,6 +78,10 @@ func gen4planSQLCalcFoundRows(vschema ContextVSchema, sel *sqlparser.Select, que if err != nil { return nil, err } + if semTable.Warning != "" { + // log it as planning warning. + vschema.PlannerWarning(semTable.Warning) + } plan, err := buildSQLCalcFoundRowsPlan(query, sel, reservedVars, vschema, planSelectGen4) if err != nil { return nil, err diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index f65fca732e2..1f3953573a6 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -437,6 +437,9 @@ type vschemaWrapper struct { version PlannerVersion } +func (vw *vschemaWrapper) PlannerWarning(_ string) { +} + func (vw *vschemaWrapper) ForeignKeyMode() string { return "allow" } diff --git a/go/vt/vtgate/planbuilder/testdata/from_cases.txt b/go/vt/vtgate/planbuilder/testdata/from_cases.txt index aab055e22e7..de0cb57ff30 100644 --- a/go/vt/vtgate/planbuilder/testdata/from_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/from_cases.txt @@ -784,7 +784,7 @@ Gen4 plan same as above } } -# Straight-join +# Straight-join (Gen4 ignores the straight_join hint) "select m1.col from unsharded as m1 straight_join unsharded as m2" { "QueryType": "SELECT", @@ -801,6 +801,21 @@ Gen4 plan same as above "Table": "unsharded" } } +{ + "QueryType": "SELECT", + "Original": "select m1.col from unsharded as m1 straight_join unsharded as m2", + "Instructions": { + "OperatorType": "Route", + "Variant": "SelectUnsharded", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "FieldQuery": "select m1.col from unsharded as m1, unsharded as m2 where 1 != 1", + "Query": "select m1.col from unsharded as m1, unsharded as m2", + "Table": "unsharded" + } +} # Three-way join "select user.col from user join unsharded as m1 join unsharded as m2" diff --git a/go/vt/vtgate/semantics/analyzer.go b/go/vt/vtgate/semantics/analyzer.go index fd2ae5f8405..5708095439e 100644 --- a/go/vt/vtgate/semantics/analyzer.go +++ b/go/vt/vtgate/semantics/analyzer.go @@ -42,6 +42,7 @@ type analyzer struct { inProjection int projErr error + warning string } // newAnalyzer create the semantic analyzer @@ -75,6 +76,7 @@ func Analyze(statement sqlparser.SelectStatement, currentDb string, si SchemaInf semTable := analyzer.newSemTable(statement) semTable.ProjectionErr = analyzer.projErr + semTable.Warning = analyzer.warning return semTable, nil } @@ -126,6 +128,8 @@ func (a *analyzer) analyzeDown(cursor *sqlparser.Cursor) bool { a.setError(err) return true } + // log any warn in rewriting. + a.warning = a.rewriter.warning a.enterProjection(cursor) // this is the visitor going down the tree. Returning false here would just not visit the children diff --git a/go/vt/vtgate/semantics/early_rewriter.go b/go/vt/vtgate/semantics/early_rewriter.go index 613d39c39a5..fcab3a63559 100644 --- a/go/vt/vtgate/semantics/early_rewriter.go +++ b/go/vt/vtgate/semantics/early_rewriter.go @@ -25,8 +25,9 @@ import ( ) type earlyRewriter struct { - scoper *scoper - clause string + scoper *scoper + clause string + warning string } func (r *earlyRewriter) down(cursor *sqlparser.Cursor) error { @@ -59,6 +60,11 @@ func (r *earlyRewriter) down(cursor *sqlparser.Cursor) error { if changed { cursor.ReplaceAndRevisit(selExprs) } + case *sqlparser.JoinTableExpr: + if node.Join == sqlparser.StraightJoinType { + node.Join = sqlparser.NormalJoinType + r.warning = "straight join is converted to normal join" + } case *sqlparser.Order: r.clause = "order clause" case sqlparser.GroupBy: diff --git a/go/vt/vtgate/semantics/semantic_state.go b/go/vt/vtgate/semantics/semantic_state.go index 8c252c2b78a..c140c3b631f 100644 --- a/go/vt/vtgate/semantics/semantic_state.go +++ b/go/vt/vtgate/semantics/semantic_state.go @@ -90,6 +90,8 @@ type ( // ColumnEqualities is used to enable transitive closures // if a == b and b == c then a == c ColumnEqualities map[columnName][]sqlparser.Expr + + Warning string } columnName struct { diff --git a/go/vt/vtgate/vcursor_impl.go b/go/vt/vtgate/vcursor_impl.go index a63a7697948..5f9b6ae2c3a 100644 --- a/go/vt/vtgate/vcursor_impl.go +++ b/go/vt/vtgate/vcursor_impl.go @@ -371,6 +371,7 @@ func (vc *vcursorImpl) TargetString() string { return vc.safeSession.TargetString } +// MaxBufferingRetries is to represent max retries on buffering. const MaxBufferingRetries = 3 func (vc *vcursorImpl) ExecutePrimitive(primitive engine.Primitive, bindVars map[string]*querypb.BindVariable, wantfields bool) (*sqltypes.Result, error) { @@ -746,7 +747,18 @@ func (vc *vcursorImpl) WarnUnshardedOnly(format string, params ...interface{}) { } } -// ForeignKey implements the VCursor interface +// PlannerWarning implements the VCursor interface +func (vc *vcursorImpl) PlannerWarning(message string) { + if message == "" { + return + } + vc.warnings = append(vc.warnings, &querypb.QueryWarning{ + Code: mysql.ERNotSupportedYet, + Message: message, + }) +} + +// ForeignKeyMode implements the VCursor interface func (vc *vcursorImpl) ForeignKeyMode() string { if foreignKeyMode == nil { return "" From db61f7d8997625d02bb30e2449b97663615a3a6b Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Wed, 13 Oct 2021 22:15:01 +0530 Subject: [PATCH 3/4] record plan warnings, added e2e test and unit test Signed-off-by: Harshit Gangal --- go/test/endtoend/vtgate/gen4/gen4_test.go | 22 +++++++++++++++ go/vt/vtgate/executor_select_test.go | 33 +++++++++++++++++++++++ go/vt/vtgate/planbuilder/gen4_planner.go | 9 ++++--- 3 files changed, 60 insertions(+), 4 deletions(-) diff --git a/go/test/endtoend/vtgate/gen4/gen4_test.go b/go/test/endtoend/vtgate/gen4/gen4_test.go index de18e1aa257..bb350f7632a 100644 --- a/go/test/endtoend/vtgate/gen4/gen4_test.go +++ b/go/test/endtoend/vtgate/gen4/gen4_test.go @@ -196,6 +196,28 @@ func TestSubQueries(t *testing.T) { assertMatches(t, conn, `select (select id from t2 order by id limit 1) from t2 order by id limit 2`, `[[INT64(1)] [INT64(1)]]`) } +func TestPlannerWarning(t *testing.T) { + ctx := context.Background() + conn, err := mysql.Connect(ctx, &vtParams) + require.NoError(t, err) + defer conn.Close() + + // straight_join query + _ = checkedExec(t, conn, `select 1 from t1 straight_join t2 on t1.id = t2.id`) + assertMatches(t, conn, `show warnings`, `[[VARCHAR("Warning") UINT16(1235) VARCHAR("straight join is converted to normal join")]]`) + + // execute same query again. + _ = checkedExec(t, conn, `select 1 from t1 straight_join t2 on t1.id = t2.id`) + assertMatches(t, conn, `show warnings`, `[[VARCHAR("Warning") UINT16(1235) VARCHAR("straight join is converted to normal join")]]`) + + // random query to reset the warning. + _ = checkedExec(t, conn, `select 1 from t1`) + + // execute same query again. + _ = checkedExec(t, conn, `select 1 from t1 straight_join t2 on t1.id = t2.id`) + assertMatches(t, conn, `show warnings`, `[[VARCHAR("Warning") UINT16(1235) VARCHAR("straight join is converted to normal join")]]`) +} + func assertMatches(t *testing.T, conn *mysql.Conn, query, expected string) { t.Helper() qr := checkedExec(t, conn, query) diff --git a/go/vt/vtgate/executor_select_test.go b/go/vt/vtgate/executor_select_test.go index e471b91147c..2e33f3930cc 100644 --- a/go/vt/vtgate/executor_select_test.go +++ b/go/vt/vtgate/executor_select_test.go @@ -2675,3 +2675,36 @@ func TestSelectScatterFails(t *testing.T) { _, err = executorExecSession(executor, "select /*vt+ ALLOW_SCATTER */ id from user", nil, sess) require.NoError(t, err) } + +func TestGen4SelectStraightJoin(t *testing.T) { + executor, sbc1, _, _ := createExecutorEnv() + executor.normalize = true + *plannerVersion = "gen4" + defer func() { + // change it back to v3 + *plannerVersion = "v3" + }() + + session := NewSafeSession(&vtgatepb.Session{TargetString: "TestExecutor"}) + query := "select u.id from user u straight_join user2 u2 on u.id = u2.id" + _, err := executor.Execute(context.Background(), + "TestGen4SelectStraightJoin", + session, + query, map[string]*querypb.BindVariable{}, + ) + require.NoError(t, err) + wantQueries := []*querypb.BoundQuery{ + { + Sql: "select u.id from `user` as u, user2 as u2 where u.id = u2.id", + BindVariables: map[string]*querypb.BindVariable{}, + }, + } + wantWarnings := []*querypb.QueryWarning{ + { + Code: 1235, + Message: "straight join is converted to normal join", + }, + } + utils.MustMatch(t, wantQueries, sbc1.Queries) + utils.MustMatch(t, wantWarnings, session.Warnings) +} diff --git a/go/vt/vtgate/planbuilder/gen4_planner.go b/go/vt/vtgate/planbuilder/gen4_planner.go index cf8a026c91f..a804f972b22 100644 --- a/go/vt/vtgate/planbuilder/gen4_planner.go +++ b/go/vt/vtgate/planbuilder/gen4_planner.go @@ -78,10 +78,9 @@ func gen4planSQLCalcFoundRows(vschema ContextVSchema, sel *sqlparser.Select, que if err != nil { return nil, err } - if semTable.Warning != "" { - // log it as planning warning. - vschema.PlannerWarning(semTable.Warning) - } + // record any warning as planner warning. + vschema.PlannerWarning(semTable.Warning) + plan, err := buildSQLCalcFoundRowsPlan(query, sel, reservedVars, vschema, planSelectGen4) if err != nil { return nil, err @@ -124,6 +123,8 @@ func newBuildSelectPlan(selStmt sqlparser.SelectStatement, reservedVars *sqlpars if err != nil { return nil, err } + // record any warning as planner warning. + vschema.PlannerWarning(semTable.Warning) err = queryRewrite(semTable, reservedVars, selStmt) if err != nil { From 137fb9a8b8e47fd759209e6e64cafc60f4bd9612 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 14 Oct 2021 16:34:41 +0200 Subject: [PATCH 4/4] minor cleanups Signed-off-by: Andres Taylor --- go/vt/vtgate/semantics/analyzer.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/go/vt/vtgate/semantics/analyzer.go b/go/vt/vtgate/semantics/analyzer.go index 5708095439e..0dd65ebe7c0 100644 --- a/go/vt/vtgate/semantics/analyzer.go +++ b/go/vt/vtgate/semantics/analyzer.go @@ -75,8 +75,6 @@ func Analyze(statement sqlparser.SelectStatement, currentDb string, si SchemaInf // Creation of the semantic table semTable := analyzer.newSemTable(statement) - semTable.ProjectionErr = analyzer.projErr - semTable.Warning = analyzer.warning return semTable, nil } @@ -88,6 +86,7 @@ func (a analyzer) newSemTable(statement sqlparser.SelectStatement) *SemTable { Tables: a.tables.Tables, selectScope: a.scoper.rScope, ProjectionErr: a.projErr, + Warning: a.warning, Comments: statement.GetComments(), SubqueryMap: a.binder.subqueryMap, SubqueryRef: a.binder.subqueryRef,