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: refine HashAgg.Close when unparallelExec #8810

Merged
merged 10 commits into from
Dec 27, 2018

Conversation

XuHuaiyu
Copy link
Contributor

@XuHuaiyu XuHuaiyu commented Dec 25, 2018

What problem does this PR solve?

fix #8787

What is changed and how it works?

Close the child executor when HashAgg executes unparalleled.

Check List

Tests

exists leak tests

Code changes

  • Has exported function/method change

Side effects

N/A

Related changes

  • Need to cherry-pick to the release branch

release-2.1


This change is Reviewable

@XuHuaiyu XuHuaiyu added type/bugfix This PR fixes a bug. sig/execution SIG execution labels Dec 25, 2018
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

LGTM

@eurekaka
Copy link
Contributor

/run-unit-tests

@XuHuaiyu
Copy link
Contributor Author

/run-all-tests

@eurekaka eurekaka added status/LGT1 Indicates that a PR has LGTM 1. status/all tests passed labels Dec 25, 2018
zz-jason
zz-jason previously approved these changes Dec 25, 2018
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

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 25, 2018
@zz-jason
Copy link
Member

@XuHuaiyu Could you add a gofail test?

@shenli
Copy link
Member

shenli commented Dec 26, 2018

Please add a test case.

@crazycs520
Copy link
Contributor

/run-unit-test

1 similar comment
@eurekaka
Copy link
Contributor

/run-unit-test

@XuHuaiyu
Copy link
Contributor Author

PTAL @zz-jason gofail test added

tk.MustExec(`drop table if exists t;`)
tk.MustExec("create table t(a int, b int)")
tk.MustExec("insert into t values(1,1),(2,2)")
result := tk.MustQuery("desc select sum(a) from (select cast(t.a as signed) as a, b from t) t group by b;")
Copy link
Member

Choose a reason for hiding this comment

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

A simple case to use hash aggregate is

select sum(a) from t group by b;

Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this test case? If the planner hasn't chosen the Hash Aggregate operator, the following gofail test would not passed I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. The case can also check whether the child of HashAgg is closed correctly, so I make a Projection there.
  2. Yep, the explain result is not really needed, I'll make it as a comment.

rs := rss[0]
chk := rs.NewChunk()
err = rs.Next(ctx, chk)
c.Assert(err, NotNil)
Copy link
Member

Choose a reason for hiding this comment

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

It's better to check the error message

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 XuHuaiyu merged commit 7108881 into pingcap:master Dec 27, 2018
@XuHuaiyu XuHuaiyu deleted the hashagg_close branch December 27, 2018 09:30
AndrewDi pushed a commit to AndrewDi/tidb that referenced this pull request Dec 28, 2018
yu34po pushed a commit to yu34po/tidb that referenced this pull request Jan 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

potential goroutine leak on parallel hash aggregate
7 participants