Merge pull request #3224 from dolthub/angela/groupby

Validate expressions in `ORDER BY` clause during `GROUP BY` validation

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,
 			},
 		},
 	},
@@ -365,4 +362,44 @@ var OrderByGroupByScriptTests = []ScriptTest{
 			},
 		},
 	},
+	{
+		Name: "valid group by order by queries",
+		SetUpScript: []string{
+			"create table t0(c0 int primary key, c1 int, c2 int, c3 int)",
+			"insert into t0 values (3, 1, 3, 1), (4, 1, 7, 2), (5, 2, 9, 3),(6,2, 1, 3), (7,2, 2, 2),(8,3,2, 5)",
+		},
+		Assertions: []ScriptTestAssertion{
+			{
+				// group by primary key
+				Query:    "select c1 from t0 group by c0 order by c2",
+				Expected: []sql.Row{{2}, {2}, {3}, {1}, {1}, {2}},
+			},
+			{
+				// order by aggregate
+				Query:    "select c1 from t0 group by c1 order by min(c2)",
+				Expected: []sql.Row{{2}, {3}, {1}},
+			},
+			{
+				// order by alias for column in group by clause
+				Query:    "select c1 as col from t0 group by c1 order by col",
+				Expected: []sql.Row{{1}, {2}, {3}},
+			},
+			{
+				// order by alias for aggregate column
+				Query:    "select min(c0) as min, c1 from t0 group by c1 order by min",
+				Expected: []sql.Row{{3, 1}, {5, 2}, {8, 3}},
+			},
+			{
+				// order by multiple columns
+				Query:    "select c1 from t0 group by c1, c2, c3 order by c2, c3",
+				Expected: []sql.Row{{2}, {2}, {3}, {1}, {1}, {2}},
+			},
+			{
+				// order by functionally dependent column
+				Dialect:  "mysql",
+				Query:    "select c1 from t0 where c2 = 3 group by c1 order by c2",
+				Expected: []sql.Row{{1}},
+			},
+		},
+	},
 }

enginetest/queries/queries.go

@@ -10423,6 +10423,10 @@ var ErrorQueries = []QueryErrorTest{
 		Query:       "select * from two_pk group by pk1 + 1, mod(pk2, 2)",
 		ExpectedErr: analyzererrors.ErrValidationGroupBy,
 	},
+	{
+		Query:       `select s from mytable group by s order by i`,
+		ExpectedErr: analyzererrors.ErrValidationGroupByOrderBy,
+	},
 }
 
 var BrokenErrorQueries = []QueryErrorTest{

sql/analyzer/analyzererrors/errors.go

@@ -19,33 +19,49 @@ import "gopkg.in/src-d/go-errors.v1"
 var (
 	// ErrValidationResolved is returned when the plan can not be resolved.
 	ErrValidationResolved = errors.NewKind("plan is not resolved because of node '%T'")
+
 	// 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 clause
 	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 is returned when an order by expression contains a nonaggregated column that does not
+	// appear in the group by clause
+	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
 	// that does not match the table name.
 	ErrValidationSchemaSource = errors.NewKind("one or more schema sources are empty")
+
 	// ErrUnknownIndexColumns is returned when there are columns in the expr
 	// to index that are unknown in the table.
 	ErrUnknownIndexColumns = errors.NewKind("unknown columns to index for table %q: %s")
+
 	// ErrCaseResultType is returned when one or more of the types of the values in
 	// a case expression don't match.
 	ErrCaseResultType = errors.NewKind(
 		"expecting all case branches to return values of type %s, " +
 			"but found value %q of type %s on %s",
 	)
+
 	// ErrIntervalInvalidUse is returned when an interval expression is not
 	// correctly used.
 	ErrIntervalInvalidUse = errors.NewKind(
 		"invalid use of an interval, which can only be used with DATE_ADD, " +
 			"DATE_SUB and +/- operators to subtract from or add to a date",
 	)
+
 	// ErrExplodeInvalidUse is returned when an EXPLODE function is used
 	// outside a Project node.
 	ErrExplodeInvalidUse = errors.NewKind(

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,36 @@ 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 when there's not an
+			// explicit GROUP BY in the query ("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 queries in MySQL, it doesn't seem like they actually
+			// 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 +368,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, nil
 	} 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