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: support memory trace for group_concat #20153

Merged
merged 18 commits into from
Oct 14, 2020

Conversation

iontang
Copy link
Contributor

@iontang iontang commented Sep 22, 2020

What problem does this PR solve?

Issue Number: close #19737

Problem Summary: Implement memDelta for group_concat function to trace the memory usage of AggFunc.

What is changed and how it works?

What's Changed: Implement memory track for group_concat AggFunc.

Check List

Tests

  • Unit test

Release note

  • Trace the memory usage of group_concat

@iontang iontang requested a review from a team as a code owner September 22, 2020 11:01
@iontang iontang requested review from qw4990 and removed request for a team September 22, 2020 11:01
@ti-challenge-bot
Copy link

There is no reward for this challenge pull request, so you can request a reward from @b41sh.

More

Tip : About reward you can refs to reward-command.

Warning: None

@ti-srebot ti-srebot added the contribution This PR is from a community contributor. label Sep 22, 2020
@ti-challenge-bot
Copy link

There is no reward for this challenge pull request, so you can request a reward from @b41sh.

More

Tip : About reward you can refs to reward-command.

Warning: None

1 similar comment
@ti-challenge-bot
Copy link

There is no reward for this challenge pull request, so you can request a reward from @b41sh.

More

Tip : About reward you can refs to reward-command.

Warning: None

@sre-bot
Copy link
Contributor

sre-bot commented Sep 22, 2020

@ti-challenge-bot
Copy link

There is no reward for this challenge pull request, so you can request a reward from @b41sh.

More

Tip : About reward you can refs to reward-command.

Warning: None

@CLAassistant
Copy link

CLAassistant commented Sep 22, 2020

CLA assistant check
All committers have signed the CLA.

@ti-challenge-bot
Copy link

There is no reward for this challenge pull request, so you can request a reward from @b41sh.

More

Tip : About reward you can refs to reward-command.

Warning: None

@ti-challenge-bot
Copy link

There is no reward for this challenge pull request, so you can request a reward from @b41sh.

More

Tip : About reward you can refs to reward-command.

Warning: None

@sre-bot
Copy link
Contributor

sre-bot commented Sep 22, 2020

@b41sh
Copy link
Member

b41sh commented Sep 22, 2020

/reward 600

@ti-challenge-bot
Copy link

Reward success.

@sre-bot
Copy link
Contributor

sre-bot commented Sep 22, 2020

@XuHuaiyu XuHuaiyu removed the request for review from qw4990 September 23, 2020 03:03
@XuHuaiyu XuHuaiyu added component/executor type/enhancement The issue or PR belongs to an enhancement. labels Sep 23, 2020
@XuHuaiyu
Copy link
Contributor

/cc @mmyj

@XuHuaiyu XuHuaiyu changed the title executor:support memory trace for group concat executor: support memory trace for group_concat Sep 23, 2020
@iontang
Copy link
Contributor Author

iontang commented Oct 11, 2020

@b41sh @mmyj PTAL~

@mmyj
Copy link
Member

mmyj commented Oct 11, 2020

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 11, 2020
}

func (e *groupConcatDistinctOrder) MergePartialResult(sctx sessionctx.Context, src, dst PartialResult) (memDelta int64, err error) {
// If order by exists, the parallel hash aggregation is forbidden in executorBuilder.buildHashAgg.
// So MergePartialResult will not be called.
return 0, terror.ClassOptimizer.New(mysql.ErrInternal, mysql.MySQLErrName[mysql.ErrInternal]).GenWithStack("groupConcatDistinctOrder.MergePartialResult should not be called")
}

// GetDatumMemSize will be called in func_group_concat_test.go
Copy link
Member

Choose a reason for hiding this comment

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

how about GetDatumMemSize is used to calculate memory size of types.Datum?

@iontang
Copy link
Contributor Author

iontang commented Oct 14, 2020

@b41sh PTAL~

@b41sh
Copy link
Member

b41sh commented Oct 14, 2020

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Oct 14, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Oct 14, 2020
@mmyj
Copy link
Member

mmyj commented Oct 14, 2020

PTAL @XuHuaiyu

@XuHuaiyu
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 14, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@william0423 merge failed.

@XuHuaiyu
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@william0423 merge failed.

@XuHuaiyu
Copy link
Contributor

/run-tics-test

@XuHuaiyu
Copy link
Contributor

/run-common-test
/run-tics-test

@XuHuaiyu XuHuaiyu merged commit 877fa63 into pingcap:master Oct 14, 2020
@ti-challenge-bot
Copy link

@william0423, Congratulations, you get 600 in this PR, and your total score is 600 in high-performance challenge program.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/executor contribution This PR is from a community contributor. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call For Participation: add memory trace for group concat agg functions
7 participants