Merge pull request #3194 from dolthub/angela/groupby

Group by validation for aggregated queries

Author: angelamayxie

enginetest/queries/column_alias_queries.go

@@ -269,11 +269,17 @@ var ColumnAliasQueries = []ScriptTest{
 		Assertions: []ScriptTestAssertion{
 			{
 				// GMS returns "expression 'dt.two' doesn't appear in the group by expressions", but MySQL will execute
-				// this query.
+				// this query. https://github.com/dolthub/dolt/issues/9717
 				Query: "select 1 as a, one + 1 as mod1, dt.* from mytable as t1, (select 1, 2 from mytable) as dt (one, two) where dt.one > 0 group by one;",
 				// column names:  a, mod1, one, two
 				Expected: []sql.Row{{1, 2, 1, 2}},
 			},
+			{
+				// MySQL will execute this query but group by validation isn't able to recognize that exprAlias is a
+				// literal https://github.com/dolthub/dolt/issues/9717
+				Query:    "select 1 as exprAlias, 2, 3, (select exprAlias + count(*) from one_pk_three_idx a cross join one_pk_three_idx b);",
+				Expected: []sql.Row{{1, 2, 3, 65}},
+			},
 		},
 	},
 }

enginetest/queries/join_queries.go

@@ -187,7 +187,7 @@ inner join pq on true order by 1,2,3,4,5,6,7,8 limit 5;`,
 	},
 	{
 		// test cross join used as projected subquery expression
-		Query:    "select 1 as exprAlias, 2, 3, (select exprAlias + count(*) from one_pk_three_idx a cross join one_pk_three_idx b);",
+		Query:    "select 1, 2, 3, (select 1 + count(*) from one_pk_three_idx a cross join one_pk_three_idx b);",
 		Expected: []sql.Row{{1, 2, 3, 65}},
 	},
 	{

enginetest/queries/order_by_group_by_queries.go

@@ -199,9 +199,6 @@ var OrderByGroupByScriptTests = []ScriptTest{
 				ExpectedErr: sql.ErrNonAggregatedColumnWithoutGroupBy,
 			},
 			{
-				// GMS currently allows this query to execute and chooses the first result for val.
-				// To match MySQL's behavior, GMS should be throwing an ErrNonAggregatedColumnWithoutGroupBy error.
-				Skip:        true,
 				Query:       "select AVG(val), val from t;",
 				ExpectedErr: sql.ErrNonAggregatedColumnWithoutGroupBy,
 			},

enginetest/queries/queries.go

@@ -1238,18 +1238,6 @@ SELECT * FROM cte WHERE  d = 2;`,
 			{int64(3), "third row"},
 		},
 	},
-	{
-		Query: "SELECT count(*), i, concat(i, i), 123, 'abc', concat('abc', 'def') FROM emptytable;",
-		Expected: []sql.Row{
-			{0, nil, nil, 123, "abc", "abcdef"},
-		},
-	},
-	{
-		Query: "SELECT count(*), i, concat(i, i), 123, 'abc', concat('abc', 'def') FROM mytable where false;",
-		Expected: []sql.Row{
-			{0, nil, nil, 123, "abc", "abcdef"},
-		},
-	},
 	{
 		Query: "SELECT pk, u, v FROM one_pk JOIN (SELECT count(*) AS u, 123 AS v FROM emptytable) uv WHERE pk = u;",
 		Expected: []sql.Row{
@@ -7877,10 +7865,6 @@ SELECT * FROM ladder;`,
 		Query:    "with a(j) as (select 1) ( with c(k) as (select 3) select k from c union select 6) union select k from c;",
 		Expected: []sql.Row{{3}, {6}},
 	},
-	{
-		Query:    "SELECT pk1, SUM(c1) FROM two_pk",
-		Expected: []sql.Row{{0, 60.0}},
-	},
 	{
 		Query: `SELECT pk,
 						(SELECT sum(c1) FROM two_pk WHERE c1 IN (SELECT c4 FROM two_pk WHERE c3 > opk.c5)) AS sum,

enginetest/queries/query_plans.go

@@ -2795,7 +2795,7 @@ CREATE TABLE tab3 (
 			},
 			{
 				// Test with subquery using multiple columns errors
-				Query:       "SELECT category, GROUP_CONCAT(name ORDER BY (SELECT AVG(value), name FROM complex_test c2 WHERE c2.id <= complex_test.id HAVING AVG(value) > 50) DESC) FROM complex_test GROUP BY category ORDER BY category",
+				Query:       "SELECT category, GROUP_CONCAT(name ORDER BY (SELECT value, name FROM complex_test c2 WHERE c2.id <= complex_test.id) DESC) FROM complex_test GROUP BY category ORDER BY category",
 				ExpectedErr: sql.ErrInvalidOperandColumns,
 			},
 			{
@@ -11697,11 +11697,9 @@ select * from t1 except (
 		Assertions: []ScriptTestAssertion{
 			{
 				// https://github.com/dolthub/dolt/issues/9761
-				Skip:  true,
-				Query: "SELECT pk, count(pk), MATCH(doc) AGAINST('aaaa') AS relevancy FROM test ORDER BY relevancy DESC;",
-				// TODO: replace with corresponding error once validation for aggregated query without group by
-				//  has been implemented https://github.com/dolthub/dolt/issues/9761
-				ExpectedErr: nil,
+				Skip:        true,
+				Query:       "SELECT pk, count(pk), MATCH(doc) AGAINST('aaaa') AS relevancy FROM test ORDER BY relevancy DESC;",
+				ExpectedErr: sql.ErrNonAggregatedColumnWithoutGroupBy,
 			},
 			{
 				Query:    "SET SESSION sql_mode = REPLACE(@@SESSION.sql_mode, 'ONLY_FULL_GROUP_BY', '');",
@@ -11714,6 +11712,51 @@ select * from t1 except (
 			},
 		},
 	},
+	{
+		Name:    "nonaggregated column in aggregated query",
+		Dialect: "mysql",
+		SetUpScript: []string{
+			"create table emptytable(i mediumint primary key, s varchar(20))",
+			"create table mytable(i bigint primary key, s varchar(20))",
+			"insert into mytable values (1, 'first'), (2, 'second'), (3, 'third');",
+			"create table two_pk (pk1 tinyint, pk2 tinyint, c1 tinyint NOT NULL, primary key (pk1, pk2))",
+			"insert into two_pk values (0,0,0), (0,1,10), (1,0,20), (1,1,30)",
+		},
+		Assertions: []ScriptTestAssertion{
+			{
+				Query:       "SELECT count(*), i, concat(i, i), 123, 'abc', concat('abc', 'def') FROM emptytable;",
+				ExpectedErr: sql.ErrNonAggregatedColumnWithoutGroupBy,
+			},
+			{
+				Query:       "SELECT count(*), i, concat(i, i), 123, 'abc', concat('abc', 'def') FROM mytable where false;",
+				ExpectedErr: sql.ErrNonAggregatedColumnWithoutGroupBy,
+			},
+			{
+				Query:       "SELECT pk1, SUM(c1) FROM two_pk",
+				ExpectedErr: sql.ErrNonAggregatedColumnWithoutGroupBy,
+			},
+			{
+				Query:    "SET SESSION sql_mode = REPLACE(@@SESSION.sql_mode, 'ONLY_FULL_GROUP_BY', '');",
+				Expected: []sql.Row{{types.NewOkResult(0)}},
+			},
+			{
+				Query: "SELECT count(*), i, concat(i, i), 123, 'abc', concat('abc', 'def') FROM emptytable;",
+				Expected: []sql.Row{
+					{0, nil, nil, 123, "abc", "abcdef"},
+				},
+			},
+			{
+				Query: "SELECT count(*), i, concat(i, i), 123, 'abc', concat('abc', 'def') FROM mytable where false;",
+				Expected: []sql.Row{
+					{0, nil, nil, 123, "abc", "abcdef"},
+				},
+			},
+			{
+				Query:    "SELECT pk1, SUM(c1) FROM two_pk",
+				Expected: []sql.Row{{0, 60.0}},
+			},
+		},
+	},
 }
 
 var SpatialScriptTests = []ScriptTest{

enginetest/queries/script_queries.go

@@ -255,10 +255,11 @@ func validateGroupBy(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scop
 	transform.Inspect(n, func(n sql.Node) bool {
 		switch n := n.(type) {
 		case *plan.GroupBy:
+			var noGroupBy bool
 			// Allow the parser use the GroupBy node to eval the aggregation functions for sql statements that don't
 			// make use of the GROUP BY expression.
 			if len(n.GroupByExprs) == 0 {
-				return true
+				noGroupBy = true
 			}
 
 			primaryKeys := make(map[string]bool)
@@ -309,8 +310,12 @@ func validateGroupBy(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scop
 			selectExprs := getSelectExprs(project, n.SelectDeps, groupBys)
 
 			for i, expr := range selectExprs {
-				if !expressionReferencesOnlyGroupBys(groupBys, expr) {
-					err = analyzererrors.ErrValidationGroupBy.New(i + 1)
+				if valid, col := expressionReferencesOnlyGroupBys(groupBys, expr, noGroupBy); !valid {
+					if noGroupBy {
+						err = sql.ErrNonAggregatedColumnWithoutGroupBy.New(i+1, col)
+					} else {
+						err = analyzererrors.ErrValidationGroupBy.New(i + 1)
+					}
 					return false
 				}
 			}
@@ -383,7 +388,8 @@ func getSelectExprs(project *plan.Project, selectDeps []sql.Expression, groupBys
 }
 
 // expressionReferencesOnlyGroupBys validates that an expression is dependent on only group by expressions
-func expressionReferencesOnlyGroupBys(groupBys map[string]bool, expr sql.Expression) bool {
+func expressionReferencesOnlyGroupBys(groupBys map[string]bool, expr sql.Expression, noGroupBy bool) (bool, string) {
+	var col string
 	valid := true
 	sql.Inspect(expr, func(expr sql.Expression) bool {
 		switch expr := expr.(type) {
@@ -401,15 +407,20 @@ func expressionReferencesOnlyGroupBys(groupBys map[string]bool, expr sql.Express
 			}
 
 			if len(expr.Children()) == 0 {
-				valid = false
+				// Allow non-matching subqueries when no explicit group by clause. If the subquery returns more than
+				// one row for an aggregated query, we will error out later on.
+				if _, isSubquery := expr.(*plan.Subquery); !(isSubquery && noGroupBy) {
+					valid = false
+					col = expr.String()
+				}
 				return false
 			}
 
 			return true
 		}
 	})
 
-	return valid
+	return valid, col
 }
 
 func validateSchemaSource(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scope, sel RuleSelector, qFlags *sql.QueryFlags) (sql.Node, transform.TreeIdentity, error) {