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.

Sign up for GitHub

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

Jump to bottom

executor: reset groupChecker for StreamAggExec when Close #10615

Merged
merged 3 commits into from
May 29, 2019
Merged

executor: reset groupChecker for StreamAggExec when Close #10615

merged 3 commits into from
May 29, 2019

Conversation

XuHuaiyu
Copy link
Contributor

What problem does this PR solve?

Fix #10608

What is changed and how it works?

Reset StreamAggExec.groupChecker when Close.

Check List

Tests

  • Integration test

Code changes

  • Has exported function/method change

Side effects
N/A

Related changes

  • Need to cherry-pick to the release branch

@XuHuaiyu XuHuaiyu added type/bugfix This PR fixes a bug. sig/execution SIG execution labels May 28, 2019
@XuHuaiyu XuHuaiyu requested review from qw4990, winoros and eurekaka May 28, 2019 03:50
@XuHuaiyu
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented May 28, 2019 •
edited
Loading

Codecov Report

Merging #10615 into master will increase coverage by 0.0152%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #10615        +/-   ##
================================================
+ Coverage   77.6763%   77.6916%   +0.0152%     
================================================
  Files           413        413                
  Lines         87553      87559         +6     
================================================
+ Hits          68008      68026        +18     
+ Misses        14387      14381         -6     
+ Partials       5158       5152         -6

alivxxx
alivxxx reviewed May 28, 2019
View reviewed changes
Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@alivxxx alivxxx added the status/LGT1 Indicates that a PR has LGTM 1. label May 28, 2019
winoros
winoros approved these changes May 28, 2019
View reviewed changes
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

@qw4990 qw4990 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 29, 2019
@XuHuaiyu XuHuaiyu merged commit 0324cf5 into pingcap:master May 29, 2019
XuHuaiyu added a commit to XuHuaiyu/tidb that referenced this pull request May 29, 2019
@XuHuaiyu
executor: reset groupChecker for StreamAggExec when Close (pingcap#10615
8c6307a 
)
@XuHuaiyu XuHuaiyu mentioned this pull request May 29, 2019
Merged
zz-jason pushed a commit that referenced this pull request May 30, 2019
@XuHuaiyu @zz-jason
executor: reset groupChecker for StreamAggExec when Close (#10615) (#…
52dde3b 
…10636)
@XuHuaiyu XuHuaiyu deleted the fix_streamagg branch May 31, 2019 02:22
XuHuaiyu added a commit to XuHuaiyu/tidb that referenced this pull request May 31, 2019
@XuHuaiyu
executor: reset groupChecker for StreamAggExec when Close (pingcap#10615
788754a 
)
@XuHuaiyu XuHuaiyu mentioned this pull request May 31, 2019
Merged
zz-jason pushed a commit that referenced this pull request May 31, 2019
@XuHuaiyu @zz-jason
executor: reset groupChecker for StreamAggExec when Close (#10615) (#…
babf93f 
…10670)
cosven
cosven reviewed Oct 23, 2019
View reviewed changes
executor/aggregate_test.go
func (s *testSuite1) TestIssue10608(c *C) {
tk := testkit.NewTestKitWithInit(c, s.store)
tk.MustExec(`drop table if exists t, s;`)
tk.MustExec("create table t(a int)")
Copy link
Contributor

Choose a reason for hiding this comment

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

this test case currently does not cover the bug since it can't reproduce the bug.

we should modify this line to create table t(a int primary key);

Copy link
Contributor

Choose a reason for hiding this comment

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

@XuHuaiyu

Copy link
Contributor Author

@XuHuaiyu XuHuaiyu Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What do you mean by it can't reproduce the bug?
@cosven

Copy link
Contributor

@cosven cosven Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

what I expect
this test case should fail in previous commits(version), such as 3.0.0-beta
it should return error 1105 - subquery returns more than 1 row

what I saw instead
it passed.

What do you mean by it can't reproduce the bug?

this test case can't prove this PR fixes the bug described in issue #10608, because this unittest case will always pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alivxxx alivxxx alivxxx left review comments

@cosven cosven cosven left review comments

@winoros winoros winoros approved these changes

@qw4990 qw4990 Awaiting requested review from qw4990

@eurekaka eurekaka Awaiting requested review from eurekaka

Assignees
No one assigned
Labels
sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GROUP_CONCAT error. subquery returns more than 1 row
6 participants
@XuHuaiyu @cosven @winoros @alivxxx @qw4990 @ngaut