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

plan: fix a bug in decorrelation #6972

Merged
merged 10 commits into from Jul 5, 2018

Conversation

@winoros
Copy link
Member

commented Jul 3, 2018

What have you changed? (mandatory)

Previous, if the decorrelation failed. The equal condition which contains the correlated column is removed.
This pr is to fix it.

What are the type of the changes (mandatory)?

  • Bug fix (non-breaking change which fixes an issue)

How has this PR been tested (mandatory)?

unit-test

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

/run-all-tests

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

/run-all-tests

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

/run-all-tests tidb-test=pr/562

originalExpr := sel.Conditions
sel.Conditions = remainedExpr
// There's no other correlated column.
if apply.extractCorColumnsBySchema(); len(eqCondWithCorCol) > 0 && len(apply.corCols) == 0 {

This comment has been minimized.

Copy link
@zz-jason

zz-jason Jul 4, 2018

Member

better to move apply.extractCorColumnsBySchema() outside the if statement

This comment has been minimized.

Copy link
@winoros

winoros Jul 4, 2018

Author Member

So move to where?

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

/run-common-test tidb-test=pr/562

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

/run-common-test tidb-test=pr/562

agg.GroupByItems = append(agg.GroupByItems, clonedCol)
}
originalExpr := sel.Conditions
sel.Conditions = remainedExpr

This comment has been minimized.

Copy link
@coocood

coocood Jul 4, 2018

Member

Can we not modify sel.Conditions here?
If we check len(eqCondWithCorCol) == len(sel.Conditions), we can determine if selection can be removed, and remainedExpr is not needed.

This comment has been minimized.

Copy link
@winoros

winoros Jul 4, 2018

Author Member

There can be the case that the plan below this selection has correlated column. It's not normal case. But it still exists.

@winoros

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2018

/run-common-test tidb-test=pr/562

originalExpr := sel.Conditions
sel.Conditions = remainedExpr
// There's no other correlated column.
if apply.extractCorColumnsBySchema(); len(eqCondWithCorCol) > 0 && len(apply.corCols) == 0 {

This comment has been minimized.

Copy link
@coocood

coocood Jul 4, 2018

Member

Keep the original indent is easier to review.

@coocood

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

/run-all-tests

}
}
originalExpr := sel.Conditions

This comment has been minimized.

Copy link
@coocood

coocood Jul 4, 2018

Member

Can we move this two lines in if len(eqCondWithCorCol) > 0?

@coocood

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

LGTM

@winoros winoros added the status/LGT1 label Jul 4, 2018
Copy link
Member

left a comment

LGTM

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Jul 5, 2018
@zz-jason

This comment has been minimized.

Copy link
Member

commented Jul 5, 2018

/run-all-tests

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 5, 2018

/run-unit-test

@winoros winoros merged commit 3914266 into pingcap:master Jul 5, 2018
4 checks passed
4 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@winoros winoros deleted the winoros:decorrelate-bug branch Jul 5, 2018
zhexuany added a commit to zhexuany/tidb that referenced this pull request Jul 13, 2018
winoros added a commit to winoros/tidb that referenced this pull request Jul 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.