From 8312ad912cb35b21adfe5e339244901fc343f2dd Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Wed, 18 May 2022 12:04:42 +0200 Subject: [PATCH 1/8] refactor: move HAVING with aggregation to QP Signed-off-by: Andres Taylor --- .../planbuilder/abstract/queryprojection.go | 41 +++++++++++++++++++ go/vt/vtgate/planbuilder/horizon_planning.go | 3 -- go/vt/vtgate/planbuilder/ordered_aggregate.go | 17 -------- .../planbuilder/testdata/aggr_cases.txt | 7 ++++ go/vt/vtgate/planbuilder/testdata/onecase.txt | 6 +++ 5 files changed, 54 insertions(+), 20 deletions(-) diff --git a/go/vt/vtgate/planbuilder/abstract/queryprojection.go b/go/vt/vtgate/planbuilder/abstract/queryprojection.go index f91c3ae5764..0244918c7f6 100644 --- a/go/vt/vtgate/planbuilder/abstract/queryprojection.go +++ b/go/vt/vtgate/planbuilder/abstract/queryprojection.go @@ -169,9 +169,50 @@ func CreateQPFromSelect(sel *sqlparser.Select) (*QueryProjection, error) { qp.groupByExprs = nil } + if sel.Having != nil { + rewriter := qp.aggrRewriter() + sqlparser.Rewrite(sel.Having.Expr, rewriter.rewrite(), nil) + if rewriter.err != nil { + return nil, rewriter.err + } + } + return qp, nil } +func (ar *aggrRewriter) rewrite() func(*sqlparser.Cursor) bool { + return func(cursor *sqlparser.Cursor) bool { + if ar.err != nil { + return false + } + sqlNode := cursor.Node() + if sqlparser.IsAggregation(sqlNode) { + fExp := sqlNode.(*sqlparser.FuncExpr) + for offset, expr := range ar.qp.SelectExprs { + ae, err := expr.GetAliasedExpr() + if err != nil { + ar.err = err + return false + } + if sqlparser.EqualsExpr(ae.Expr, fExp) { + cursor.Replace(sqlparser.Offset(offset)) + } + } + } + + return true + } +} + +type aggrRewriter struct { + qp *QueryProjection + err error +} + +func (qp *QueryProjection) aggrRewriter() *aggrRewriter { + return &aggrRewriter{qp: qp} +} + func (qp *QueryProjection) addSelectExpressions(sel *sqlparser.Select) error { for _, selExp := range sel.SelectExprs { switch selExp := selExp.(type) { diff --git a/go/vt/vtgate/planbuilder/horizon_planning.go b/go/vt/vtgate/planbuilder/horizon_planning.go index fe9f8f58c92..4f45a15ec63 100644 --- a/go/vt/vtgate/planbuilder/horizon_planning.go +++ b/go/vt/vtgate/planbuilder/horizon_planning.go @@ -1340,9 +1340,6 @@ func pushHaving(ctx *plancontext.PlanningContext, expr sqlparser.Expr, plan logi case *simpleProjection: return nil, vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: filtering on results of cross-shard derived table") case *orderedAggregate: - if sqlparser.ContainsAggregation(expr) { - expr = sqlparser.Rewrite(expr, node.rewriteAggrExpressions(), nil).(sqlparser.Expr) - } return newFilter(ctx, plan, expr) } return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "[BUG] unreachable %T.filtering", plan) diff --git a/go/vt/vtgate/planbuilder/ordered_aggregate.go b/go/vt/vtgate/planbuilder/ordered_aggregate.go index 8a940e340d3..c79120a8a89 100644 --- a/go/vt/vtgate/planbuilder/ordered_aggregate.go +++ b/go/vt/vtgate/planbuilder/ordered_aggregate.go @@ -387,20 +387,3 @@ func (oa *orderedAggregate) OutputColumns() []sqlparser.SelectExpr { func (oa *orderedAggregate) SetTruncateColumnCount(count int) { oa.truncateColumnCount = count } - -// rewriteAggrExpressions is used when our predicate expression contains aggregation. -// In these cases, we need to rewrite it, so it uses the column output from the ordered aggregate -func (oa *orderedAggregate) rewriteAggrExpressions() func(*sqlparser.Cursor) bool { - return func(cursor *sqlparser.Cursor) bool { - sqlNode := cursor.Node() - if sqlparser.IsAggregation(sqlNode) { - fExp := sqlNode.(*sqlparser.FuncExpr) - for _, aggregate := range oa.aggregates { - if sqlparser.EqualsExpr(aggregate.Expr, fExp) { - cursor.Replace(sqlparser.Offset(aggregate.Col)) - } - } - } - return true - } -} diff --git a/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt b/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt index f08c0f2e72a..79a2e1bcabd 100644 --- a/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt @@ -4171,3 +4171,10 @@ Gen4 error: aggregate functions take a single argument 'count(distinct user_id, ] } } + +# having should be able to add new aggregation expressions in having +"select foo from user group by foo having count(*) = 3" +"unsupported: filtering on results of aggregates" +{ + +} diff --git a/go/vt/vtgate/planbuilder/testdata/onecase.txt b/go/vt/vtgate/planbuilder/testdata/onecase.txt index e819513f354..9f11cfc2aad 100644 --- a/go/vt/vtgate/planbuilder/testdata/onecase.txt +++ b/go/vt/vtgate/planbuilder/testdata/onecase.txt @@ -1 +1,7 @@ # Add your test case here for debugging and run go test -run=One. +# having should be able to add new aggregation expressions in having +"select foo, count(*) from user group by foo having count(*) = 3" +"unsupported: filtering on results of aggregates" +{ + +} From f19fdc73518c77fda1a094e75d221209ff9d191d Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Wed, 18 May 2022 13:34:32 +0200 Subject: [PATCH 2/8] feat: add aggregation expressions needed by HAVING Signed-off-by: Andres Taylor --- .../planbuilder/abstract/queryprojection.go | 48 ++-- go/vt/vtgate/planbuilder/horizon_planning.go | 19 +- .../planbuilder/testdata/aggr_cases.txt | 234 ++++++++++++++++++ go/vt/vtgate/planbuilder/testdata/onecase.txt | 6 - .../testdata/unsupported_cases.txt | 5 - 5 files changed, 278 insertions(+), 34 deletions(-) diff --git a/go/vt/vtgate/planbuilder/abstract/queryprojection.go b/go/vt/vtgate/planbuilder/abstract/queryprojection.go index 0244918c7f6..5795be2c6b2 100644 --- a/go/vt/vtgate/planbuilder/abstract/queryprojection.go +++ b/go/vt/vtgate/planbuilder/abstract/queryprojection.go @@ -45,6 +45,9 @@ type ( OrderExprs []OrderBy CanPushDownSorting bool HasStar bool + + // AddedColumn keeps a counter for expressions added to solve HAVING expressions the user is not selecting + AddedColumn int } // OrderBy contains the expression to used in order by and also if ordering is needed at VTGate level then what the weight_string function expression to be sent down for evaluation. @@ -169,20 +172,18 @@ func CreateQPFromSelect(sel *sqlparser.Select) (*QueryProjection, error) { qp.groupByExprs = nil } - if sel.Having != nil { - rewriter := qp.aggrRewriter() - sqlparser.Rewrite(sel.Having.Expr, rewriter.rewrite(), nil) - if rewriter.err != nil { - return nil, rewriter.err - } - } - return qp, nil } -func (ar *aggrRewriter) rewrite() func(*sqlparser.Cursor) bool { +type AggrRewriter struct { + qp *QueryProjection + Err error +} + +// Rewrite will go through an expression, add aggregations to the QP, and rewrite them to use column offset +func (ar *AggrRewriter) Rewrite() func(*sqlparser.Cursor) bool { return func(cursor *sqlparser.Cursor) bool { - if ar.err != nil { + if ar.Err != nil { return false } sqlNode := cursor.Node() @@ -191,26 +192,33 @@ func (ar *aggrRewriter) rewrite() func(*sqlparser.Cursor) bool { for offset, expr := range ar.qp.SelectExprs { ae, err := expr.GetAliasedExpr() if err != nil { - ar.err = err + ar.Err = err return false } if sqlparser.EqualsExpr(ae.Expr, fExp) { cursor.Replace(sqlparser.Offset(offset)) + return false // no need to visit aggregation children } } + + col := SelectExpr{ + Aggr: true, + Col: &sqlparser.AliasedExpr{Expr: fExp}, + } + ar.qp.HasAggr = true + + cursor.Replace(sqlparser.Offset(len(ar.qp.SelectExprs))) + ar.qp.SelectExprs = append(ar.qp.SelectExprs, col) + ar.qp.AddedColumn++ } return true } } -type aggrRewriter struct { - qp *QueryProjection - err error -} - -func (qp *QueryProjection) aggrRewriter() *aggrRewriter { - return &aggrRewriter{qp: qp} +// AggrRewriter extracts +func (qp *QueryProjection) AggrRewriter() *AggrRewriter { + return &AggrRewriter{qp: qp} } func (qp *QueryProjection) addSelectExpressions(sel *sqlparser.Select) error { @@ -590,6 +598,10 @@ func (qp *QueryProjection) AddGroupBy(by GroupBy) { qp.groupByExprs = append(qp.groupByExprs, by) } +func (qp *QueryProjection) GetColumnCount() int { + return len(qp.SelectExprs) - qp.AddedColumn +} + func checkForInvalidGroupingExpressions(expr sqlparser.Expr) error { return sqlparser.Walk(func(node sqlparser.SQLNode) (bool, error) { if sqlparser.IsAggregation(node) { diff --git a/go/vt/vtgate/planbuilder/horizon_planning.go b/go/vt/vtgate/planbuilder/horizon_planning.go index 4f45a15ec63..14380cf9fb9 100644 --- a/go/vt/vtgate/planbuilder/horizon_planning.go +++ b/go/vt/vtgate/planbuilder/horizon_planning.go @@ -138,18 +138,18 @@ func pushProjections(ctx *plancontext.PlanningContext, plan logicalPlan, selectE } func (hp *horizonPlanning) truncateColumnsIfNeeded(ctx *plancontext.PlanningContext, plan logicalPlan) (logicalPlan, error) { - if len(plan.OutputColumns()) == hp.sel.GetColumnCount() { + if len(plan.OutputColumns()) == hp.qp.GetColumnCount() { return plan, nil } switch p := plan.(type) { case *routeGen4: - p.eroute.SetTruncateColumnCount(hp.sel.GetColumnCount()) + p.eroute.SetTruncateColumnCount(hp.qp.GetColumnCount()) case *joinGen4, *semiJoin, *hashJoin: // since this is a join, we can safely add extra columns and not need to truncate them case *orderedAggregate: - p.truncateColumnCount = hp.sel.GetColumnCount() + p.truncateColumnCount = hp.qp.GetColumnCount() case *memorySort: - p.truncater.SetTruncateColumnCount(hp.sel.GetColumnCount()) + p.truncater.SetTruncateColumnCount(hp.qp.GetColumnCount()) case *pulloutSubquery: newUnderlyingPlan, err := hp.truncateColumnsIfNeeded(ctx, p.underlying) if err != nil { @@ -162,7 +162,8 @@ func (hp *horizonPlanning) truncateColumnsIfNeeded(ctx *plancontext.PlanningCont eSimpleProj: &engine.SimpleProjection{}, } - err := pushProjections(ctx, plan, hp.qp.SelectExprs) + exprs := hp.qp.SelectExprs[0:hp.qp.GetColumnCount()] + err := pushProjections(ctx, plan, exprs) if err != nil { return nil, err } @@ -528,6 +529,14 @@ func (hp *horizonPlanning) planAggrUsingOA( }) } + if hp.sel.Having != nil { + rewriter := hp.qp.AggrRewriter() + sqlparser.Rewrite(hp.sel.Having.Expr, rewriter.Rewrite(), nil) + if rewriter.Err != nil { + return nil, rewriter.Err + } + } + aggregationExprs, err := hp.qp.AggregationExpressions() if err != nil { return nil, err diff --git a/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt b/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt index 79a2e1bcabd..e427fb1d969 100644 --- a/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt @@ -4176,5 +4176,239 @@ Gen4 error: aggregate functions take a single argument 'count(distinct user_id, "select foo from user group by foo having count(*) = 3" "unsupported: filtering on results of aggregates" { + "QueryType": "SELECT", + "Original": "select foo from user group by foo having count(*) = 3", + "Instructions": { + "OperatorType": "SimpleProjection", + "Columns": [ + 0 + ], + "Inputs": [ + { + "OperatorType": "Filter", + "Predicate": "[1] = 3", + "Inputs": [ + { + "OperatorType": "Aggregate", + "Variant": "Ordered", + "Aggregates": "sum(1) AS count(*)", + "GroupBy": "(0|2)", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select foo, count(*), weight_string(foo) from `user` where 1 != 1 group by foo, weight_string(foo)", + "OrderBy": "(0|2) ASC", + "Query": "select foo, count(*), weight_string(foo) from `user` group by foo, weight_string(foo) order by foo asc", + "Table": "`user`" + } + ] + } + ] + } + ] + } +} +"select u.id from user u join user_extra ue on ue.id = u.id group by u.id having count(u.name) = 3" +"unsupported: cross-shard query with aggregates" +{ + "QueryType": "SELECT", + "Original": "select u.id from user u join user_extra ue on ue.id = u.id group by u.id having count(u.name) = 3", + "Instructions": { + "OperatorType": "SimpleProjection", + "Columns": [ + 0 + ], + "Inputs": [ + { + "OperatorType": "Filter", + "Predicate": "[1] = 3", + "Inputs": [ + { + "OperatorType": "Aggregate", + "Variant": "Ordered", + "Aggregates": "sum(1) AS count(u.`name`)", + "GroupBy": "(0|2)", + "Inputs": [ + { + "OperatorType": "Projection", + "Expressions": [ + "[COLUMN 0] as id", + "[COLUMN 2] * [COLUMN 3] as count(u.`name`)", + "[COLUMN 1]" + ], + "Inputs": [ + { + "OperatorType": "Sort", + "Variant": "Memory", + "OrderBy": "(0|1) ASC", + "Inputs": [ + { + "OperatorType": "Join", + "Variant": "Join", + "JoinColumnIndexes": "R:1,R:2,L:1,R:0", + "JoinVars": { + "ue_id": 0 + }, + "TableName": "user_extra_`user`", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select ue.id, count(*), weight_string(ue.id) from user_extra as ue where 1 != 1 group by ue.id, weight_string(ue.id)", + "Query": "select ue.id, count(*), weight_string(ue.id) from user_extra as ue group by ue.id, weight_string(ue.id)", + "Table": "user_extra" + }, + { + "OperatorType": "Route", + "Variant": "EqualUnique", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select count(u.`name`), u.id, weight_string(u.id) from `user` as u where 1 != 1 group by u.id, weight_string(u.id)", + "Query": "select count(u.`name`), u.id, weight_string(u.id) from `user` as u where u.id = :ue_id group by u.id, weight_string(u.id)", + "Table": "`user`", + "Values": [ + ":ue_id" + ], + "Vindex": "user_index" + } + ] + } + ] + } + ] + } + ] + } + ] + } + ] + } +} + +"select u.id from user u join user_extra ue on ue.user_id = u.id group by u.id having count(u.name) = 3" +{ + "QueryType": "SELECT", + "Original": "select u.id from user u join user_extra ue on ue.user_id = u.id group by u.id having count(u.name) = 3", + "Instructions": { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select u.id from `user` as u join user_extra as ue on ue.user_id = u.id where 1 != 1 group by u.id", + "Query": "select u.id from `user` as u join user_extra as ue on ue.user_id = u.id group by u.id having count(u.`name`) = 3", + "Table": "`user`, user_extra" + } +} +{ + "QueryType": "SELECT", + "Original": "select u.id from user u join user_extra ue on ue.user_id = u.id group by u.id having count(u.name) = 3", + "Instructions": { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select u.id from `user` as u, user_extra as ue where 1 != 1 group by u.id", + "Query": "select u.id from `user` as u, user_extra as ue where ue.user_id = u.id group by u.id having count(u.`name`) = 3", + "Table": "`user`, user_extra" + } +} + +# only extract the aggregation once, even if used twice +"select u.id from user u join user_extra ue on ue.id = u.id group by u.id having count(*) < 3 and count(*) > 5" +"unsupported: cross-shard query with aggregates" +{ + "QueryType": "SELECT", + "Original": "select u.id from user u join user_extra ue on ue.id = u.id group by u.id having count(*) \u003c 3 and count(*) \u003e 5", + "Instructions": { + "OperatorType": "SimpleProjection", + "Columns": [ + 0 + ], + "Inputs": [ + { + "OperatorType": "Filter", + "Predicate": "[1] \u003c 3 and [1] \u003e 5", + "Inputs": [ + { + "OperatorType": "Aggregate", + "Variant": "Ordered", + "Aggregates": "sum(1) AS count(*)", + "GroupBy": "(0|2)", + "Inputs": [ + { + "OperatorType": "Projection", + "Expressions": [ + "[COLUMN 0] as id", + "[COLUMN 2] * [COLUMN 3] as count(*)", + "[COLUMN 1]" + ], + "Inputs": [ + { + "OperatorType": "Sort", + "Variant": "Memory", + "OrderBy": "(0|1) ASC", + "Inputs": [ + { + "OperatorType": "Join", + "Variant": "Join", + "JoinColumnIndexes": "R:1,R:2,L:1,R:0", + "JoinVars": { + "ue_id": 0 + }, + "TableName": "user_extra_`user`", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select ue.id, count(*), weight_string(ue.id) from user_extra as ue where 1 != 1 group by ue.id, weight_string(ue.id)", + "Query": "select ue.id, count(*), weight_string(ue.id) from user_extra as ue group by ue.id, weight_string(ue.id)", + "Table": "user_extra" + }, + { + "OperatorType": "Route", + "Variant": "EqualUnique", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select count(*), u.id, weight_string(u.id) from `user` as u where 1 != 1 group by u.id, weight_string(u.id)", + "Query": "select count(*), u.id, weight_string(u.id) from `user` as u where u.id = :ue_id group by u.id, weight_string(u.id)", + "Table": "`user`", + "Values": [ + ":ue_id" + ], + "Vindex": "user_index" + } + ] + } + ] + } + ] + } + ] + } + ] + } + ] + } } diff --git a/go/vt/vtgate/planbuilder/testdata/onecase.txt b/go/vt/vtgate/planbuilder/testdata/onecase.txt index 9f11cfc2aad..e819513f354 100644 --- a/go/vt/vtgate/planbuilder/testdata/onecase.txt +++ b/go/vt/vtgate/planbuilder/testdata/onecase.txt @@ -1,7 +1 @@ # Add your test case here for debugging and run go test -run=One. -# having should be able to add new aggregation expressions in having -"select foo, count(*) from user group by foo having count(*) = 3" -"unsupported: filtering on results of aggregates" -{ - -} diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt index 7e84405e174..4e178ef2920 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt @@ -619,11 +619,6 @@ Gen4 error: unsupported: in scatter query: aggregation function 'avg' "generating order by clause: ambiguous symbol reference: a" Gen4 plan same as above -# TODO this should be planned without using OA and MS -"select u.id from user u join user_extra ue on ue.id = u.id group by u.id having count(u.name) = 3" -"unsupported: cross-shard query with aggregates" -Gen4 error: cannot push projections in ordered aggregates - "select (select 1 from user u having count(ue.col) > 10) from user_extra ue" "symbol ue.col not found in subquery" Gen4 error: cannot push projections in ordered aggregates From 1cfae2c4040369044dfec3aa6c5fb9106ebc4df5 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 19 May 2022 09:20:18 +0200 Subject: [PATCH 3/8] test&refactor: add integration test and clean up types Signed-off-by: Andres Taylor --- .../queries/aggregation/aggregation_test.go | 3 +++ .../planbuilder/abstract/queryprojection.go | 19 ++++++++++--------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go index fc5d1915b7a..61c887ac74d 100644 --- a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go +++ b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go @@ -179,6 +179,9 @@ func TestAggrOnJoin(t *testing.T) { mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ a.val1, count(*) as leCount from aggr_test a join t3 t on a.val2 = t.id7 group by a.val1 having leCount = 4", `[[VARCHAR("a") INT64(4)]]`) + + mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ a.val1 from aggr_test a join t3 t on a.val2 = t.id7 group by a.val1 having count(*) = 4", + `[[VARCHAR("a")]]`) } func TestNotEqualFilterOnScatter(t *testing.T) { diff --git a/go/vt/vtgate/planbuilder/abstract/queryprojection.go b/go/vt/vtgate/planbuilder/abstract/queryprojection.go index 5795be2c6b2..fd02d1e2eee 100644 --- a/go/vt/vtgate/planbuilder/abstract/queryprojection.go +++ b/go/vt/vtgate/planbuilder/abstract/queryprojection.go @@ -68,6 +68,7 @@ type ( aliasedExpr *sqlparser.AliasedExpr } + // Aggr encodes all information needed for aggregation functions Aggr struct { Original *sqlparser.AliasedExpr Func *sqlparser.FuncExpr @@ -77,6 +78,15 @@ type ( Index *int Distinct bool } + + Aggrs []Aggr + + AggrRewriter struct { + qp *QueryProjection + Err error + } + + GroupBys []GroupBy ) func (b GroupBy) AsOrderBy() OrderBy { @@ -175,11 +185,6 @@ func CreateQPFromSelect(sel *sqlparser.Select) (*QueryProjection, error) { return qp, nil } -type AggrRewriter struct { - qp *QueryProjection - Err error -} - // Rewrite will go through an expression, add aggregations to the QP, and rewrite them to use column offset func (ar *AggrRewriter) Rewrite() func(*sqlparser.Cursor) bool { return func(cursor *sqlparser.Cursor) bool { @@ -616,8 +621,6 @@ func checkForInvalidGroupingExpressions(expr sqlparser.Expr) error { }, expr) } -type Aggrs []Aggr - // Len implements the sort.Interface func (a Aggrs) Len() int { return len(a) @@ -645,8 +648,6 @@ func CompareRefInt(a *int, b *int) bool { return *a < *b } -type GroupBys []GroupBy - // Len implements the sort.Interface func (gbys GroupBys) Len() int { return len(gbys) From 3df6cd3594b497d601fc58fb56cb6e7835c313dd Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 19 May 2022 09:39:20 +0200 Subject: [PATCH 4/8] refactor: use sort.Slice to avoid extra types Signed-off-by: Andres Taylor --- .../planbuilder/abstract/queryprojection.go | 40 +++++-------------- .../vtgate/planbuilder/aggregation_pushing.go | 6 +-- 2 files changed, 11 insertions(+), 35 deletions(-) diff --git a/go/vt/vtgate/planbuilder/abstract/queryprojection.go b/go/vt/vtgate/planbuilder/abstract/queryprojection.go index fd02d1e2eee..5480c76e1af 100644 --- a/go/vt/vtgate/planbuilder/abstract/queryprojection.go +++ b/go/vt/vtgate/planbuilder/abstract/queryprojection.go @@ -79,14 +79,10 @@ type ( Distinct bool } - Aggrs []Aggr - AggrRewriter struct { qp *QueryProjection Err error } - - GroupBys []GroupBy ) func (b GroupBy) AsOrderBy() OrderBy { @@ -567,7 +563,7 @@ func (qp *QueryProjection) AlignGroupByAndOrderBy() { // The query didn't ask for any particular order, so we are free to add arbitrary ordering. // We'll align the grouping and ordering by the output columns newGrouping = qp.GetGrouping() - sort.Sort(GroupBys(newGrouping)) + SortGrouping(newGrouping) for _, groupBy := range newGrouping { qp.OrderExprs = append(qp.OrderExprs, groupBy.AsOrderBy()) } @@ -621,19 +617,16 @@ func checkForInvalidGroupingExpressions(expr sqlparser.Expr) error { }, expr) } -// Len implements the sort.Interface -func (a Aggrs) Len() int { - return len(a) +func SortAggregations(a []Aggr) { + sort.Slice(a, func(i, j int) bool { + return CompareRefInt(a[i].Index, a[j].Index) + }) } -// Less implements the sort.Interface -func (a Aggrs) Less(i, j int) bool { - return CompareRefInt(a[i].Index, a[j].Index) -} - -// Swap implements the sort.Interface -func (a Aggrs) Swap(i, j int) { - a[i], a[j] = a[j], a[i] +func SortGrouping(a []GroupBy) { + sort.Slice(a, func(i, j int) bool { + return CompareRefInt(a[i].InnerIndex, a[j].InnerIndex) + }) } // CompareRefInt compares two references of integers. @@ -647,18 +640,3 @@ func CompareRefInt(a *int, b *int) bool { } return *a < *b } - -// Len implements the sort.Interface -func (gbys GroupBys) Len() int { - return len(gbys) -} - -// Less implements the sort.Interface -func (gbys GroupBys) Less(i, j int) bool { - return CompareRefInt(gbys[i].InnerIndex, gbys[j].InnerIndex) -} - -// Swap implements the sort.Interface -func (gbys GroupBys) Swap(i, j int) { - gbys[i], gbys[j] = gbys[j], gbys[i] -} diff --git a/go/vt/vtgate/planbuilder/aggregation_pushing.go b/go/vt/vtgate/planbuilder/aggregation_pushing.go index bae6486f2b5..96105d95fcd 100644 --- a/go/vt/vtgate/planbuilder/aggregation_pushing.go +++ b/go/vt/vtgate/planbuilder/aggregation_pushing.go @@ -17,8 +17,6 @@ limitations under the License. package planbuilder import ( - "sort" - vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" "vitess.io/vitess/go/vt/sqlparser" "vitess.io/vitess/go/vt/vterrors" @@ -484,8 +482,8 @@ func sortOffsets(grouping []abstract.GroupBy, aggregations []abstract.Aggr) ([]a originalAggr := make([]abstract.Aggr, len(aggregations)) copy(originalAggr, aggregations) copy(originalGrouping, grouping) - sort.Sort(abstract.Aggrs(aggregations)) - sort.Sort(abstract.GroupBys(grouping)) + abstract.SortAggregations(aggregations) + abstract.SortGrouping(grouping) reorg := func(groupByOffsets []offsets, aggrOffsets [][]offsets) ([]offsets, [][]offsets) { orderedGroupingOffsets := make([]offsets, 0, len(originalGrouping)) From daace3a5b60d341cc872334ed30795bf5f8d9618 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 19 May 2022 10:11:43 +0200 Subject: [PATCH 5/8] feat: change the string representation of offset for better clarity Signed-off-by: Andres Taylor --- go/vt/sqlparser/ast.go | 10 +++- go/vt/sqlparser/ast_clone.go | 21 +++++-- go/vt/sqlparser/ast_equals.go | 30 +++++++--- go/vt/sqlparser/ast_format.go | 13 +++-- go/vt/sqlparser/ast_format_fast.go | 21 +++++-- go/vt/sqlparser/ast_funcs.go | 4 ++ go/vt/sqlparser/ast_rewrite.go | 57 ++++++++++--------- go/vt/sqlparser/ast_visit.go | 25 ++++---- go/vt/sqlparser/cached_size.go | 12 ++++ go/vt/vtgate/engine/projection_test.go | 4 +- go/vt/vtgate/evalengine/translate.go | 4 +- .../planbuilder/abstract/queryprojection.go | 4 +- go/vt/vtgate/planbuilder/horizon_planning.go | 8 +-- .../planbuilder/testdata/aggr_cases.txt | 12 ++-- 14 files changed, 144 insertions(+), 81 deletions(-) diff --git a/go/vt/sqlparser/ast.go b/go/vt/sqlparser/ast.go index acd91f5d53f..01d61ceaa33 100644 --- a/go/vt/sqlparser/ast.go +++ b/go/vt/sqlparser/ast.go @@ -2353,8 +2353,12 @@ type ( JSONVal Expr } - // Offset is another AST type that is used during planning and never produced by the parser - Offset int + // Offset is an AST type that is used during planning and never produced by the parser + // it is the column offset from the incoming result stream + Offset struct { + V int + Original string + } // JSONArrayExpr represents JSON_ARRAY() // More information on https://dev.mysql.com/doc/refman/8.0/en/json-creation-functions.html#function_json-array @@ -2597,7 +2601,7 @@ func (*ExtractedSubquery) iExpr() {} func (*TrimFuncExpr) iExpr() {} func (*JSONSchemaValidFuncExpr) iExpr() {} func (*JSONSchemaValidationReportFuncExpr) iExpr() {} -func (Offset) iExpr() {} +func (*Offset) iExpr() {} func (*JSONPrettyExpr) iExpr() {} func (*JSONStorageFreeExpr) iExpr() {} func (*JSONStorageSizeExpr) iExpr() {} diff --git a/go/vt/sqlparser/ast_clone.go b/go/vt/sqlparser/ast_clone.go index c771ab85ee1..6b1c85e7c2b 100644 --- a/go/vt/sqlparser/ast_clone.go +++ b/go/vt/sqlparser/ast_clone.go @@ -255,8 +255,8 @@ func CloneSQLNode(in SQLNode) SQLNode { return CloneRefOfNotExpr(in) case *NullVal: return CloneRefOfNullVal(in) - case Offset: - return in + case *Offset: + return CloneRefOfOffset(in) case OnDup: return CloneOnDup(in) case *OptLike: @@ -1612,6 +1612,15 @@ func CloneRefOfNullVal(n *NullVal) *NullVal { return &out } +// CloneRefOfOffset creates a deep clone of the input. +func CloneRefOfOffset(n *Offset) *Offset { + if n == nil { + return nil + } + out := *n + return &out +} + // CloneOnDup creates a deep clone of the input. func CloneOnDup(n OnDup) OnDup { if n == nil { @@ -2815,8 +2824,8 @@ func CloneExpr(in Expr) Expr { return CloneRefOfNotExpr(in) case *NullVal: return CloneRefOfNullVal(in) - case Offset: - return in + case *Offset: + return CloneRefOfOffset(in) case *OrExpr: return CloneRefOfOrExpr(in) case *Subquery: @@ -2961,8 +2970,8 @@ func CloneJSONPathParam(in JSONPathParam) JSONPathParam { return CloneRefOfNotExpr(in) case *NullVal: return CloneRefOfNullVal(in) - case Offset: - return in + case *Offset: + return CloneRefOfOffset(in) case *OrExpr: return CloneRefOfOrExpr(in) case *Subquery: diff --git a/go/vt/sqlparser/ast_equals.go b/go/vt/sqlparser/ast_equals.go index 41ce6a314dc..0d311eb350a 100644 --- a/go/vt/sqlparser/ast_equals.go +++ b/go/vt/sqlparser/ast_equals.go @@ -722,12 +722,12 @@ func EqualsSQLNode(inA, inB SQLNode) bool { return false } return EqualsRefOfNullVal(a, b) - case Offset: - b, ok := inB.(Offset) + case *Offset: + b, ok := inB.(*Offset) if !ok { return false } - return a == b + return EqualsRefOfOffset(a, b) case OnDup: b, ok := inB.(OnDup) if !ok { @@ -2607,6 +2607,18 @@ func EqualsRefOfNullVal(a, b *NullVal) bool { return true } +// EqualsRefOfOffset does deep equals between the two objects. +func EqualsRefOfOffset(a, b *Offset) bool { + if a == b { + return true + } + if a == nil || b == nil { + return false + } + return a.V == b.V && + a.Original == b.Original +} + // EqualsOnDup does deep equals between the two objects. func EqualsOnDup(a, b OnDup) bool { if len(a) != len(b) { @@ -4454,12 +4466,12 @@ func EqualsExpr(inA, inB Expr) bool { return false } return EqualsRefOfNullVal(a, b) - case Offset: - b, ok := inB.(Offset) + case *Offset: + b, ok := inB.(*Offset) if !ok { return false } - return a == b + return EqualsRefOfOffset(a, b) case *OrExpr: b, ok := inB.(*OrExpr) if !ok { @@ -4850,12 +4862,12 @@ func EqualsJSONPathParam(inA, inB JSONPathParam) bool { return false } return EqualsRefOfNullVal(a, b) - case Offset: - b, ok := inB.(Offset) + case *Offset: + b, ok := inB.(*Offset) if !ok { return false } - return a == b + return EqualsRefOfOffset(a, b) case *OrExpr: b, ok := inB.(*OrExpr) if !ok { diff --git a/go/vt/sqlparser/ast_format.go b/go/vt/sqlparser/ast_format.go index 7be21d9633c..5f8eadf9122 100644 --- a/go/vt/sqlparser/ast_format.go +++ b/go/vt/sqlparser/ast_format.go @@ -2144,8 +2144,13 @@ func (node *JtOnResponse) Format(buf *TrackedBuffer) { } // Format formats the node. -func (node Offset) Format(buf *TrackedBuffer) { - buf.astPrintf(node, "[%d]", int(node)) +// Using capital letter for this function as an indicator that it's not a normal function call ¯\_(ツ)_/¯ +func (node *Offset) Format(buf *TrackedBuffer) { + if node.Original == "" { + buf.astPrintf(node, "OFFSET(%d)", node.V) + } else { + buf.astPrintf(node, "OFFSET(%d, '%s')", node.V, node.Original) + } } // Format formats the node. @@ -2160,7 +2165,7 @@ func (node *JSONSchemaValidationReportFuncExpr) Format(buf *TrackedBuffer) { // Format formats the node. func (node *JSONArrayExpr) Format(buf *TrackedBuffer) { - //buf.astPrintf(node,"%s(,"node.Name.Lowered()) + // buf.astPrintf(node,"%s(,"node.Name.Lowered()) buf.literal("json_array(") if len(node.Params) > 0 { var prefix string @@ -2174,7 +2179,7 @@ func (node *JSONArrayExpr) Format(buf *TrackedBuffer) { // Format formats the node. func (node *JSONObjectExpr) Format(buf *TrackedBuffer) { - //buf.astPrintf(node,"%s(,"node.Name.Lowered()) + // buf.astPrintf(node,"%s(,"node.Name.Lowered()) buf.literal("json_object(") if len(node.Params) > 0 { for i, p := range node.Params { diff --git a/go/vt/sqlparser/ast_format_fast.go b/go/vt/sqlparser/ast_format_fast.go index d48e056e731..069cfc5bc45 100644 --- a/go/vt/sqlparser/ast_format_fast.go +++ b/go/vt/sqlparser/ast_format_fast.go @@ -2793,10 +2793,19 @@ func (node *JtOnResponse) formatFast(buf *TrackedBuffer) { } // formatFast formats the node. -func (node Offset) formatFast(buf *TrackedBuffer) { - buf.WriteByte('[') - buf.WriteString(fmt.Sprintf("%d", int(node))) - buf.WriteByte(']') +// Using capital letter for this function as an indicator that it's not a normal function call ¯\_(ツ)_/¯ +func (node *Offset) formatFast(buf *TrackedBuffer) { + if node.Original == "" { + buf.WriteString("OFFSET(") + buf.WriteString(fmt.Sprintf("%d", node.V)) + buf.WriteByte(')') + } else { + buf.WriteString("OFFSET(") + buf.WriteString(fmt.Sprintf("%d", node.V)) + buf.WriteString(", '") + buf.WriteString(node.Original) + buf.WriteString("')") + } } // formatFast formats the node. @@ -2819,7 +2828,7 @@ func (node *JSONSchemaValidationReportFuncExpr) formatFast(buf *TrackedBuffer) { // formatFast formats the node. func (node *JSONArrayExpr) formatFast(buf *TrackedBuffer) { - //buf.astPrintf(node,"%s(,"node.Name.Lowered()) + // buf.astPrintf(node,"%s(,"node.Name.Lowered()) buf.WriteString("json_array(") if len(node.Params) > 0 { var prefix string @@ -2834,7 +2843,7 @@ func (node *JSONArrayExpr) formatFast(buf *TrackedBuffer) { // formatFast formats the node. func (node *JSONObjectExpr) formatFast(buf *TrackedBuffer) { - //buf.astPrintf(node,"%s(,"node.Name.Lowered()) + // buf.astPrintf(node,"%s(,"node.Name.Lowered()) buf.WriteString("json_object(") if len(node.Params) > 0 { for i, p := range node.Params { diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index 966893730f5..2a07f97109e 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -670,6 +670,10 @@ func NewColIdentWithAt(str string, at AtCount) ColIdent { } } +func NewOffset(v int, original Expr) *Offset { + return &Offset{V: v, Original: String(original)} +} + // IsEmpty returns true if the name is empty. func (node ColIdent) IsEmpty() bool { return node.val == "" diff --git a/go/vt/sqlparser/ast_rewrite.go b/go/vt/sqlparser/ast_rewrite.go index 2efcebfd0d8..0d6f2f4264b 100644 --- a/go/vt/sqlparser/ast_rewrite.go +++ b/go/vt/sqlparser/ast_rewrite.go @@ -254,8 +254,8 @@ func (a *application) rewriteSQLNode(parent SQLNode, node SQLNode, replacer repl return a.rewriteRefOfNotExpr(parent, node, replacer) case *NullVal: return a.rewriteRefOfNullVal(parent, node, replacer) - case Offset: - return a.rewriteOffset(parent, node, replacer) + case *Offset: + return a.rewriteRefOfOffset(parent, node, replacer) case OnDup: return a.rewriteOnDup(parent, node, replacer) case *OptLike: @@ -3938,6 +3938,30 @@ func (a *application) rewriteRefOfNullVal(parent SQLNode, node *NullVal, replace } return true } +func (a *application) rewriteRefOfOffset(parent SQLNode, node *Offset, replacer replacerFunc) bool { + if node == nil { + return true + } + if a.pre != nil { + a.cur.replacer = replacer + a.cur.parent = parent + a.cur.node = node + if !a.pre(&a.cur) { + return true + } + } + if a.post != nil { + if a.pre == nil { + a.cur.replacer = replacer + a.cur.parent = parent + a.cur.node = node + } + if !a.post(&a.cur) { + return false + } + } + return true +} func (a *application) rewriteOnDup(parent SQLNode, node OnDup, replacer replacerFunc) bool { if node == nil { return true @@ -6856,8 +6880,8 @@ func (a *application) rewriteExpr(parent SQLNode, node Expr, replacer replacerFu return a.rewriteRefOfNotExpr(parent, node, replacer) case *NullVal: return a.rewriteRefOfNullVal(parent, node, replacer) - case Offset: - return a.rewriteOffset(parent, node, replacer) + case *Offset: + return a.rewriteRefOfOffset(parent, node, replacer) case *OrExpr: return a.rewriteRefOfOrExpr(parent, node, replacer) case *Subquery: @@ -6998,8 +7022,8 @@ func (a *application) rewriteJSONPathParam(parent SQLNode, node JSONPathParam, r return a.rewriteRefOfNotExpr(parent, node, replacer) case *NullVal: return a.rewriteRefOfNullVal(parent, node, replacer) - case Offset: - return a.rewriteOffset(parent, node, replacer) + case *Offset: + return a.rewriteRefOfOffset(parent, node, replacer) case *OrExpr: return a.rewriteRefOfOrExpr(parent, node, replacer) case *Subquery: @@ -7350,27 +7374,6 @@ func (a *application) rewriteMatchAction(parent SQLNode, node MatchAction, repla } return true } -func (a *application) rewriteOffset(parent SQLNode, node Offset, replacer replacerFunc) bool { - if a.pre != nil { - a.cur.replacer = replacer - a.cur.parent = parent - a.cur.node = node - if !a.pre(&a.cur) { - return true - } - } - if a.post != nil { - if a.pre == nil { - a.cur.replacer = replacer - a.cur.parent = parent - a.cur.node = node - } - if !a.post(&a.cur) { - return false - } - } - return true -} func (a *application) rewriteReferenceAction(parent SQLNode, node ReferenceAction, replacer replacerFunc) bool { if a.pre != nil { a.cur.replacer = replacer diff --git a/go/vt/sqlparser/ast_visit.go b/go/vt/sqlparser/ast_visit.go index 8faf0f6fe76..6da93887884 100644 --- a/go/vt/sqlparser/ast_visit.go +++ b/go/vt/sqlparser/ast_visit.go @@ -254,8 +254,8 @@ func VisitSQLNode(in SQLNode, f Visit) error { return VisitRefOfNotExpr(in, f) case *NullVal: return VisitRefOfNullVal(in, f) - case Offset: - return VisitOffset(in, f) + case *Offset: + return VisitRefOfOffset(in, f) case OnDup: return VisitOnDup(in, f) case *OptLike: @@ -2033,6 +2033,15 @@ func VisitRefOfNullVal(in *NullVal, f Visit) error { } return nil } +func VisitRefOfOffset(in *Offset, f Visit) error { + if in == nil { + return nil + } + if cont, err := f(in); err != nil || !cont { + return err + } + return nil +} func VisitOnDup(in OnDup, f Visit) error { if in == nil { return nil @@ -3505,8 +3514,8 @@ func VisitExpr(in Expr, f Visit) error { return VisitRefOfNotExpr(in, f) case *NullVal: return VisitRefOfNullVal(in, f) - case Offset: - return VisitOffset(in, f) + case *Offset: + return VisitRefOfOffset(in, f) case *OrExpr: return VisitRefOfOrExpr(in, f) case *Subquery: @@ -3647,8 +3656,8 @@ func VisitJSONPathParam(in JSONPathParam, f Visit) error { return VisitRefOfNotExpr(in, f) case *NullVal: return VisitRefOfNullVal(in, f) - case Offset: - return VisitOffset(in, f) + case *Offset: + return VisitRefOfOffset(in, f) case *OrExpr: return VisitRefOfOrExpr(in, f) case *Subquery: @@ -3880,10 +3889,6 @@ func VisitMatchAction(in MatchAction, f Visit) error { _, err := f(in) return err } -func VisitOffset(in Offset, f Visit) error { - _, err := f(in) - return err -} func VisitReferenceAction(in ReferenceAction, f Visit) error { _, err := f(in) return err diff --git a/go/vt/sqlparser/cached_size.go b/go/vt/sqlparser/cached_size.go index 767efd39a70..b93fbe4f354 100644 --- a/go/vt/sqlparser/cached_size.go +++ b/go/vt/sqlparser/cached_size.go @@ -2104,6 +2104,18 @@ func (cached *NotExpr) CachedSize(alloc bool) int64 { } return size } +func (cached *Offset) CachedSize(alloc bool) int64 { + if cached == nil { + return int64(0) + } + size := int64(0) + if alloc { + size += int64(24) + } + // field Original string + size += hack.RuntimeAllocSize(int64(len(cached.Original))) + return size +} func (cached *OptLike) CachedSize(alloc bool) int64 { if cached == nil { return int64(0) diff --git a/go/vt/vtgate/engine/projection_test.go b/go/vt/vtgate/engine/projection_test.go index 09bc9ff3e40..42bc7555ed7 100644 --- a/go/vt/vtgate/engine/projection_test.go +++ b/go/vt/vtgate/engine/projection_test.go @@ -32,8 +32,8 @@ import ( func TestMultiply(t *testing.T) { expr := &sqlparser.BinaryExpr{ Operator: sqlparser.MultOp, - Left: sqlparser.Offset(0), - Right: sqlparser.Offset(1), + Left: &sqlparser.Offset{V: 0}, + Right: &sqlparser.Offset{V: 1}, } evalExpr, err := evalengine.Translate(expr, nil) require.NoError(t, err) diff --git a/go/vt/vtgate/evalengine/translate.go b/go/vt/vtgate/evalengine/translate.go index 6a5d9ba507f..4239d0df065 100644 --- a/go/vt/vtgate/evalengine/translate.go +++ b/go/vt/vtgate/evalengine/translate.go @@ -572,8 +572,8 @@ func translateExpr(e sqlparser.Expr, lookup TranslationLookup) (Expr, error) { return NewLiteralInt(0), nil case *sqlparser.ColName: return translateColName(node, lookup) - case sqlparser.Offset: - return NewColumn(int(node), getCollation(node, lookup)), nil + case *sqlparser.Offset: + return NewColumn(node.V, getCollation(node, lookup)), nil case *sqlparser.ComparisonExpr: return translateComparisonExpr(node.Operator, node.Left, node.Right, lookup) case sqlparser.Argument: diff --git a/go/vt/vtgate/planbuilder/abstract/queryprojection.go b/go/vt/vtgate/planbuilder/abstract/queryprojection.go index 5480c76e1af..2c63f078e10 100644 --- a/go/vt/vtgate/planbuilder/abstract/queryprojection.go +++ b/go/vt/vtgate/planbuilder/abstract/queryprojection.go @@ -197,7 +197,7 @@ func (ar *AggrRewriter) Rewrite() func(*sqlparser.Cursor) bool { return false } if sqlparser.EqualsExpr(ae.Expr, fExp) { - cursor.Replace(sqlparser.Offset(offset)) + cursor.Replace(sqlparser.NewOffset(offset, fExp)) return false // no need to visit aggregation children } } @@ -208,7 +208,7 @@ func (ar *AggrRewriter) Rewrite() func(*sqlparser.Cursor) bool { } ar.qp.HasAggr = true - cursor.Replace(sqlparser.Offset(len(ar.qp.SelectExprs))) + cursor.Replace(sqlparser.NewOffset(len(ar.qp.SelectExprs), fExp)) ar.qp.SelectExprs = append(ar.qp.SelectExprs, col) ar.qp.AddedColumn++ } diff --git a/go/vt/vtgate/planbuilder/horizon_planning.go b/go/vt/vtgate/planbuilder/horizon_planning.go index 14380cf9fb9..5fb0b759d87 100644 --- a/go/vt/vtgate/planbuilder/horizon_planning.go +++ b/go/vt/vtgate/planbuilder/horizon_planning.go @@ -609,13 +609,13 @@ func passGroupingColumns(proj *projection, groupings []offsets, grouping []abstr for idx, grp := range groupings { origGrp := grouping[idx] var offs offsets - alias := origGrp.AsAliasedExpr().ColumnName() - offs.col, err = proj.addColumn(origGrp.InnerIndex, sqlparser.Offset(grp.col), alias) + expr := origGrp.AsAliasedExpr() + offs.col, err = proj.addColumn(origGrp.InnerIndex, sqlparser.NewOffset(grp.col, expr.Expr), expr.ColumnName()) if err != nil { return nil, err } if grp.wsCol != -1 { - offs.wsCol, err = proj.addColumn(nil, sqlparser.Offset(grp.wsCol), "") + offs.wsCol, err = proj.addColumn(nil, sqlparser.NewOffset(grp.wsCol, weightStringFor(expr.Expr)), "") if err != nil { return nil, err } @@ -634,7 +634,7 @@ func generateAggregateParams(aggrs []abstract.Aggr, aggrParamOffsets [][]offsets if proj != nil { var aggrExpr sqlparser.Expr for _, ofs := range paramOffset { - curr := sqlparser.Offset(ofs.col) + curr := &sqlparser.Offset{V: ofs.col} if aggrExpr == nil { aggrExpr = curr } else { diff --git a/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt b/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt index e427fb1d969..b32677550c0 100644 --- a/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt @@ -4056,7 +4056,7 @@ Gen4 error: aggregate functions take a single argument 'count(distinct user_id, "Inputs": [ { "OperatorType": "Filter", - "Predicate": "[1] = 3", + "Predicate": "OFFSET(1, 'count(*)') = 3", "Inputs": [ { "OperatorType": "Aggregate", @@ -4100,7 +4100,7 @@ Gen4 error: aggregate functions take a single argument 'count(distinct user_id, "Inputs": [ { "OperatorType": "Filter", - "Predicate": "[1] + [2] = 42", + "Predicate": "OFFSET(1, 'sum(foo)') + OFFSET(2, 'sum(bar)') = 42", "Inputs": [ { "OperatorType": "Aggregate", @@ -4144,7 +4144,7 @@ Gen4 error: aggregate functions take a single argument 'count(distinct user_id, "Inputs": [ { "OperatorType": "Filter", - "Predicate": "fooSum + [2] = 42", + "Predicate": "fooSum + OFFSET(2, 'sum(bar)') = 42", "Inputs": [ { "OperatorType": "Aggregate", @@ -4186,7 +4186,7 @@ Gen4 error: aggregate functions take a single argument 'count(distinct user_id, "Inputs": [ { "OperatorType": "Filter", - "Predicate": "[1] = 3", + "Predicate": "OFFSET(1, 'count(*)') = 3", "Inputs": [ { "OperatorType": "Aggregate", @@ -4227,7 +4227,7 @@ Gen4 error: aggregate functions take a single argument 'count(distinct user_id, "Inputs": [ { "OperatorType": "Filter", - "Predicate": "[1] = 3", + "Predicate": "OFFSET(1, 'count(u.`name`)') = 3", "Inputs": [ { "OperatorType": "Aggregate", @@ -4343,7 +4343,7 @@ Gen4 error: aggregate functions take a single argument 'count(distinct user_id, "Inputs": [ { "OperatorType": "Filter", - "Predicate": "[1] \u003c 3 and [1] \u003e 5", + "Predicate": "OFFSET(1, 'count(*)') \u003c 3 and OFFSET(1, 'count(*)') \u003e 5", "Inputs": [ { "OperatorType": "Aggregate", From fe410eeba315ab6a40f7cd9f22ed0e05af943c3e Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 19 May 2022 12:06:08 +0200 Subject: [PATCH 6/8] feat: handle columns with not in grouping or aggregations better Signed-off-by: Andres Taylor --- .../planbuilder/abstract/queryprojection.go | 2 - .../vtgate/planbuilder/aggregation_pushing.go | 9 +- go/vt/vtgate/planbuilder/horizon_planning.go | 31 +++-- .../planbuilder/testdata/aggr_cases.txt | 108 +++++++++++++++++- .../testdata/unsupported_cases.txt | 67 ++++++++++- 5 files changed, 194 insertions(+), 23 deletions(-) diff --git a/go/vt/vtgate/planbuilder/abstract/queryprojection.go b/go/vt/vtgate/planbuilder/abstract/queryprojection.go index 2c63f078e10..7ba9fde31a6 100644 --- a/go/vt/vtgate/planbuilder/abstract/queryprojection.go +++ b/go/vt/vtgate/planbuilder/abstract/queryprojection.go @@ -71,7 +71,6 @@ type ( // Aggr encodes all information needed for aggregation functions Aggr struct { Original *sqlparser.AliasedExpr - Func *sqlparser.FuncExpr OpCode engine.AggregateOpcode Alias string // The index at which the user expects to see this aggregated function. Set to nil, if the user does not ask for it @@ -495,7 +494,6 @@ func (qp *QueryProjection) AggregationExpressions() (out []Aggr, err error) { out = append(out, Aggr{ Original: aliasedExpr, - Func: fExpr, OpCode: opcode, Alias: aliasedExpr.ColumnName(), Index: &idxCopy, diff --git a/go/vt/vtgate/planbuilder/aggregation_pushing.go b/go/vt/vtgate/planbuilder/aggregation_pushing.go index 96105d95fcd..cb937910d8a 100644 --- a/go/vt/vtgate/planbuilder/aggregation_pushing.go +++ b/go/vt/vtgate/planbuilder/aggregation_pushing.go @@ -166,7 +166,7 @@ func addAggregationToSelect(sel *sqlparser.Select, aggregation abstract.Aggr) of if !isAliasedExpr { continue } - if sqlparser.EqualsExpr(aliasedExpr.Expr, aggregation.Func) { + if sqlparser.EqualsExpr(aliasedExpr.Expr, aggregation.Original.Expr) { return newOffset(i) } } @@ -184,7 +184,6 @@ func countStarAggr() *abstract.Aggr { return &abstract.Aggr{ Original: &sqlparser.AliasedExpr{Expr: f}, - Func: f, OpCode: engine.AggregateCount, Alias: "count(*)", } @@ -375,11 +374,11 @@ func splitAggregationsToLeftAndRight( var lhsAggrs, rhsAggrs []*abstract.Aggr for _, aggr := range aggregations { newAggr := aggr - if isCountStar(aggr.Func) { + if isCountStar(aggr.Original.Expr) { lhsAggrs = append(lhsAggrs, &newAggr) rhsAggrs = append(rhsAggrs, &newAggr) } else { - deps := ctx.SemTable.RecursiveDeps(aggr.Func) + deps := ctx.SemTable.RecursiveDeps(aggr.Original.Expr) var other *abstract.Aggr // if we are sending down min/max, we don't have to multiply the results with anything if !isMinOrMax(aggr.OpCode) { @@ -499,7 +498,7 @@ func sortOffsets(grouping []abstract.GroupBy, aggregations []abstract.Aggr) ([]a orderedAggrs := make([][]offsets, 0, len(originalAggr)) for _, og := range originalAggr { for i, g := range aggregations { - if og.Func == g.Func { + if og.Original.Expr == g.Original.Expr { orderedAggrs = append(orderedAggrs, aggrOffsets[i]) break } diff --git a/go/vt/vtgate/planbuilder/horizon_planning.go b/go/vt/vtgate/planbuilder/horizon_planning.go index 5fb0b759d87..81945b1f413 100644 --- a/go/vt/vtgate/planbuilder/horizon_planning.go +++ b/go/vt/vtgate/planbuilder/horizon_planning.go @@ -17,6 +17,7 @@ limitations under the License. package planbuilder import ( + "vitess.io/vitess/go/mysql/collations" "vitess.io/vitess/go/sqltypes" "vitess.io/vitess/go/vt/vtgate/planbuilder/abstract" "vitess.io/vitess/go/vt/vtgate/planbuilder/physical" @@ -322,7 +323,18 @@ func pushProjection( return key.KeyCol, false, nil } } - return 0, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "cannot push projections in ordered aggregates") + offset, _, err := pushProjection(ctx, expr, node.input, inner, true, hasAggregation) + if err != nil { + return 0, false, err + } + node.aggregates = append(node.aggregates, &engine.AggregateParams{ + Opcode: engine.AggregateRandom, + Col: offset, + Alias: expr.ColumnName(), + Expr: expr.Expr, + Original: expr, + }) + return offset, true, nil case *vindexFunc: colsBefore := len(node.eVindexFunc.Cols) i, err := node.SupplyProjection(expr, reuseCol) @@ -666,7 +678,7 @@ func generateAggregateParams(aggrs []abstract.Aggr, aggrParamOffsets [][]offsets Opcode: opcode, Col: offset, Alias: aggr.Alias, - Expr: aggr.Func, + Expr: aggr.Original.Expr, Original: aggr.Original, } } @@ -698,7 +710,11 @@ func addColumnsToOA( o := groupings[count] count++ a := aggregationExprs[offset] - collID := ctx.SemTable.CollationForExpr(a.Func.Exprs[0].(*sqlparser.AliasedExpr).Expr) + collID := collations.Unknown + fnc, ok := a.Original.Expr.(*sqlparser.FuncExpr) + if ok { + collID = ctx.SemTable.CollationForExpr(fnc.Exprs[0].(*sqlparser.AliasedExpr).Expr) + } oa.aggregates = append(oa.aggregates, &engine.AggregateParams{ Opcode: a.OpCode, Col: o.col, @@ -706,7 +722,6 @@ func addColumnsToOA( WAssigned: o.wsCol >= 0, WCol: o.wsCol, Alias: a.Alias, - Expr: a.Func, Original: a.Original, CollationID: collID, }) @@ -745,7 +760,8 @@ func (hp *horizonPlanning) handleDistinctAggr(ctx *plancontext.PlanningContext, aggrs = append(aggrs, expr) continue } - aliasedExpr, ok := expr.Func.Exprs[0].(*sqlparser.AliasedExpr) + funcExpr := expr.Original.Expr.(*sqlparser.FuncExpr) // we wouldn't be in this method if this wasn't a function + aliasedExpr, ok := funcExpr.Exprs[0].(*sqlparser.AliasedExpr) if !ok { err = vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "syntax error: %s", sqlparser.String(expr.Original)) return @@ -822,8 +838,9 @@ func (hp *horizonPlanning) createGroupingsForColumns(columns []*sqlparser.ColNam return lhsGrouping, nil } -func isCountStar(f *sqlparser.FuncExpr) bool { - if f == nil { +func isCountStar(e sqlparser.Expr) bool { + f, ok := e.(*sqlparser.FuncExpr) + if !ok || !f.Name.EqualString("count") { return false } _, isStar := f.Exprs[0].(*sqlparser.StarExpr) diff --git a/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt b/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt index b32677550c0..f9a06585e6a 100644 --- a/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt @@ -2112,7 +2112,35 @@ Gen4 plan same as above ] } } -Gen4 error: [BUG] unable to plan distinct query as the column is not projected: a +{ + "QueryType": "SELECT", + "Original": "select distinct a, count(*) from user", + "Instructions": { + "OperatorType": "Aggregate", + "Variant": "Ordered", + "GroupBy": "0, 1", + "Inputs": [ + { + "OperatorType": "Aggregate", + "Variant": "Scalar", + "Aggregates": "random(0) AS a, sum(1) AS count(*)", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select a, count(*) from `user` where 1 != 1", + "Query": "select a, count(*) from `user`", + "Table": "`user`" + } + ] + } + ] + } +} # distinct and aggregate functions "select distinct a, count(*) from user group by a" @@ -3243,7 +3271,46 @@ Gen4 plan same as above "Vindex": "name_user_map" } } -Gen4 error: cannot push projections in ordered aggregates +{ + "QueryType": "SELECT", + "Original": "select 1 from user having count(id) = 10 and name = 'a'", + "Instructions": { + "OperatorType": "SimpleProjection", + "Columns": [ + 0 + ], + "Inputs": [ + { + "OperatorType": "Filter", + "Predicate": "OFFSET(1, 'count(id)') = 10", + "Inputs": [ + { + "OperatorType": "Aggregate", + "Variant": "Scalar", + "Aggregates": "random(0) AS 1, sum(1) AS count(id)", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Equal", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select 1, count(id) from `user` where 1 != 1", + "Query": "select 1, count(id) from `user` where `name` = 'a'", + "Table": "`user`", + "Values": [ + "VARCHAR(\"a\")" + ], + "Vindex": "name_user_map" + } + ] + } + ] + } + ] + } +} # Aggregates and joins "select count(*) from user join user_extra" @@ -3315,7 +3382,42 @@ Gen4 error: cannot push projections in ordered aggregates "Table": "`user`" } } -Gen4 error: cannot push projections in ordered aggregates +{ + "QueryType": "SELECT", + "Original": "select 1 from user having count(id) = 10", + "Instructions": { + "OperatorType": "SimpleProjection", + "Columns": [ + 0 + ], + "Inputs": [ + { + "OperatorType": "Filter", + "Predicate": "OFFSET(1, 'count(id)') = 10", + "Inputs": [ + { + "OperatorType": "Aggregate", + "Variant": "Scalar", + "Aggregates": "random(0) AS 1, sum(1) AS count(id)", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select 1, count(id) from `user` where 1 != 1", + "Query": "select 1, count(id) from `user`", + "Table": "`user`" + } + ] + } + ] + } + ] + } +} # Aggregate on join "select user.a, count(*) from user join user_extra group by user.a" diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt index 4e178ef2920..8224ab51ea5 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt @@ -121,7 +121,7 @@ Gen4 error: aggregate functions take a single argument 'count(a, b)' "Instructions": { "OperatorType": "Aggregate", "Variant": "Scalar", - "Aggregates": "random(0) AS id, random(0) AS id, sum(2) AS count(*)", + "Aggregates": "random(0) AS id, random(1) AS id, sum(2) AS count(*)", "ResultColumns": 3, "Inputs": [ { @@ -237,7 +237,7 @@ Gen4 error: unsupported: in scatter query: complex aggregate expression { "OperatorType": "Join", "Variant": "Join", - "JoinColumnIndexes": "L:2,L:3,L:1,R:0", + "JoinColumnIndexes": "L:1,L:2,L:0,R:0", "JoinVars": { "user_col": 0 }, @@ -250,9 +250,9 @@ Gen4 error: unsupported: in scatter query: complex aggregate expression "Name": "user", "Sharded": true }, - "FieldQuery": "select `user`.col, `user`.col, `user`.id, weight_string(`user`.id) from `user` where 1 != 1 group by `user`.col, `user`.id, weight_string(`user`.id)", - "OrderBy": "(2|3) ASC", - "Query": "select `user`.col, `user`.col, `user`.id, weight_string(`user`.id) from `user` group by `user`.col, `user`.id, weight_string(`user`.id) order by `user`.id asc", + "FieldQuery": "select `user`.col, `user`.id, weight_string(`user`.id) from `user` where 1 != 1 group by `user`.col, `user`.id, weight_string(`user`.id)", + "OrderBy": "(1|2) ASC", + "Query": "select `user`.col, `user`.id, weight_string(`user`.id) from `user` group by `user`.col, `user`.id, weight_string(`user`.id) order by `user`.id asc", "Table": "`user`" }, { @@ -621,7 +621,62 @@ Gen4 plan same as above "select (select 1 from user u having count(ue.col) > 10) from user_extra ue" "symbol ue.col not found in subquery" -Gen4 error: cannot push projections in ordered aggregates +{ + "QueryType": "SELECT", + "Original": "select (select 1 from user u having count(ue.col) \u003e 10) from user_extra ue", + "Instructions": { + "OperatorType": "Subquery", + "Variant": "PulloutValue", + "PulloutVars": [ + "__sq1" + ], + "Inputs": [ + { + "OperatorType": "SimpleProjection", + "Columns": [ + 0 + ], + "Inputs": [ + { + "OperatorType": "Filter", + "Predicate": "OFFSET(1, 'count(ue.col)') \u003e 10", + "Inputs": [ + { + "OperatorType": "Aggregate", + "Variant": "Scalar", + "Aggregates": "random(0) AS 1, sum(1) AS count(ue.col)", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select 1, count(ue.col) from `user` as u where 1 != 1", + "Query": "select 1, count(ue.col) from `user` as u", + "Table": "`user`" + } + ] + } + ] + } + ] + }, + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select :__sq1 from user_extra as ue where 1 != 1", + "Query": "select :__sq1 from user_extra as ue", + "Table": "user_extra" + } + ] + } +} # subquery of information_schema with itself and star expression in outer select "select a.*, u.id from information_schema.a a, user u where a.id in (select * from information_schema.b)" From 69b01bf05a45a939b9580e9a1a4ba5fb157c7eb1 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 19 May 2022 12:35:01 +0200 Subject: [PATCH 7/8] test: update test expectations Signed-off-by: Andres Taylor --- .../queries/aggregation/aggregation_test.go | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go index 61c887ac74d..0e209222883 100644 --- a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go +++ b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go @@ -120,8 +120,7 @@ func TestEqualFilterOnScatter(t *testing.T) { mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a = \"5\"", `[[INT64(5)]]`) mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a = 5.00", `[[INT64(5)]]`) mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a, val1 from aggr_test group by val1 having a = 1.00", `[[INT64(1) VARCHAR("a")] [INT64(1) VARCHAR("b")] [INT64(1) VARCHAR("c")] [INT64(1) VARCHAR("d")] [INT64(1) VARCHAR("e")]]`) - - utils.AssertContainsError(t, mcmp.VtConn, "select /*vt+ PLANNER=gen4 */ 1 from aggr_test having count(*) = 5", `expr cannot be translated, not supported`) // will fail since `count(*)` is a FuncExpr + mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ 1 from aggr_test having count(*) = 5", `[[INT64(1)]]`) }) } } @@ -203,8 +202,7 @@ func TestNotEqualFilterOnScatter(t *testing.T) { mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a != \"1\"", `[[INT64(5)]]`) mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a != \"5\"", `[]`) mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a != 5.00", `[]`) - - utils.AssertContainsError(t, mcmp.VtConn, "select /*vt+ PLANNER=gen4 */ 1 from aggr_test having count(*) != 5", `expr cannot be translated, not supported`) // will fail since `count(*)` is a FuncExpr + mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ 1 from aggr_test having count(*) != 5", `[]`) }) } } @@ -227,8 +225,7 @@ func TestLessFilterOnScatter(t *testing.T) { mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a < \"10\"", `[[INT64(5)]]`) mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a < \"5\"", `[]`) mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a < 6.00", `[[INT64(5)]]`) - - utils.AssertContainsError(t, mcmp.VtConn, "select /*vt+ PLANNER=gen4 */ 1 from aggr_test having count(*) < 5", `expr cannot be translated, not supported`) // will fail since `count(*)` is a FuncExpr + mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ 1 from aggr_test having count(*) < 5", `[]`) }) } } @@ -252,8 +249,7 @@ func TestLessEqualFilterOnScatter(t *testing.T) { mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a <= \"10\"", `[[INT64(5)]]`) mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a <= \"5\"", `[[INT64(5)]]`) mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a <= 5.00", `[[INT64(5)]]`) - - utils.AssertContainsError(t, mcmp.VtConn, "select /*vt+ PLANNER=gen4 */ 1 from aggr_test having count(*) <= 5", `expr cannot be translated, not supported`) // will fail since `count(*)` is a FuncExpr + mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ 1 from aggr_test having count(*) <= 5", `[[INT64(1)]]`) }) } } @@ -277,8 +273,7 @@ func TestGreaterFilterOnScatter(t *testing.T) { mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a > \"1\"", `[[INT64(5)]]`) mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a > \"5\"", `[]`) mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a > 4.00", `[[INT64(5)]]`) - - utils.AssertContainsError(t, mcmp.VtConn, "select /*vt+ PLANNER=gen4 */ 1 from aggr_test having count(*) > 5", `expr cannot be translated, not supported`) // will fail since `count(*)` is a FuncExpr + mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ 1 from aggr_test having count(*) > 5", `[]`) }) } } @@ -302,8 +297,7 @@ func TestGreaterEqualFilterOnScatter(t *testing.T) { mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a >= \"1\"", `[[INT64(5)]]`) mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a >= \"5\"", `[[INT64(5)]]`) mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ count(*) as a from aggr_test having a >= 5.00", `[[INT64(5)]]`) - - utils.AssertContainsError(t, mcmp.VtConn, "select /*vt+ PLANNER=gen4 */ 1 from aggr_test having count(*) >= 5", `expr cannot be translated, not supported`) // will fail since `count(*)` is a FuncExpr + mcmp.AssertMatches("select /*vt+ PLANNER=gen4 */ 1 from aggr_test having count(*) >= 5", `[[INT64(1)]]`) }) } } From 96778ced9cfebd1daddf7a2f1c6c2740bc2b012c Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 19 May 2022 20:39:58 +0200 Subject: [PATCH 8/8] test refactor: move out test cases that pass from unsupported_cases Signed-off-by: Andres Taylor --- .../planbuilder/testdata/aggr_cases.txt | 254 +++++++++++++++ .../testdata/systemtables_cases.txt | 20 ++ .../testdata/unsupported_cases.txt | 302 ------------------ 3 files changed, 274 insertions(+), 302 deletions(-) diff --git a/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt b/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt index f9a06585e6a..a8ca9c018d0 100644 --- a/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt @@ -4514,3 +4514,257 @@ Gen4 error: aggregate functions take a single argument 'count(distinct user_id, ] } } + +"select (select 1 from user u having count(ue.col) > 10) from user_extra ue" +"symbol ue.col not found in subquery" +{ + "QueryType": "SELECT", + "Original": "select (select 1 from user u having count(ue.col) \u003e 10) from user_extra ue", + "Instructions": { + "OperatorType": "Subquery", + "Variant": "PulloutValue", + "PulloutVars": [ + "__sq1" + ], + "Inputs": [ + { + "OperatorType": "SimpleProjection", + "Columns": [ + 0 + ], + "Inputs": [ + { + "OperatorType": "Filter", + "Predicate": "OFFSET(1, 'count(ue.col)') \u003e 10", + "Inputs": [ + { + "OperatorType": "Aggregate", + "Variant": "Scalar", + "Aggregates": "random(0) AS 1, sum(1) AS count(ue.col)", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select 1, count(ue.col) from `user` as u where 1 != 1", + "Query": "select 1, count(ue.col) from `user` as u", + "Table": "`user`" + } + ] + } + ] + } + ] + }, + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select :__sq1 from user_extra as ue where 1 != 1", + "Query": "select :__sq1 from user_extra as ue", + "Table": "user_extra" + } + ] + } +} + +# group by and ',' joins with condition +"select user.col from user join user_extra on user_extra.col = user.col group by user.id" +"unsupported: cross-shard query with aggregates" +{ + "QueryType": "SELECT", + "Original": "select user.col from user join user_extra on user_extra.col = user.col group by user.id", + "Instructions": { + "OperatorType": "Aggregate", + "Variant": "Ordered", + "Aggregates": "random(0) AS col", + "GroupBy": "(2|1)", + "ResultColumns": 1, + "Inputs": [ + { + "OperatorType": "Projection", + "Expressions": [ + "[COLUMN 2] * [COLUMN 3] as col", + "[COLUMN 1]", + "[COLUMN 0] as id" + ], + "Inputs": [ + { + "OperatorType": "Join", + "Variant": "Join", + "JoinColumnIndexes": "L:1,L:2,L:0,R:0", + "JoinVars": { + "user_col": 0 + }, + "TableName": "`user`_user_extra", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select `user`.col, `user`.id, weight_string(`user`.id) from `user` where 1 != 1 group by `user`.col, `user`.id, weight_string(`user`.id)", + "OrderBy": "(1|2) ASC", + "Query": "select `user`.col, `user`.id, weight_string(`user`.id) from `user` group by `user`.col, `user`.id, weight_string(`user`.id) order by `user`.id asc", + "Table": "`user`" + }, + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select count(*) from user_extra where 1 != 1", + "Query": "select count(*) from user_extra where user_extra.col = :user_col", + "Table": "user_extra" + } + ] + } + ] + } + ] + } +} + + +# scatter aggregate symtab lookup error +"select id, b as id, count(*) from user order by id" +"ambiguous symbol reference: id" +{ + "QueryType": "SELECT", + "Original": "select id, b as id, count(*) from user order by id", + "Instructions": { + "OperatorType": "Aggregate", + "Variant": "Scalar", + "Aggregates": "random(0) AS id, random(1) AS id, sum(2) AS count(*)", + "ResultColumns": 3, + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select id, b as id, count(*), weight_string(b) from `user` where 1 != 1", + "OrderBy": "(1|3) ASC", + "Query": "select id, b as id, count(*), weight_string(b) from `user` order by id asc", + "Table": "`user`" + } + ] + } +} + +# aggr and non-aggr without group by (with query does not give useful result out) +"select id, count(*) from user" +{ + "QueryType": "SELECT", + "Original": "select id, count(*) from user", + "Instructions": { + "OperatorType": "Aggregate", + "Variant": "Scalar", + "Aggregates": "count(1)", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select id, count(*) from `user` where 1 != 1", + "Query": "select id, count(*) from `user`", + "Table": "`user`" + } + ] + } +} +{ + "QueryType": "SELECT", + "Original": "select id, count(*) from user", + "Instructions": { + "OperatorType": "Aggregate", + "Variant": "Scalar", + "Aggregates": "random(0) AS id, sum(1) AS count(*)", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select id, count(*) from `user` where 1 != 1", + "Query": "select id, count(*) from `user`", + "Table": "`user`" + } + ] + } +} + +# group by and ',' joins +"select user.id from user, user_extra group by id" +"unsupported: cross-shard query with aggregates" +{ + "QueryType": "SELECT", + "Original": "select user.id from user, user_extra group by id", + "Instructions": { + "OperatorType": "Aggregate", + "Variant": "Ordered", + "Aggregates": "random(0) AS id", + "GroupBy": "(2|1)", + "ResultColumns": 1, + "Inputs": [ + { + "OperatorType": "Projection", + "Expressions": [ + "[COLUMN 2] * [COLUMN 3] as id", + "[COLUMN 1]", + "[COLUMN 0] as id" + ], + "Inputs": [ + { + "OperatorType": "Join", + "Variant": "Join", + "JoinColumnIndexes": "L:0,L:1,L:0,R:0", + "TableName": "`user`_user_extra", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select `user`.id, weight_string(id) from `user` where 1 != 1 group by id, weight_string(id)", + "OrderBy": "(0|1) ASC", + "Query": "select `user`.id, weight_string(id) from `user` group by id, weight_string(id) order by id asc", + "Table": "`user`" + }, + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select count(*) from user_extra where 1 != 1", + "Query": "select count(*) from user_extra", + "Table": "user_extra" + } + ] + } + ] + } + ] + } +} diff --git a/go/vt/vtgate/planbuilder/testdata/systemtables_cases.txt b/go/vt/vtgate/planbuilder/testdata/systemtables_cases.txt index 86029491aae..465354d28f0 100644 --- a/go/vt/vtgate/planbuilder/testdata/systemtables_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/systemtables_cases.txt @@ -1431,3 +1431,23 @@ Gen4 plan same as above "Table": "information_schema.key_column_usage" } } + +# table_schema OR predicate +# It is unsupported because we do not route queries to multiple keyspaces right now +"SELECT * FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = 'ks' OR TABLE_SCHEMA = 'main'" +{ + "QueryType": "SELECT", + "Original": "SELECT * FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = 'ks' OR TABLE_SCHEMA = 'main'", + "Instructions": { + "OperatorType": "Route", + "Variant": "DBA", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "FieldQuery": "select * from INFORMATION_SCHEMA.`TABLES` where 1 != 1", + "Query": "select * from INFORMATION_SCHEMA.`TABLES` where TABLE_SCHEMA = 'ks' or TABLE_SCHEMA = 'main'", + "Table": "INFORMATION_SCHEMA.`TABLES`" + } +} +Gen4 plan same as above diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt index 8224ab51ea5..4124d264a29 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt @@ -68,35 +68,6 @@ Gen4 plan same as above "unsupported: '*' expression in cross-shard query" Gen4 error: cannot use column offsets in group statement when using `*` -# group by must reference select list -"select a from user group by b" -"unsupported: in scatter query: group by column must reference column in SELECT list" -{ - "QueryType": "SELECT", - "Original": "select a from user group by b", - "Instructions": { - "OperatorType": "Aggregate", - "Variant": "Ordered", - "Aggregates": "random(0) AS a", - "GroupBy": "(1|2)", - "ResultColumns": 1, - "Inputs": [ - { - "OperatorType": "Route", - "Variant": "Scatter", - "Keyspace": { - "Name": "user", - "Sharded": true - }, - "FieldQuery": "select a, b, weight_string(b) from `user` where 1 != 1 group by b, weight_string(b)", - "OrderBy": "(1|2) ASC", - "Query": "select a, b, weight_string(b) from `user` group by b, weight_string(b) order by b asc", - "Table": "`user`" - } - ] - } -} - # complex group by expression "select a from user group by a+1" "unsupported: in scatter query: only simple references allowed" @@ -112,34 +83,6 @@ Gen4 plan same as above "unsupported: only one expression allowed inside aggregates: count(a, b)" Gen4 error: aggregate functions take a single argument 'count(a, b)' -# scatter aggregate symtab lookup error -"select id, b as id, count(*) from user order by id" -"ambiguous symbol reference: id" -{ - "QueryType": "SELECT", - "Original": "select id, b as id, count(*) from user order by id", - "Instructions": { - "OperatorType": "Aggregate", - "Variant": "Scalar", - "Aggregates": "random(0) AS id, random(1) AS id, sum(2) AS count(*)", - "ResultColumns": 3, - "Inputs": [ - { - "OperatorType": "Route", - "Variant": "Scatter", - "Keyspace": { - "Name": "user", - "Sharded": true - }, - "FieldQuery": "select id, b as id, count(*), weight_string(b) from `user` where 1 != 1", - "OrderBy": "(1|3) ASC", - "Query": "select id, b as id, count(*), weight_string(b) from `user` order by id asc", - "Table": "`user`" - } - ] - } -} - # scatter aggregate complex order by "select id from user group by id order by id+1" "unsupported: in scatter query: complex order by expression: id + 1" @@ -155,125 +98,6 @@ Gen4 plan same as above "unsupported: cross-shard query with aggregates" Gen4 error: unsupported: in scatter query: complex aggregate expression -# group by and ',' joins -"select user.id from user, user_extra group by id" -"unsupported: cross-shard query with aggregates" -{ - "QueryType": "SELECT", - "Original": "select user.id from user, user_extra group by id", - "Instructions": { - "OperatorType": "Aggregate", - "Variant": "Ordered", - "Aggregates": "random(0) AS id", - "GroupBy": "(2|1)", - "ResultColumns": 1, - "Inputs": [ - { - "OperatorType": "Projection", - "Expressions": [ - "[COLUMN 2] * [COLUMN 3] as id", - "[COLUMN 1]", - "[COLUMN 0] as id" - ], - "Inputs": [ - { - "OperatorType": "Join", - "Variant": "Join", - "JoinColumnIndexes": "L:0,L:1,L:0,R:0", - "TableName": "`user`_user_extra", - "Inputs": [ - { - "OperatorType": "Route", - "Variant": "Scatter", - "Keyspace": { - "Name": "user", - "Sharded": true - }, - "FieldQuery": "select `user`.id, weight_string(id) from `user` where 1 != 1 group by id, weight_string(id)", - "OrderBy": "(0|1) ASC", - "Query": "select `user`.id, weight_string(id) from `user` group by id, weight_string(id) order by id asc", - "Table": "`user`" - }, - { - "OperatorType": "Route", - "Variant": "Scatter", - "Keyspace": { - "Name": "user", - "Sharded": true - }, - "FieldQuery": "select count(*) from user_extra where 1 != 1", - "Query": "select count(*) from user_extra", - "Table": "user_extra" - } - ] - } - ] - } - ] - } -} - -# group by and ',' joins with condition -"select user.col from user join user_extra on user_extra.col = user.col group by user.id" -"unsupported: cross-shard query with aggregates" -{ - "QueryType": "SELECT", - "Original": "select user.col from user join user_extra on user_extra.col = user.col group by user.id", - "Instructions": { - "OperatorType": "Aggregate", - "Variant": "Ordered", - "Aggregates": "random(0) AS col", - "GroupBy": "(2|1)", - "ResultColumns": 1, - "Inputs": [ - { - "OperatorType": "Projection", - "Expressions": [ - "[COLUMN 2] * [COLUMN 3] as col", - "[COLUMN 1]", - "[COLUMN 0] as id" - ], - "Inputs": [ - { - "OperatorType": "Join", - "Variant": "Join", - "JoinColumnIndexes": "L:1,L:2,L:0,R:0", - "JoinVars": { - "user_col": 0 - }, - "TableName": "`user`_user_extra", - "Inputs": [ - { - "OperatorType": "Route", - "Variant": "Scatter", - "Keyspace": { - "Name": "user", - "Sharded": true - }, - "FieldQuery": "select `user`.col, `user`.id, weight_string(`user`.id) from `user` where 1 != 1 group by `user`.col, `user`.id, weight_string(`user`.id)", - "OrderBy": "(1|2) ASC", - "Query": "select `user`.col, `user`.id, weight_string(`user`.id) from `user` group by `user`.col, `user`.id, weight_string(`user`.id) order by `user`.id asc", - "Table": "`user`" - }, - { - "OperatorType": "Route", - "Variant": "Scatter", - "Keyspace": { - "Name": "user", - "Sharded": true - }, - "FieldQuery": "select count(*) from user_extra where 1 != 1", - "Query": "select count(*) from user_extra where user_extra.col = :user_col", - "Table": "user_extra" - } - ] - } - ] - } - ] - } -} - # subqueries not supported in group by "select id from user group by id, (select id from user_extra)" "unsupported: subqueries disallowed in GROUP or ORDER BY" @@ -542,73 +366,6 @@ Gen4 plan same as above "Select query does not belong to the same keyspace as the view statement" Gen4 plan same as above -# table_schema OR predicate -# It is unsupported because we do not route queries to multiple keyspaces right now -"SELECT * FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = 'ks' OR TABLE_SCHEMA = 'main'" -{ - "QueryType": "SELECT", - "Original": "SELECT * FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = 'ks' OR TABLE_SCHEMA = 'main'", - "Instructions": { - "OperatorType": "Route", - "Variant": "DBA", - "Keyspace": { - "Name": "main", - "Sharded": false - }, - "FieldQuery": "select * from INFORMATION_SCHEMA.`TABLES` where 1 != 1", - "Query": "select * from INFORMATION_SCHEMA.`TABLES` where TABLE_SCHEMA = 'ks' or TABLE_SCHEMA = 'main'", - "Table": "INFORMATION_SCHEMA.`TABLES`" - } -} -Gen4 plan same as above - -# aggr and non-aggr without group by (with query does not give useful result out) -"select id, count(*) from user" -{ - "QueryType": "SELECT", - "Original": "select id, count(*) from user", - "Instructions": { - "OperatorType": "Aggregate", - "Variant": "Scalar", - "Aggregates": "count(1)", - "Inputs": [ - { - "OperatorType": "Route", - "Variant": "Scatter", - "Keyspace": { - "Name": "user", - "Sharded": true - }, - "FieldQuery": "select id, count(*) from `user` where 1 != 1", - "Query": "select id, count(*) from `user`", - "Table": "`user`" - } - ] - } -} -{ - "QueryType": "SELECT", - "Original": "select id, count(*) from user", - "Instructions": { - "OperatorType": "Aggregate", - "Variant": "Scalar", - "Aggregates": "random(0) AS id, sum(1) AS count(*)", - "Inputs": [ - { - "OperatorType": "Route", - "Variant": "Scatter", - "Keyspace": { - "Name": "user", - "Sharded": true - }, - "FieldQuery": "select id, count(*) from `user` where 1 != 1", - "Query": "select id, count(*) from `user`", - "Table": "`user`" - } - ] - } -} - # avg function on scatter query "select avg(id) from user" "unsupported: in scatter query: complex aggregate expression" @@ -619,65 +376,6 @@ Gen4 error: unsupported: in scatter query: aggregation function 'avg' "generating order by clause: ambiguous symbol reference: a" Gen4 plan same as above -"select (select 1 from user u having count(ue.col) > 10) from user_extra ue" -"symbol ue.col not found in subquery" -{ - "QueryType": "SELECT", - "Original": "select (select 1 from user u having count(ue.col) \u003e 10) from user_extra ue", - "Instructions": { - "OperatorType": "Subquery", - "Variant": "PulloutValue", - "PulloutVars": [ - "__sq1" - ], - "Inputs": [ - { - "OperatorType": "SimpleProjection", - "Columns": [ - 0 - ], - "Inputs": [ - { - "OperatorType": "Filter", - "Predicate": "OFFSET(1, 'count(ue.col)') \u003e 10", - "Inputs": [ - { - "OperatorType": "Aggregate", - "Variant": "Scalar", - "Aggregates": "random(0) AS 1, sum(1) AS count(ue.col)", - "Inputs": [ - { - "OperatorType": "Route", - "Variant": "Scatter", - "Keyspace": { - "Name": "user", - "Sharded": true - }, - "FieldQuery": "select 1, count(ue.col) from `user` as u where 1 != 1", - "Query": "select 1, count(ue.col) from `user` as u", - "Table": "`user`" - } - ] - } - ] - } - ] - }, - { - "OperatorType": "Route", - "Variant": "Scatter", - "Keyspace": { - "Name": "user", - "Sharded": true - }, - "FieldQuery": "select :__sq1 from user_extra as ue where 1 != 1", - "Query": "select :__sq1 from user_extra as ue", - "Table": "user_extra" - } - ] - } -} - # subquery of information_schema with itself and star expression in outer select "select a.*, u.id from information_schema.a a, user u where a.id in (select * from information_schema.b)" "unsupported: '*' expression in cross-shard query"