Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Gen4 only_full_group_by regression #10069

Merged
merged 2 commits into from
Apr 12, 2022
Merged

Fix Gen4 only_full_group_by regression #10069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da3b4c8
fix: directly use vindexes.Table instead of looking it up using a Tab…
frouioui Apr 11, 2022
42e5a61
fix: remove unrequired plan tests
frouioui Apr 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 12 additions & 19 deletions go/vt/vtgate/planbuilder/horizon_planning.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
19 changes: 19 additions & 0 deletions go/vt/vtgate/planbuilder/testdata/select_cases.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
systay marked this conversation as resolved.
Show resolved Hide resolved