abandoning this for now

Author: angelamayxie

sql/analyzer/validation_rules.go

@@ -249,41 +249,59 @@ func validateGroupBy(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scop
 	}
 
 	var err error
-	//var parent sql.Node
+	var parent sql.Node
 	transform.Inspect(n, func(n sql.Node) bool {
-		//defer func() {
-		//	parent = n
-		//}()
+		defer func() {
+			parent = n
+		}()
 
 		gb, ok := n.(*plan.GroupBy)
 		if !ok {
 			return true
 		}
 
-		//switch parent.(type) {
-		//case *plan.Having, *plan.Project, *plan.Sort:
-		//	// TODO: these shouldn't be skipped; you can group by primary key without problem b/c only one value
-		//	// https://dev.mysql.com/doc/refman/8.0/en/group-by-handling.html#:~:text=The%20query%20is%20valid%20if%20name%20is%20a%20primary%20key
-		//	return true
-		//}
+		switch parent.(type) {
+		case *plan.Having, *plan.Project, *plan.Sort:
+			// TODO: these shouldn't be skipped; you can group by primary key without problem b/c only one value
+			// https://dev.mysql.com/doc/refman/8.0/en/group-by-handling.html#:~:text=The%20query%20is%20valid%20if%20name%20is%20a%20primary%20key
+			return true
+		}
 
 		// 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(gb.GroupByExprs) == 0 {
 			return true
 		}
 
-		var groupBys []string
+		primaryKeys := make(map[string]bool)
+		for _, col := range gb.Child.Schema() {
+			if col.PrimaryKey {
+				primaryKeys[strings.ToLower(col.Name)] = true
+			}
+		}
+
+		groupBys := make(map[string]bool)
+		groupByAliases := make(map[string]bool)
+		groupByPrimaryKeys := 0
 		for _, expr := range gb.GroupByExprs {
-			groupBys = append(groupBys, expr.String())
+			exprStr := strings.ToLower(expr.String())
+			groupBys[exprStr] = true
+			if primaryKeys[exprStr] {
+				groupByPrimaryKeys++
+			}
+			if _, ok := expr.(sql.Aggregation); ok {
+				groupByAliases[exprStr] = true
+			}
+		}
+
+		if len(primaryKeys) != 0 && groupByPrimaryKeys == len(primaryKeys) {
+			return true
 		}
 
 		for _, expr := range gb.SelectedExprs {
-			if _, ok := expr.(sql.Aggregation); !ok {
-				if !expressionReferencesOnlyGroupBys(groupBys, expr) {
-					err = analyzererrors.ErrValidationGroupBy.New(expr.String())
-					return false
-				}
+			if !expressionReferencesOnlyGroupBys(groupBys, groupByAliases, expr) {
+				err = analyzererrors.ErrValidationGroupBy.New(expr.String())
+				return false
 			}
 		}
 		return true
@@ -292,22 +310,15 @@ func validateGroupBy(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scop
 	return n, transform.SameTree, err
 }
 
-func expressionReferencesOnlyGroupBys(groupBys []string, expr sql.Expression) bool {
+func expressionReferencesOnlyGroupBys(groupBys, groupByAliases map[string]bool, expr sql.Expression) bool {
 	valid := true
 	sql.Inspect(expr, func(expr sql.Expression) bool {
+		exprStr := strings.ToLower(expr.String())
 		switch expr := expr.(type) {
 		case nil, sql.Aggregation, *expression.Literal:
 			return false
-		case *expression.Alias, sql.FunctionExpression:
-			if stringContains(groupBys, expr.String()) {
-				return false
-			}
-			return true
-		// cc: https://dev.mysql.com/doc/refman/8.0/en/group-by-handling.html
-		// Each part of the SelectExpr must refer to the aggregated columns in some way
-		// TODO: this isn't complete, it's overly restrictive. Dependant columns are fine to reference.
 		default:
-			if stringContains(groupBys, expr.String()) {
+			if groupBys[exprStr] {
 				return false
 			}
 

sql/plan/group_by.go

@@ -30,8 +30,17 @@ var ErrGroupBy = errors.NewKind("group by aggregation '%v' not supported")
 // GroupBy groups the rows by some expressions.
 type GroupBy struct {
 	UnaryNode
+	// SelectedExprs are projection dependencies. They are not the explicit select expressions. For example, given the
+	// query "SELECT pk div 2 from one_pk group by 1," SelectedExprs would contain a GetField for one_pk.pk even though
+	// the explicit select expression is "pk div 2".
 	SelectedExprs []sql.Expression
 	GroupByExprs  []sql.Expression
+	// SelectedAliasMap maps a projection dependency to an alias if it was part of one. For example, given the query
+	// "SELECT pk div 2, pk + 3 from one_pk group by 2", SelectedAliasMap would contain a mapping from the GetField for
+	// one_pk.pk to the Alias for "pk div 2" and to the Alias for "pk + 3". This allows for projection dependencies to
+	// be validated for GroupBy dependencies. Since SelectedAliasMap is only used for validating GroupBys dependencies,
+	// the expressions are stored as strings. A nested map is used for faster access during validation.
+	SelectedAliasMap map[string][]string
 }
 
 var _ sql.Expressioner = (*GroupBy)(nil)
@@ -43,11 +52,12 @@ var _ sql.CollationCoercible = (*GroupBy)(nil)
 // will appear in the output of the query. Some of these fields may be aggregate functions, some may be columns or
 // other expressions. Unlike a project, the GroupBy also has a list of group-by expressions, which usually also appear
 // in the list of selected expressions.
-func NewGroupBy(selectedExprs, groupByExprs []sql.Expression, child sql.Node) *GroupBy {
+func NewGroupBy(selectedExprs, groupByExprs []sql.Expression, selectedAliasMap map[string][]string, child sql.Node) *GroupBy {
 	return &GroupBy{
-		UnaryNode:     UnaryNode{Child: child},
-		SelectedExprs: selectedExprs,
-		GroupByExprs:  groupByExprs,
+		UnaryNode:        UnaryNode{Child: child},
+		SelectedExprs:    selectedExprs,
+		GroupByExprs:     groupByExprs,
+		SelectedAliasMap: selectedAliasMap,
 	}
 }
 
@@ -101,7 +111,7 @@ func (g *GroupBy) WithChildren(children ...sql.Node) (sql.Node, error) {
 		return nil, sql.ErrInvalidChildrenNumber.New(g, len(children), 1)
 	}
 
-	return NewGroupBy(g.SelectedExprs, g.GroupByExprs, children[0]), nil
+	return NewGroupBy(g.SelectedExprs, g.GroupByExprs, g.SelectedAliasMap, children[0]), nil
 }
 
 // CollationCoercibility implements the interface sql.CollationCoercible.
@@ -122,7 +132,7 @@ func (g *GroupBy) WithExpressions(exprs ...sql.Expression) (sql.Node, error) {
 	grouping := make([]sql.Expression, len(g.GroupByExprs))
 	copy(grouping, exprs[len(g.SelectedExprs):])
 
-	return NewGroupBy(agg, grouping, g.Child), nil
+	return NewGroupBy(agg, grouping, g.SelectedAliasMap, g.Child), nil
 }
 
 func (g *GroupBy) String() string {

sql/planbuilder/aggregates.go

@@ -203,6 +203,7 @@ func (b *Builder) buildAggregation(fromScope, projScope *scope, groupingCols []s
 	var selectExprs []sql.Expression
 	var selectGfs []sql.Expression
 	selectStr := make(map[string]bool)
+	aliasMap := make(map[string][]string)
 	for _, e := range group.aggregations() {
 		if !selectStr[strings.ToLower(e.String())] {
 			selectExprs = append(selectExprs, e.scalar)
@@ -212,12 +213,14 @@ func (b *Builder) buildAggregation(fromScope, projScope *scope, groupingCols []s
 	}
 	var aliases []sql.Expression
 	for _, col := range projScope.cols {
+		var isAlias bool
 		// eval aliases in project scope
 		switch e := col.scalar.(type) {
 		case *expression.Alias:
 			if !e.Unreferencable() {
 				aliases = append(aliases, e.WithId(sql.ColumnId(col.id)).(*expression.Alias))
 			}
+			isAlias = true
 		default:
 		}
 
@@ -231,6 +234,9 @@ func (b *Builder) buildAggregation(fromScope, projScope *scope, groupingCols []s
 					selectGfs = append(selectGfs, e)
 					selectStr[colName] = true
 				}
+				if isAlias {
+					aliasMap[colName] = append(aliasMap[colName], strings.ToLower(col.scalar.String()))
+				}
 			default:
 			}
 			return false
@@ -245,7 +251,7 @@ func (b *Builder) buildAggregation(fromScope, projScope *scope, groupingCols []s
 			selectStr[e.String()] = true
 		}
 	}
-	gb := plan.NewGroupBy(selectExprs, groupingCols, fromScope.node)
+	gb := plan.NewGroupBy(selectExprs, groupingCols, aliasMap, fromScope.node)
 	outScope.node = gb
 
 	if len(aliases) > 0 {