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

planner: do not eliminate group_concat in aggregate elimination #9967

Merged
merged 5 commits into from Apr 1, 2019

Conversation

Projects
None yet
4 participants
@XuHuaiyu
Copy link
Contributor

commented Apr 1, 2019

What problem does this PR solve?

fix #9922

What is changed and how it works?

Do not eliminate group_concat considering system variable group_concat_max_len.

Actually, we can rewrite GROUP_CONCAT when all the arguments it
accepts are promised to be NOT-NULL. (Recorded as a task in #9968)
When it accepts only 1 argument, we can extract this argument into a
projection.
When it accepts multiple arguments, we can wrap the arguments with a
function CONCAT_WS and extract this function into a projection.
BUT, GROUP_CONCAT should truncate the final result according to the
system variable group_concat_max_len. To ensure the correctness of
the result, we close the elimination of GROUP_CONCAT here.

Check List

Tests

  • Integration test

Code changes

  • Has exported function/method change

Side effects

  • Possible performance regression
    Performance may regress when group_concat accepts only 1 argument.

Related changes

  • Need to cherry-pick to the release branch

@zz-jason zz-jason changed the title planner, executor: do not eliminate group_concat considering group_concat_max_len planner: do not eliminate group_concat in aggregate elimination Apr 1, 2019

@XuHuaiyu

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

PTAL @zz-jason
Issue created as #9968

@XuHuaiyu

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

/run-all-tests

@zz-jason
Copy link
Member

left a comment

LGTM

@zz-jason zz-jason added the status/LGT1 label Apr 1, 2019

@@ -36,6 +36,19 @@ type aggregationEliminateChecker struct {
// For count(expr), sum(expr), avg(expr), count(distinct expr, [expr...]) we may need to rewrite the expr. Details are shown below.
// If we can eliminate agg successful, we return a projection. Else we return a nil pointer.
func (a *aggregationEliminateChecker) tryToEliminateAggregation(agg *LogicalAggregation) *LogicalProjection {
for _, af := range agg.AggFuncs {
// TODO: Actually, we can rewrite GROUP_CONCAT.
// When it accepts only 1 argument, we can extract this argument into a

This comment has been minimized.

Copy link
@winoros

winoros Apr 1, 2019

Member

This case is easy to be added in this pr?

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Apr 1, 2019

Author Contributor

Noop, since group_concat_max_len is configurable.
We can not ensure whether the length of the final result exceeds max_len even if there is only 1 argument.

And, maybe it'll be easier to implement if we wrap the argument(s) into a concat_ws despite the count of the arguments.

XuHuaiyu added some commits Apr 1, 2019

@XuHuaiyu

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

/run-all-tests

@codecov

This comment has been minimized.

Copy link

commented Apr 1, 2019

Codecov Report

Merging #9967 into master will increase coverage by 0.0067%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master      #9967        +/-   ##
================================================
+ Coverage   77.4704%   77.4772%   +0.0067%     
================================================
  Files           403        404         +1     
  Lines         81777      81775         -2     
================================================
+ Hits          63353      63357         +4     
+ Misses        13707      13704         -3     
+ Partials       4717       4714         -3

@XuHuaiyu XuHuaiyu requested a review from winoros Apr 1, 2019

@eurekaka
Copy link
Contributor

left a comment

LGTM

@eurekaka eurekaka added status/LGT2 and removed status/LGT1 labels Apr 1, 2019

@zz-jason

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

/run-unit-test

@XuHuaiyu XuHuaiyu merged commit 4c91f53 into pingcap:master Apr 1, 2019

5 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/code_coverage Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@XuHuaiyu XuHuaiyu deleted the XuHuaiyu:agg_eliminate branch Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.