From 7526894c8690600859b0595276bf4c9ad7c43f63 Mon Sep 17 00:00:00 2001
From: guo-shaoge <shaoge1994@163.com>
Date: Thu, 11 Mar 2021 15:30:55 +0800
Subject: [PATCH] cherry pick #23131 to release-4.0

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
---
 executor/aggfuncs/func_group_concat.go | 31 +++++++++++++++++---------
 executor/aggregate_test.go             | 17 ++++++++++++++
 2 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/executor/aggfuncs/func_group_concat.go b/executor/aggfuncs/func_group_concat.go
index d13bc754b0842..c2d3b745d12fc 100644
--- a/executor/aggfuncs/func_group_concat.go
+++ b/executor/aggfuncs/func_group_concat.go
@@ -236,6 +236,11 @@ type topNRows struct {
 	currSize  uint64
 	limitSize uint64
 	sepSize   uint64
+	// If sep is truncated, we need to append part of sep to result.
+	// In the following example, session.group_concat_max_len is 10 and sep is '---'.
+	// ('---', 'ccc') should be poped from heap, so '-' should be appended to result.
+	// eg: 'aaa---bbb---ccc' -> 'aaa---bbb-'
+	isSepTruncated bool
 }
 
 func (h topNRows) Len() int {
@@ -296,6 +301,7 @@ func (h *topNRows) tryToAdd(row sortRow) (truncated bool) {
 		} else {
 			h.currSize -= uint64(h.rows[0].buffer.Len()) + h.sepSize
 			heap.Pop(h)
+			h.isSepTruncated = true
 		}
 	}
 	return true
@@ -316,10 +322,11 @@ func (h *topNRows) concat(sep string, truncated bool) string {
 		}
 		buffer.Write(row.buffer.Bytes())
 	}
-	if truncated && uint64(buffer.Len()) < h.limitSize {
-		// append the last separator, because the last separator may be truncated in tryToAdd.
+	if h.isSepTruncated {
 		buffer.WriteString(sep)
-		buffer.Truncate(int(h.limitSize))
+		if uint64(buffer.Len()) > h.limitSize {
+			buffer.Truncate(int(h.limitSize))
+		}
 	}
 	return buffer.String()
 }
@@ -349,10 +356,11 @@ func (e *groupConcatOrder) AllocPartialResult() PartialResult {
 	}
 	p := &partialResult4GroupConcatOrder{
 		topN: &topNRows{
-			desc:      desc,
-			currSize:  0,
-			limitSize: e.maxLen,
-			sepSize:   uint64(len(e.sep)),
+			desc:           desc,
+			currSize:       0,
+			limitSize:      e.maxLen,
+			sepSize:        uint64(len(e.sep)),
+			isSepTruncated: false,
 		},
 	}
 	return PartialResult(p)
@@ -449,10 +457,11 @@ func (e *groupConcatDistinctOrder) AllocPartialResult() PartialResult {
 	}
 	p := &partialResult4GroupConcatOrderDistinct{
 		topN: &topNRows{
-			desc:      desc,
-			currSize:  0,
-			limitSize: e.maxLen,
-			sepSize:   uint64(len(e.sep)),
+			desc:           desc,
+			currSize:       0,
+			limitSize:      e.maxLen,
+			sepSize:        uint64(len(e.sep)),
+			isSepTruncated: false,
 		},
 		valSet: set.NewStringSet(),
 	}
diff --git a/executor/aggregate_test.go b/executor/aggregate_test.go
index ad48bc68ae314..842688795e44b 100644
--- a/executor/aggregate_test.go
+++ b/executor/aggregate_test.go
@@ -630,6 +630,23 @@ func (s *testSuiteAgg) TestGroupConcatAggr(c *C) {
 
 	// issue #9920
 	tk.MustQuery("select group_concat(123, null)").Check(testkit.Rows("<nil>"))
+
+	// issue #23129
+	tk.MustExec("drop table if exists t1;")
+	tk.MustExec("create table t1(cid int, sname varchar(100));")
+	tk.MustExec("insert into t1 values(1, 'Bob'), (1, 'Alice');")
+	tk.MustExec("insert into t1 values(3, 'Ace');")
+	tk.MustExec("set @@group_concat_max_len=5;")
+	rows := tk.MustQuery("select group_concat(sname order by sname) from t1 group by cid;")
+	rows.Check(testkit.Rows("Alice", "Ace"))
+
+	tk.MustExec("drop table if exists t1;")
+	tk.MustExec("create table t1(c1 varchar(10));")
+	tk.MustExec("insert into t1 values('0123456789');")
+	tk.MustExec("insert into t1 values('12345');")
+	tk.MustExec("set @@group_concat_max_len=8;")
+	rows = tk.MustQuery("select group_concat(c1 order by c1) from t1 group by c1;")
+	rows.Check(testkit.Rows("01234567", "12345"))
 }
 
 func (s *testSuiteAgg) TestSelectDistinct(c *C) {