Repository: vitessio/vitess
Pull request #9939 (2022-03-22 09:27:47)

ORDER BY scoping rules should not be used in group_concat
## Description
We do some special handling of scopes in ORDER BY, to allow the use of both aliases introduced in the SELECT expressions, but also columns coming from a table not listed in the SELECT list. These scoping rules should not be used on the ORDER BY that can be used inside the `group_concat` aggregation function.

## Checklist
- [x] Should this PR be backported? ``Maybe. It's simple and safe enough``
- [x] Tests were added or are not required
- [x] Documentation was added or is not required

systay: > We should also add a plan with `group_concat` in a join / sharded plan.

I don't think we support that today

Patch:
diff --git a/go/vt/vtgate/semantics/analyzer.go b/go/vt/vtgate/semantics/analyzer.go
index df5e251c42..5b83bb6efb 100644
--- a/go/vt/vtgate/semantics/analyzer.go
+++ b/go/vt/vtgate/semantics/analyzer.go
@@ -218,6 +218,11 @@ func isParentSelect(cursor *sqlparser.Cursor) bool {
 	return isSelect
 }
 
+func isParentSelectStatement(cursor *sqlparser.Cursor) bool {
+	_, isSelect := cursor.Parent().(sqlparser.SelectStatement)
+	return isSelect
+}
+
 type originable interface {
 	tableSetFor(t *sqlparser.AliasedTableExpr) TableSet
 	depsForExpr(expr sqlparser.Expr) (direct, recursive TableSet, typ *Type)
diff --git a/go/vt/vtgate/semantics/scoper.go b/go/vt/vtgate/semantics/scoper.go
index fc4683cd58..fcc4455f54 100644
--- a/go/vt/vtgate/semantics/scoper.go
+++ b/go/vt/vtgate/semantics/scoper.go
@@ -91,14 +91,16 @@ func (s *scoper) down(cursor *sqlparser.Cursor) error {
 		}
 		wScope.tables = []TableInfo{createVTableInfoForExpressions(node, s.currentScope().tables, s.org)}
 	case sqlparser.OrderBy:
-		err := s.createSpecialScopePostProjection(cursor.Parent())
-		if err != nil {
-			return err
-		}
-		for _, order := range node {
-			lit := keepIntLiteral(order.Expr)
-			if lit != nil {
-				s.specialExprScopes[lit] = s.currentScope()
+		if isParentSelectStatement(cursor) {
+			err := s.createSpecialScopePostProjection(cursor.Parent())
+			if err != nil {
+				return err
+			}
+			for _, order := range node {
+				lit := keepIntLiteral(order.Expr)
+				if lit != nil {
+					s.specialExprScopes[lit] = s.currentScope()
+				}
 			}
 		}
 	case sqlparser.GroupBy:
@@ -143,7 +145,11 @@ func keepIntLiteral(e sqlparser.Expr) *sqlparser.Literal {
 func (s *scoper) up(cursor *sqlparser.Cursor) error {
 	node := cursor.Node()
 	switch node := node.(type) {
-	case *sqlparser.Select, sqlparser.OrderBy, sqlparser.GroupBy:
+	case sqlparser.OrderBy:
+		if isParentSelectStatement(cursor) {
+			s.popScope()
+		}
+	case *sqlparser.Select, sqlparser.GroupBy:
 		s.popScope()
 	case *sqlparser.Where:
 		if node.Type != sqlparser.HavingClause {


Test patch:
diff --git a/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt b/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt
index f69053c43e..853b18d915 100644
--- a/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt
+++ b/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt
@@ -3857,6 +3857,25 @@ Gen4 error: cannot push projections in ordered aggregates
   }
 }
 
+# group_concat on single shards
+"select group_concat(user_id order by name), id from user group by id"
+{
+  "QueryType": "SELECT",
+  "Original": "select group_concat(user_id order by name), id from user group by id",
+  "Instructions": {
+    "OperatorType": "Route",
+    "Variant": "Scatter",
+    "Keyspace": {
+      "Name": "user",
+      "Sharded": true
+    },
+    "FieldQuery": "select group_concat(user_id order by `name` asc), id from `user` where 1 != 1 group by id",
+    "Query": "select group_concat(user_id order by `name` asc), id from `user` group by id",
+    "Table": "`user`"
+  }
+}
+Gen4 plan same as above
+
 "select count(distinct user_id, name) from unsharded"
 {
   "QueryType": "SELECT",