parser, expression: support SEPARATOR in group_concat aggregate function

executor/aggregate_test.go

@@ -307,6 +307,33 @@ func (s *testSuite) TestAggregation(c *C) {
 	tk.MustQuery("select 11 from idx_agg group by a").Check(testkit.Rows("11", "11"))
 }
 
+func (s *testSuite) TestGroupConcatAggr(c *C) {
+	// issue #5411
+	tk := testkit.NewTestKit(c, s.store)
+	tk.MustExec("use test")
+	tk.MustExec("create table test(id int, name int)")
+	tk.MustExec("insert into test values(1, 10);")
+	tk.MustExec("insert into test values(1, 20);")
+	tk.MustExec("insert into test values(1, 30);")
+	tk.MustExec("insert into test values(2, 20);")
+	tk.MustExec("insert into test values(3, 200);")
+	tk.MustExec("insert into test values(3, 500);")
+	result := tk.MustQuery("select id, group_concat(name) from test group by id")
+	result.Check(testkit.Rows("1 10,20,30", "2 20", "3 200,500"))
+
+	result = tk.MustQuery("select id, group_concat(name SEPARATOR ';') from test group by id")
+	result.Check(testkit.Rows("1 10;20;30", "2 20", "3 200;500"))
+
+	result = tk.MustQuery("select id, group_concat(name SEPARATOR ',') from test group by id")
+	result.Check(testkit.Rows("1 10,20,30", "2 20", "3 200,500"))
+
+	result = tk.MustQuery("select id, group_concat(name SEPARATOR '%') from test group by id")
+	result.Check(testkit.Rows("1 10%20%30", "2 20", "3 200%500"))
+
+	result = tk.MustQuery("select id, group_concat(name SEPARATOR '') from test group by id")
+	result.Check(testkit.Rows("1 102030", "2 20", "3 200500"))
+}
+
 func (s *testSuite) TestAggPrune(c *C) {
 	tk := testkit.NewTestKit(c, s.store)
 	tk.MustExec("use test")

expression/aggregation/concat.go

@@ -25,6 +25,8 @@ import (
 
 type concatFunction struct {
 	aggFunction
+	separator string
+	sepInited bool
 }
 
 // Clone implements Aggregation interface.
@@ -49,9 +51,30 @@ func (cf *concatFunction) writeValue(ctx *AggEvaluateContext, val types.Datum) {
 	}
 }
 
+func (cf *concatFunction) initSeparator(sc *variable.StatementContext, row []types.Datum) error {
+	sepArg := cf.Args[len(cf.Args)-1]
+	sep, isNull, err := sepArg.EvalString(row, sc)
+	if err != nil {
+		return errors.Trace(err)
+	}
+	if isNull {
+		return errors.Errorf("Invalid separator argument.")
+	}
+	cf.separator = sep
+	cf.Args = cf.Args[:len(cf.Args)-1]
+	return nil
+}
+
 // Update implements Aggregation interface.
 func (cf *concatFunction) Update(ctx *AggEvaluateContext, sc *variable.StatementContext, row []types.Datum) error {
 	datumBuf := make([]types.Datum, 0, len(cf.Args))
+	if !cf.sepInited {
+		err := cf.initSeparator(sc, row)
+		if err != nil {
+			return errors.Trace(err)
+		}
+		cf.sepInited = true
+	}
 	for _, a := range cf.Args {
 		value, err := a.Eval(row)
 		if err != nil {
@@ -74,8 +97,7 @@ func (cf *concatFunction) Update(ctx *AggEvaluateContext, sc *variable.Statement
 	if ctx.Buffer == nil {
 		ctx.Buffer = &bytes.Buffer{}
 	} else {
-		// now use comma separator
-		ctx.Buffer.WriteString(",")
+		ctx.Buffer.WriteString(cf.separator)
 	}
 	for _, val := range datumBuf {
 		cf.writeValue(ctx, val)

parser/misc.go

@@ -388,6 +388,7 @@ var tokenMap = map[string]int{
 	"SERIALIZABLE":        serializable,
 	"SESSION":             session,
 	"SET":                 set,
+	"SEPARATOR":           separator,
 	"SHARE":               share,
 	"SHARED":              shared,
 	"SHOW":                show,

parser/parser.y

@@ -327,6 +327,7 @@ import (
 	rowCount	"ROW_COUNT"
 	rowFormat	"ROW_FORMAT"
 	second		"SECOND"
+	separator 	"SEPARATOR"
 	serializable	"SERIALIZABLE"
 	session		"SESSION"
 	share		"SHARE"
@@ -600,6 +601,7 @@ import (
 	ReferDef			"Reference definition"
 	OnDeleteOpt			"optional ON DELETE clause"
 	OnUpdateOpt			"optional ON UPDATE clause"
+	OptGConcatSeparator		"optional GROUP_CONCAT SEPARATOR"
 	ReferOpt			"reference option"
 	ReplacePriority			"replace statement priority"
 	RowFormat			"Row format option"
@@ -2293,7 +2295,7 @@ UnReservedKeyword:
 | "REPEATABLE" | "COMMITTED" | "UNCOMMITTED" | "ONLY" | "SERIALIZABLE" | "LEVEL" | "VARIABLES" | "SQL_CACHE" | "INDEXES" | "PROCESSLIST"
 | "SQL_NO_CACHE" | "DISABLE"  | "ENABLE" | "REVERSE" | "PRIVILEGES" | "NO" | "BINLOG" | "FUNCTION" | "VIEW" | "MODIFY" | "EVENTS" | "PARTITIONS"
 | "NONE" | "SUPER" | "EXCLUSIVE" | "STATS_PERSISTENT" | "ROW_COUNT" | "COALESCE" | "MONTH" | "PROCESS"
-| "MICROSECOND" | "MINUTE" | "PLUGINS" | "QUERY" | "SECOND" | "SHARE" | "SHARED"
+| "MICROSECOND" | "MINUTE" | "PLUGINS" | "QUERY" | "SECOND" | "SEPARATOR" | "SHARE" | "SHARED"
 
 TiDBKeyword:
 "ADMIN" | "CANCEL" | "DDL" | "JOBS" | "STATS" | "STATS_META" | "STATS_HISTOGRAMS" | "STATS_BUCKETS" | "TIDB" | "TIDB_SMJ" | "TIDB_INLJ"
@@ -3192,9 +3194,11 @@ SumExpr:
 		args := []ast.ExprNode{ast.NewValueExpr(1)}
 		$$ = &ast.AggregateFuncExpr{F: $1, Args: args}
 	}
-|	"GROUP_CONCAT" '(' BuggyDefaultFalseDistinctOpt ExpressionList ')'
+|	"GROUP_CONCAT" '(' BuggyDefaultFalseDistinctOpt ExpressionList OptGConcatSeparator')'
 	{
-		$$ = &ast.AggregateFuncExpr{F: $1, Args: $4.([]ast.ExprNode), Distinct: $3.(bool)}
+		args := $4.([]ast.ExprNode)
+		args = append(args, $5.(ast.ExprNode))
+		$$ = &ast.AggregateFuncExpr{F: $1, Args: args, Distinct: $3.(bool)}
 	}
 |	"MAX" '(' BuggyDefaultFalseDistinctOpt Expression ')'
 	{
@@ -3209,6 +3213,15 @@ SumExpr:
 		$$ = &ast.AggregateFuncExpr{F: $1, Args: []ast.ExprNode{$4}, Distinct: $3.(bool)}
 	}
 
+OptGConcatSeparator:
+        {
+            	$$ = ast.NewValueExpr(",")
+        }
+| "SEPARATOR" stringLit
+	{ 
+		$$ = ast.NewValueExpr($2)
+	}
+
 FunctionCallGeneric:
 	identifier '(' ExpressionListOpt ')'
 	{

parser/parser_test.go

@@ -1138,6 +1138,7 @@ func (s *testParserSuite) TestBuiltin(c *C) {
 		{`select count(distinct all c1) from t;`, false},
 		{`select count(distinctrow all c1) from t;`, false},
 		{`select group_concat(c2,c1) from t group by c1;`, true},
+		{`select group_concat(c2,c1 SEPARATOR ';') from t group by c1;`, true},
 		{`select group_concat(distinct c2,c1) from t group by c1;`, true},
 		{`select group_concat(distinctrow c2,c1) from t group by c1;`, true},