Skip to content
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: fix group_concat when chunk size is set to 1 #7328

Merged
merged 2 commits into from
Aug 13, 2018

Conversation

XuHuaiyu
Copy link
Contributor

@XuHuaiyu XuHuaiyu commented Aug 9, 2018

What have you changed? (mandatory)

Before this PR, the following sql would return wrong result.

create table idx_agg (a int, b int, index (b));
insert idx_agg values (1, 1), (1, 2), (2, 2);
set @@tidb_max_chunk_size=1;
tidb> select group_concat(b) from idx_agg group by b;
+-----------------+
| group_concat(b) |
+-----------------+
| 1              |
| 22            |
+-----------------+

The PR fix the problem:

tidb> select group_concat(b) from idx_agg group by b;
+-----------------+
| group_concat(b) |
+-----------------+
| 1             |
| 2,2         |
+-----------------+
2 rows in set (0.00 sec)

How does this bug occur?

This bug is caused by this line.
When max_chunk_size is set to 1, rowInGroup pass into the function
contains only 1 row every time. As we can see, separator be appended
here would be truncated out of the loop.

How does this bug be fixed?

This commit first check whether p.buffer != nil, which means we've
got part of the result for a group, if it's true, we'll append separator at
the end of p.buffer. Through this, we can avoid truncating separator
wrongly. Then we append the string be consist of the values. If
this string is null, we'll truncated the separator which is appended at
the first.

What is the type of the changes? (mandatory)

Bug-fix

How has this PR been tested? (mandatory)

unit test

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

no

Does this PR need to be added to the release notes? (mandatory)

no

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

@XuHuaiyu XuHuaiyu added type/bugfix This PR fixes a bug. sig/execution SIG execution labels Aug 9, 2018
@XuHuaiyu XuHuaiyu added this to the 2.1 milestone Aug 9, 2018
@winoros
Copy link
Member

winoros commented Aug 9, 2018

@XuHuaiyu The 800+ line change is all to fix the problem described in title?

@XuHuaiyu
Copy link
Contributor Author

XuHuaiyu commented Aug 9, 2018

@winoros Noop, most of the lines are from #7281 .
It would be nice if you can review it first.

The following functions fix and test the bug.
func_group_concat.go: func(e *groupConcat) UpdatePartialResult
func_avg_test.go: TestMergePartialResult4GroupConcat

@zz-jason
Copy link
Member

@XuHuaiyu can you remove the irrelevant changes so that we can be focused on the bugfix itself and merge this PR as soon as possible?

@XuHuaiyu
Copy link
Contributor Author

PTAL @zz-jason @winoros

Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@XuHuaiyu
Copy link
Contributor Author

PTAL @zz-jason

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@XuHuaiyu
Copy link
Contributor Author

/run-all-tests

@XuHuaiyu XuHuaiyu merged commit 1d567ef into pingcap:master Aug 13, 2018
@XuHuaiyu XuHuaiyu deleted the fix_group_concat branch August 13, 2018 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants