-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
executor: group_concat aggr panic when session.group_concat_max_len is small #23131
executor: group_concat aggr panic when session.group_concat_max_len is small #23131
Conversation
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Another compatible bug found(Same case as this PR.) mysql 8.0:
TiDB master:
The result of TiDB has an extra comma.Not critical, will fix in another issue. Actually, we should add "truncated" flag for each group instead of use one flag for all groups. For each group, if got truncated flag, we add the last separator. Otherwise, donot add. |
Have you tested the case in #23129? |
Yes. The case is OK. |
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
9b8be35
to
48463a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refine the release note to describe what you fixed in this commit, not the phenomenon of the bug.
@@ -280,6 +280,10 @@ type topNRows struct { | |||
currSize uint64 | |||
limitSize uint64 | |||
sepSize uint64 | |||
// If (separator, row) pair is poped from heap, we need to append separator in concat() | |||
// In the following example, (sep2, str3) pair is poped, but partial sep2 need to be appended. | |||
// eg: "str1 sep1 str2 sep2 str3" -> "str1 sep1 str2 se" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- sep1 --> sep
- sep2 --> sep
- what's the group_concat_length of this example
@@ -280,6 +280,10 @@ type topNRows struct { | |||
currSize uint64 | |||
limitSize uint64 | |||
sepSize uint64 | |||
// If (separator, row) pair is poped from heap, we need to append separator in concat() | |||
// In the following example, (sep2, str3) pair is poped, but partial sep2 need to be appended. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(sep2, str3) --> (sep, str3)
partial sep2 --> part of the second sep
Signed-off-by: guo-shaoge <shaoge1994@163.com>
41eef30
to
b10f031
Compare
@lzmhhh123 @XuHuaiyu PTAL |
/LGTM |
/lgtm |
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by writing |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: b10f031
|
/rebuild |
/run-tics-test |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
cherry pick to release-4.0 in PR #23257 |
Signed-off-by: guo-shaoge shaoge1994@163.com
What problem does this PR solve?
Issue Number: close #23129
Problem Summary:
What is changed and how it works?
What's Changed: when concat all strings from a group(AppendFinalResult2Chunk), we add the last separator to result, and then truncate to sesssion.group_concat_max_len. But we forgot to check if the concated string is shorter than sesssion.group_concat_max_len. So program panic.
PS: Another compatible bug found when fixing this bug, but it's not critical. Will fix in another issue.
Related changes
Check List
Tests
Side effects: no
Release note