From ac2102703be038c341b4800188adc38c8f4db4ff Mon Sep 17 00:00:00 2001 From: FlorentP <35779988+frouioui@users.noreply.github.com> Date: Tue, 12 Apr 2022 17:03:04 +0200 Subject: [PATCH] Fix Gen4 only_full_group_by regression (#10069) * fix: directly use vindexes.Table instead of looking it up using a TableName Signed-off-by: Florent Poinsard * fix: remove unrequired plan tests Signed-off-by: Florent Poinsard --- go/vt/vtgate/planbuilder/horizon_planning.go | 31 +++++++------------ .../planbuilder/testdata/select_cases.txt | 19 ++++++++++++ 2 files changed, 31 insertions(+), 19 deletions(-) diff --git a/go/vt/vtgate/planbuilder/horizon_planning.go b/go/vt/vtgate/planbuilder/horizon_planning.go index 401de2ada5c..c331170c668 100644 --- a/go/vt/vtgate/planbuilder/horizon_planning.go +++ b/go/vt/vtgate/planbuilder/horizon_planning.go @@ -466,7 +466,7 @@ func checkIfAlreadyExists(expr *sqlparser.AliasedExpr, node sqlparser.SelectStat func (hp *horizonPlanning) planAggregations(ctx *plancontext.PlanningContext, plan logicalPlan) (logicalPlan, error) { isPushable := !isJoin(plan) grouping := hp.qp.GetGrouping() - vindexOverlapWithGrouping := hasUniqueVindex(ctx.VSchema, ctx.SemTable, grouping) + vindexOverlapWithGrouping := hasUniqueVindex(ctx.SemTable, grouping) if isPushable && vindexOverlapWithGrouping { // If we have a plan that we can push the group by and aggregation through, we don't need to do aggregation // at the vtgate level at all @@ -736,7 +736,7 @@ func (hp *horizonPlanning) handleDistinctAggr(ctx *plancontext.PlanningContext, if err != nil { return nil, nil, nil, err } - if exprHasVindex(ctx.VSchema, ctx.SemTable, innerWS, false) { + if exprHasVindex(ctx.SemTable, innerWS, false) { aggrs = append(aggrs, expr) continue } @@ -812,9 +812,9 @@ func isCountStar(f *sqlparser.FuncExpr) bool { return isStar } -func hasUniqueVindex(vschema plancontext.VSchema, semTable *semantics.SemTable, groupByExprs []abstract.GroupBy) bool { +func hasUniqueVindex(semTable *semantics.SemTable, groupByExprs []abstract.GroupBy) bool { for _, groupByExpr := range groupByExprs { - if exprHasUniqueVindex(vschema, semTable, groupByExpr.WeightStrExpr) { + if exprHasUniqueVindex(semTable, groupByExpr.WeightStrExpr) { return true } } @@ -1150,7 +1150,7 @@ func (hp *horizonPlanning) planDistinct(ctx *plancontext.PlanningContext, plan l // we always make the underlying query distinct, // and then we might also add a distinct operator on top if it is needed p.Select.MakeDistinct() - if p.isSingleShard() || selectHasUniqueVindex(ctx.VSchema, ctx.SemTable, hp.qp.SelectExprs) { + if p.isSingleShard() || selectHasUniqueVindex(ctx.SemTable, hp.qp.SelectExprs) { return plan, nil } @@ -1275,14 +1275,14 @@ func isAmbiguousOrderBy(index int, col sqlparser.ColIdent, exprs []abstract.Sele return false } -func selectHasUniqueVindex(vschema plancontext.VSchema, semTable *semantics.SemTable, sel []abstract.SelectExpr) bool { +func selectHasUniqueVindex(semTable *semantics.SemTable, sel []abstract.SelectExpr) bool { for _, expr := range sel { exp, err := expr.GetExpr() if err != nil { // TODO: handle star expression error return false } - if exprHasUniqueVindex(vschema, semTable, exp) { + if exprHasUniqueVindex(semTable, exp) { return true } } @@ -1313,7 +1313,7 @@ func (hp *horizonPlanning) needDistinctHandling( // Unreachable return true, innerAliased, nil } - if exprHasUniqueVindex(ctx.VSchema, ctx.SemTable, innerAliased.Expr) { + if exprHasUniqueVindex(ctx.SemTable, innerAliased.Expr) { // if we can see a unique vindex on this table/column, // we know the results will be unique, and we don't need to DISTINCTify them return false, nil, nil @@ -1353,11 +1353,11 @@ func isJoin(plan logicalPlan) bool { } } -func exprHasUniqueVindex(vschema plancontext.VSchema, semTable *semantics.SemTable, expr sqlparser.Expr) bool { - return exprHasVindex(vschema, semTable, expr, true) +func exprHasUniqueVindex(semTable *semantics.SemTable, expr sqlparser.Expr) bool { + return exprHasVindex(semTable, expr, true) } -func exprHasVindex(vschema plancontext.VSchema, semTable *semantics.SemTable, expr sqlparser.Expr, hasToBeUnique bool) bool { +func exprHasVindex(semTable *semantics.SemTable, expr sqlparser.Expr, hasToBeUnique bool) bool { col, isCol := expr.(*sqlparser.ColName) if !isCol { return false @@ -1367,14 +1367,7 @@ func exprHasVindex(vschema plancontext.VSchema, semTable *semantics.SemTable, ex if err != nil { return false } - tableName, err := tableInfo.Name() - if err != nil { - return false - } - vschemaTable, _, _, _, _, err := vschema.FindTableOrVindex(tableName) - if err != nil { - return false - } + vschemaTable := tableInfo.GetVindexTable() for _, vindex := range vschemaTable.ColumnVindexes { if len(vindex.Columns) > 1 || hasToBeUnique && !vindex.IsUnique() { return false diff --git a/go/vt/vtgate/planbuilder/testdata/select_cases.txt b/go/vt/vtgate/planbuilder/testdata/select_cases.txt index f063dfeac73..82df4fb6afe 100644 --- a/go/vt/vtgate/planbuilder/testdata/select_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/select_cases.txt @@ -3229,3 +3229,22 @@ Gen4 plan same as above } } Gen4 plan same as above + +# groupe by with non aggregated columns and table alias +"select u.id, u.age from user u group by u.id" +{ + "QueryType": "SELECT", + "Original": "select u.id, u.age from user u group by u.id", + "Instructions": { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select u.id, u.age from `user` as u where 1 != 1 group by u.id", + "Query": "select u.id, u.age from `user` as u group by u.id", + "Table": "`user`" + } +} +Gen4 plan same as above