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 <[email protected]> * fix: remove unrequired plan tests Signed-off-by: Florent Poinsard <[email protected]>
vitessio · Apr 12, 2022 · ac21027 · ac21027
1 parent 5edbc68
commit ac21027
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 19 deletions.
diff --git 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
@@ -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