validate order by expressions

Author: angelamayxie

enginetest/enginetests.go

@@ -828,7 +828,7 @@ func TestOrderByGroupBy(t *testing.T, harness Harness) {
 		// group by with any_value or non-strict are non-deterministic (unless there's only one value), so we must accept multiple
 		// group by with any_value()
 
-		_, rowIter, _, err = e.Query(ctx, "select any_value(id), team from members group by team order by id")
+		_, rowIter, _, err = e.Query(ctx, "select any_value(id), team from members group by team")
 		require.NoError(t, err)
 		rowCount = 0
 
@@ -867,6 +867,7 @@ func TestOrderByGroupBy(t *testing.T, harness Harness) {
 		require.Equal(t, rowCount, 3)
 
 		AssertErr(t, e, harness, "select id, team from members group by team order by id", nil, analyzererrors.ErrValidationGroupBy)
+		AssertErr(t, e, harness, "select any_value(id), team from members group by team order by id", nil, analyzererrors.ErrValidationGroupByOrderBy)
 	})
 }
 

enginetest/queries/order_by_group_by_queries.go

@@ -109,11 +109,8 @@ var OrderByGroupByScriptTests = []ScriptTest{
 				},
 			},
 			{
-				Query: "select binary s from t group by binary s order by s",
-				Expected: []sql.Row{
-					{[]uint8("abc")},
-					{[]uint8("def")},
-				},
+				Query:       "select binary s from t group by binary s order by s",
+				ExpectedErr: analyzererrors.ErrValidationGroupByOrderBy,
 			},
 		},
 	},

sql/analyzer/analyzererrors/errors.go

@@ -22,10 +22,17 @@ var (
 	// ErrValidationOrderBy is returned when the order by contains aggregation
 	// expressions.
 	ErrValidationOrderBy = errors.NewKind("OrderBy does not support aggregation expressions")
-	// ErrValidationGroupBy is returned when the aggregation expression does not
-	// appear in the grouping columns.
+	// ErrValidationGroupBy is returned when a selected expression contains a nonaggregated column that does not appear
+	// in the group by clauses.
 	ErrValidationGroupBy = errors.NewKind(
-		"Expression #%d of SELECT list is not in GROUP BY clause; " +
+		"Expression #%d of SELECT list is not in GROUP BY clause and contains nonaggregated column '%s' which " +
+			"is not functionally dependent on columns in GROUP BY clause; " +
+			"this is incompatible with sql_mode=only_full_group_by",
+	)
+
+	ErrValidationGroupByOrderBy = errors.NewKind(
+		"Expression #%d of ORDER BY clause is not in GROUP BY clause and contains nonaggregated column '%s' which " +
+			"is not functionally dependent on columns in GROUP BY clause; " +
 			"this is incompatible with sql_mode=only_full_group_by",
 	)
 	// ErrValidationSchemaSource is returned when there is any column source

sql/analyzer/validation_rules.go

@@ -252,6 +252,7 @@ func validateGroupBy(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scop
 
 	var err error
 	var project *plan.Project
+	var orderBy *plan.Sort
 	transform.Inspect(n, func(n sql.Node) bool {
 		switch n := n.(type) {
 		case *plan.GroupBy:
@@ -307,20 +308,35 @@ func validateGroupBy(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scop
 				return true
 			}
 
-			selectExprs := getSelectExprs(project, n.SelectDeps, groupBys)
+			selectExprs, orderByExprs := getSelectAndOrderByExprs(project, orderBy, n.SelectDeps, groupBys)
 
 			for i, expr := range selectExprs {
 				if valid, col := expressionReferencesOnlyGroupBys(groupBys, expr, noGroupBy); !valid {
 					if noGroupBy {
 						err = sql.ErrNonAggregatedColumnWithoutGroupBy.New(i+1, col)
 					} else {
-						err = analyzererrors.ErrValidationGroupBy.New(i + 1)
+						err = analyzererrors.ErrValidationGroupBy.New(i+1, col)
 					}
 					return false
 				}
 			}
+			// According to MySQL documentation, we should still be validating ORDER BY expressions ("If a query has
+			// aggregate functions and no GROUP BY clause, it cannot have nonaggregated columns in the select list, HAVING
+			// condition, or ORDER BY list with ONLY_FULL_GROUP_BY enabled"). But when testing actual queries in MySQL,
+			// it doesn't seem like they validate ORDER BY expressions in aggregate queries without an explicit GROUP BY
+			if !noGroupBy {
+				for i, expr := range orderByExprs {
+					if valid, col := expressionReferencesOnlyGroupBys(groupBys, expr, noGroupBy); !valid {
+						err = analyzererrors.ErrValidationGroupByOrderBy.New(i+1, col)
+						return false
+					}
+				}
+			}
 		case *plan.Project:
 			project = n
+			orderBy = nil
+		case *plan.Sort:
+			orderBy = n
 		}
 		return true
 	})
@@ -351,42 +367,56 @@ func getEqualsDependencies(expr sql.Expression) []sql.Expression {
 
 // getSelectExprs transforms the projection expressions from a Project node such that it uses the appropriate select
 // dependency expressions.
-func getSelectExprs(project *plan.Project, selectDeps []sql.Expression, groupBys map[string]bool) []sql.Expression {
-	if project == nil {
-		return selectDeps
+func getSelectAndOrderByExprs(project *plan.Project, orderBy *plan.Sort, selectDeps []sql.Expression, groupBys map[string]bool) ([]sql.Expression, []sql.Expression) {
+	if project == nil && orderBy == nil {
+		return selectDeps, make([]sql.Expression, 0)
 	} else {
 		sd := make(map[string]sql.Expression, len(selectDeps))
 		for _, dep := range selectDeps {
 			sd[strings.ToLower(dep.String())] = dep
 		}
 
 		selectExprs := make([]sql.Expression, 0)
+		orderByExprs := make([]sql.Expression, 0)
 
 		for _, expr := range project.Projections {
 			if !project.AliasDeps[strings.ToLower(expr.String())] {
-				resolvedExpr, _, _ := transform.Expr(expr, func(expr sql.Expression) (sql.Expression, transform.TreeIdentity, error) {
-					if groupBys[strings.ToLower(expr.String())] {
-						return expr, transform.SameTree, nil
-					}
-					switch expr := expr.(type) {
-					case *expression.Alias:
-						if dep, ok := sd[strings.ToLower(expr.Child.String())]; ok {
-							return dep, transform.NewTree, nil
-						}
-					case *expression.GetField:
-						if dep, ok := sd[strings.ToLower(expr.String())]; ok {
-							return dep, transform.NewTree, nil
-						}
-					}
-					return expr, transform.SameTree, nil
-				})
+				resolvedExpr := resolveExpr(expr, sd, groupBys)
 				selectExprs = append(selectExprs, resolvedExpr)
 			}
 		}
-		return selectExprs
+
+		if orderBy != nil {
+			for _, expr := range orderBy.Expressions() {
+				resolvedExpr := resolveExpr(expr, sd, groupBys)
+				orderByExprs = append(orderByExprs, resolvedExpr)
+			}
+		}
+
+		return selectExprs, orderByExprs
 	}
 }
 
+func resolveExpr(expr sql.Expression, selectDeps map[string]sql.Expression, groupBys map[string]bool) sql.Expression {
+	resolvedExpr, _, _ := transform.Expr(expr, func(expr sql.Expression) (sql.Expression, transform.TreeIdentity, error) {
+		if groupBys[strings.ToLower(expr.String())] {
+			return expr, transform.SameTree, nil
+		}
+		switch expr := expr.(type) {
+		case *expression.Alias:
+			if dep, ok := selectDeps[strings.ToLower(expr.Child.String())]; ok {
+				return dep, transform.NewTree, nil
+			}
+		case *expression.GetField:
+			if dep, ok := selectDeps[strings.ToLower(expr.String())]; ok {
+				return dep, transform.NewTree, nil
+			}
+		}
+		return expr, transform.SameTree, nil
+	})
+	return resolvedExpr
+}
+
 // expressionReferencesOnlyGroupBys validates that an expression is dependent on only group by expressions
 func expressionReferencesOnlyGroupBys(groupBys map[string]bool, expr sql.Expression, noGroupBy bool) (bool, string) {
 	var col string