Repository: tobymao/sqlglot
Pull request: feat(mysql): Support for multi arg GROUP_CONCAT
Fixes #3142 

Change MySQL parsing for `exp.GroupConcat` to reduce/normalize the list of expressions into an `exp.Concat` node, thus making it easily transpilable to other dialects which only accept a single expression.   

Docs
------------
- https://dev.mysql.com/doc/refman/8.3/en/aggregate-functions.html#function_group-concat

Problem statement:
feat: support for mysql group_concat with multi columns
**Is your feature request related to a problem? Please describe.**

```python 
import sqlglot
sql = "select a, group_concat(b, ' ', c SEPARATOR ',') from table group by a;"
sqlglot.parse_one(sql, read="mysql")
# or 
sqlglot.transpile(sql, read="mysql", write="postgres", pretty=True)

```

```
sqlglot/parser.py:1255, in Parser.raise_error(self, message, token)
   1243 error = ParseError.new(
   1244     f"{message}. Line {token.line}, Col: {token.col}.\n"
   1245     f"  {start_context}\033[4m{highlight}\033[0m{end_context}",
   (...)
   1251     end_context=end_context,
   1252 )
   1254 if self.error_level == ErrorLevel.IMMEDIATE:
-> 1255     raise error
   1257 self.errors.append(error)

ParseError: Expecting ). Line 1, Col: 38.
```


**Describe the solution you'd like**

transpile the sql to postgresql
output:
select a STRING_AGG(b || ' ' || c, ',')  from table group  a;

**Additional context**
Add any other context or screenshots about the feature request here.

Hints from issue discussion:

Commit message: Refactor #1

Patch to review:
diff --git a/sqlglot/dialects/mysql.py b/sqlglot/dialects/mysql.py
index 750d4a36f3..c81baae583 100644
--- a/sqlglot/dialects/mysql.py
+++ b/sqlglot/dialects/mysql.py
@@ -319,11 +319,7 @@ class Parser(parser.Parser):
         FUNCTION_PARSERS = {
             **parser.Parser.FUNCTION_PARSERS,
             "CHAR": lambda self: self._parse_chr(),
-            "GROUP_CONCAT": lambda self: self.expression(
-                exp.GroupConcat,
-                this=self._parse_lambda(),
-                separator=self._match(TokenType.SEPARATOR) and self._parse_field(),
-            ),
+            "GROUP_CONCAT": lambda self: self._parse_group_concat(),
             # https://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_values
             "VALUES": lambda self: self.expression(
                 exp.Anonymous, this="VALUES", expressions=[self._parse_id_var()]
@@ -617,6 +613,32 @@ def _parse_chr(self) -> t.Optional[exp.Expression]:
 
             return self.expression(exp.Chr, **kwargs)
 
+        def _parse_group_concat(self) -> t.Optional[exp.Expression]:
+            def concat_exprs(node, exprs):
+                if node and isinstance(node, exp.Distinct) and len(node.expressions) > 1:
+                    concat_exprs = [self.expression(exp.Concat, expressions=node.expressions)]
+                    node.set("expressions", concat_exprs)
+                    return node
+                return (
+                    exprs[0] if len(exprs) == 1 else self.expression(exp.Concat, expressions=args)
+                )
+
+            args = self._parse_csv(self._parse_lambda)
+            order = args[-1] if isinstance(args[-1], exp.Order) else None
+
+            if order:
+                # Order By is the last (or only) expression in the list and has consumed the 'expr' before it,
+                # remove 'expr' from exp.Order and add it back to args
+                args.pop()
+                args.append(order.this)
+                order.set("this", concat_exprs(order.this, args))
+
+            this = order or concat_exprs(args[0], args)
+
+            separator = self._parse_field() if self._match(TokenType.SEPARATOR) else None
+
+            return self.expression(exp.GroupConcat, this=this, separator=separator)
+
     class Generator(generator.Generator):
         LOCKING_READS_SUPPORTED = True
         NULL_ORDERING_SUPPORTED = None
diff --git a/tests/dialects/test_mysql.py b/tests/dialects/test_mysql.py
index 5f23c4401a..6fb9fad964 100644
--- a/tests/dialects/test_mysql.py
+++ b/tests/dialects/test_mysql.py
@@ -723,6 +723,42 @@ def test_mysql(self):
                 "postgres": "STRING_AGG(DISTINCT x, '' ORDER BY y DESC NULLS LAST)",
             },
         )
+        self.validate_all(
+            "GROUP_CONCAT(a, b, c SEPARATOR '')",
+            write={
+                "mysql": "GROUP_CONCAT(CONCAT(a, b, c) SEPARATOR '')",
+                "sqlite": "GROUP_CONCAT(a || b || c, '')",
+                "tsql": "STRING_AGG(CONCAT(a, b, c), '')",
+                "postgres": "STRING_AGG(CONCAT(a, b, c), '')",
+            },
+        )
+        self.validate_all(
+            "GROUP_CONCAT(DISTINCT a, b, c SEPARATOR '')",
+            write={
+                "mysql": "GROUP_CONCAT(DISTINCT CONCAT(a, b, c) SEPARATOR '')",
+                "sqlite": "GROUP_CONCAT(DISTINCT a || b || c, '')",
+                "tsql": "STRING_AGG(CONCAT(a, b, c), '')",
+                "postgres": "STRING_AGG(DISTINCT CONCAT(a, b, c), '')",
+            },
+        )
+        self.validate_all(
+            "GROUP_CONCAT(a, b, c ORDER BY d SEPARATOR '')",
+            write={
+                "mysql": "GROUP_CONCAT(CONCAT(a, b, c) ORDER BY d SEPARATOR '')",
+                "sqlite": "GROUP_CONCAT(a || b || c, '')",
+                "tsql": "STRING_AGG(CONCAT(a, b, c), '') WITHIN GROUP (ORDER BY d)",
+                "postgres": "STRING_AGG(CONCAT(a, b, c), '' ORDER BY d NULLS FIRST)",
+            },
+        )
+        self.validate_all(
+            "GROUP_CONCAT(DISTINCT a, b, c ORDER BY d SEPARATOR '')",
+            write={
+                "mysql": "GROUP_CONCAT(DISTINCT CONCAT(a, b, c) ORDER BY d SEPARATOR '')",
+                "sqlite": "GROUP_CONCAT(DISTINCT a || b || c, '')",
+                "tsql": "STRING_AGG(CONCAT(a, b, c), '') WITHIN GROUP (ORDER BY d)",
+                "postgres": "STRING_AGG(DISTINCT CONCAT(a, b, c), '' ORDER BY d NULLS FIRST)",
+            },
+        )
         self.validate_identity(
             "CREATE TABLE z (a INT) ENGINE=InnoDB AUTO_INCREMENT=1 CHARACTER SET=utf8 COLLATE=utf8_bin COMMENT='x'"
         )


Code review comments:
diff_hunk: @@ -617,6 +613,32 @@ def _parse_chr(self) -> t.Optional[exp.Expression]:
 
             return self.expression(exp.Chr, **kwargs)
 
+        def _parse_group_concat(self) -> t.Optional[exp.Expression]:
+            def concat_exprs(node, exprs):
+                if node and isinstance(node, exp.Distinct) and len(node.expressions) > 1:
+                    concat_exprs = [self.expression(exp.Concat, expressions=node.expressions)]
+                    node.set("expressions", concat_exprs)
+                    return node
+                return (
+                    exprs[0] if len(exprs) == 1 else self.expression(exp.Concat, expressions=args)
+                )
+
+            args = self._parse_csv(self._parse_lambda)
+            order = args[-1] if isinstance(args[-1], exp.Order) else None
+
+            if order:
+                # Order By is the last (or only) expression in the list and has consumed the 'expr' before it,
+                # remove 'expr' from exp.Order and add it back to args
+                args.pop()
+                args.append(order.this)
path: sqlglot/dialects/mysql.py
text: @user1:
Maybe
```suggestion
                args[-1] = order.this
```

diff_hunk: @@ -617,6 +613,32 @@ def _parse_chr(self) -> t.Optional[exp.Expression]:
 
             return self.expression(exp.Chr, **kwargs)
 
+        def _parse_group_concat(self) -> t.Optional[exp.Expression]:
+            def concat_exprs(node, exprs):
+                if node and isinstance(node, exp.Distinct) and len(node.expressions) > 1:
+                    concat_exprs = [self.expression(exp.Concat, expressions=node.expressions)]
+                    node.set("expressions", concat_exprs)
+                    return node
+                return (
+                    exprs[0] if len(exprs) == 1 else self.expression(exp.Concat, expressions=args)
+                )
+
+            args = self._parse_csv(self._parse_lambda)
+            order = args[-1] if isinstance(args[-1], exp.Order) else None
+
+            if order:
+                # Order By is the last (or only) expression in the list and has consumed the 'expr' before it,
+                # remove 'expr' from exp.Order and add it back to args
+                args.pop()
+                args.append(order.this)
+                order.set("this", concat_exprs(order.this, args))
+
+            this = order or concat_exprs(args[0], args)
path: sqlglot/dialects/mysql.py
text: @user1:
Ditto about `seq_get(args, 0)` over the `args[0]` syntax

diff_hunk: @@ -617,6 +613,32 @@ def _parse_chr(self) -> t.Optional[exp.Expression]:
 
             return self.expression(exp.Chr, **kwargs)
 
+        def _parse_group_concat(self) -> t.Optional[exp.Expression]:
+            def concat_exprs(node, exprs):
+                if node and isinstance(node, exp.Distinct) and len(node.expressions) > 1:
+                    concat_exprs = [self.expression(exp.Concat, expressions=node.expressions)]
+                    node.set("expressions", concat_exprs)
+                    return node
+                return (
+                    exprs[0] if len(exprs) == 1 else self.expression(exp.Concat, expressions=args)
+                )
+
+            args = self._parse_csv(self._parse_lambda)
+            order = args[-1] if isinstance(args[-1], exp.Order) else None
+
+            if order:
path: sqlglot/dialects/mysql.py
text: @user1:
```suggestion
            if order and args:
```

diff_hunk: @@ -617,6 +613,32 @@ def _parse_chr(self) -> t.Optional[exp.Expression]:
 
             return self.expression(exp.Chr, **kwargs)
 
+        def _parse_group_concat(self) -> t.Optional[exp.Expression]:
+            def concat_exprs(node, exprs):
+                if node and isinstance(node, exp.Distinct) and len(node.expressions) > 1:
+                    concat_exprs = [self.expression(exp.Concat, expressions=node.expressions)]
+                    node.set("expressions", concat_exprs)
+                    return node
+                return (
+                    exprs[0] if len(exprs) == 1 else self.expression(exp.Concat, expressions=args)
+                )
+
+            args = self._parse_csv(self._parse_lambda)
+            order = args[-1] if isinstance(args[-1], exp.Order) else None
path: sqlglot/dialects/mysql.py
text: @user1:
Let's instead do:

```python
order = seq_get(args, -1)
if not isinstance(order, exp.Order):
    order = None
```

Since SQLGlot allows partial parsing (`error_level=ErrorLevel.IGNORE`), we don't want this to raise an index error if there are no args.

diff_hunk: @@ -617,6 +613,32 @@ def _parse_chr(self) -> t.Optional[exp.Expression]:
 
             return self.expression(exp.Chr, **kwargs)
 
+        def _parse_group_concat(self) -> t.Optional[exp.Expression]:
+            def concat_exprs(node, exprs):
+                if node and isinstance(node, exp.Distinct) and len(node.expressions) > 1:
path: sqlglot/dialects/mysql.py
text: @user1:
```suggestion
                if isinstance(node, exp.Distinct) and len(node.expressions) > 1:
```

@user1:
Let's add type hints to the closure.