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: fix a corner case in column pruning rule #10974

Merged
merged 3 commits into from Jul 1, 2019

Conversation

Projects
None yet
3 participants
@bb7133
Copy link
Contributor

commented Jun 28, 2019

What problem does this PR solve?

This PR tries to close #9125

What is changed and how it works?

For a LogicalAggregation, if all of its functions are pruned and it doesn't have group-by items, a dummy aggregate function is added to keep the correctness.

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Possible performance regression

Related changes

  • Need to cherry-pick to the release branch
@codecov

This comment has been minimized.

Copy link

commented Jun 28, 2019

Codecov Report

Merging #10974 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #10974   +/-   ##
===========================================
  Coverage   81.0119%   81.0119%           
===========================================
  Files           418        418           
  Lines         89335      89335           
===========================================
  Hits          72372      72372           
  Misses        11731      11731           
  Partials       5232       5232
return err
}
la.AggFuncs = []*aggregation.AggFuncDesc{one}
la.schema.Columns = []*expression.Column{{TblName: model.NewCIStr("dummy_cnt"), RetType: types.NewFieldType(mysql.TypeLonglong)}}

This comment has been minimized.

Copy link
@eurekaka

eurekaka Jun 28, 2019

Contributor

Fill ColName and UniqueID as well?

This comment has been minimized.

Copy link
@bb7133

bb7133 Jun 30, 2019

Author Contributor

Done, thanks

if len(la.GroupByItems) == 0 && len(la.AggFuncs) == 0 {
// If all the aggregate functions are pruned and there is no group-by item, we should add
// an aggregate function to keep the correctness.
one, err := aggregation.NewAggFuncDesc(la.ctx, ast.AggFuncCount, []expression.Expression{expression.One}, false)

This comment has been minimized.

Copy link
@eurekaka

eurekaka Jun 28, 2019

Contributor

I prefer using first_row(1) here.

This comment has been minimized.

Copy link
@bb7133

bb7133 Jun 30, 2019

Author Contributor

You're right... first_row is better

@@ -117,6 +119,17 @@ func (la *LogicalAggregation) PruneColumns(parentUsedCols []*expression.Column)
for _, aggrFunc := range la.AggFuncs {
selfUsedCols = expression.ExtractColumnsFromExpressions(selfUsedCols, aggrFunc.Args, nil)
}
if len(la.GroupByItems) == 0 && len(la.AggFuncs) == 0 {

This comment has been minimized.

Copy link
@eurekaka

eurekaka Jun 28, 2019

Contributor

If len(la.GroupByItems) > 0 && len(la.AggFuncs) == 0, we'd better add this dummy aggregation function as well. Though the result is correct now, we are depending on the specific implementation of executor, if executor is changed in the future, we may have problem for this case then.

This comment has been minimized.

Copy link
@bb7133

bb7133 Jun 30, 2019

Author Contributor

Thanks for the explanation

@bb7133 bb7133 force-pushed the bb7133:bb7133/fix_column_prune branch from e9cb460 to 3259bac Jun 30, 2019

@bb7133 bb7133 force-pushed the bb7133:bb7133/fix_column_prune branch from 3259bac to 3fd835b Jun 30, 2019

@bb7133

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

addressed, thank you @eurekaka

@eurekaka
Copy link
Contributor

left a comment

LGTM

@eurekaka eurekaka added the status/LGT1 label Jul 1, 2019

@eurekaka eurekaka requested review from lamxTyler, zz-jason and winoros Jul 1, 2019

@zz-jason
Copy link
Member

left a comment

LGTM

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Jul 1, 2019

@zz-jason

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

/run-all-tests

@zz-jason zz-jason merged commit 3847ec8 into pingcap:master Jul 1, 2019

14 checks passed

ci/circleci Your tests passed on CircleCI!
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@zz-jason

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

needs to be cherry-picked to release-2.1 and release-3.0

@bb7133

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

needs to be cherry-picked to release-2.1 and release-3.0

OKay

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.