plan: fix a bug in decorrelation #6972
Conversation
|
/run-all-tests |
|
/run-all-tests |
|
/run-all-tests tidb-test=pr/562 |
plan/rule_decorrelate.go
Outdated
| originalExpr := sel.Conditions | ||
| sel.Conditions = remainedExpr | ||
| // There's no other correlated column. | ||
| if apply.extractCorColumnsBySchema(); len(eqCondWithCorCol) > 0 && len(apply.corCols) == 0 { |
There was a problem hiding this comment.
better to move apply.extractCorColumnsBySchema() outside the if statement
|
/run-common-test tidb-test=pr/562 |
|
/run-common-test tidb-test=pr/562 |
plan/rule_decorrelate.go
Outdated
| agg.GroupByItems = append(agg.GroupByItems, clonedCol) | ||
| } | ||
| originalExpr := sel.Conditions | ||
| sel.Conditions = remainedExpr |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There can be the case that the plan below this selection has correlated column. It's not normal case. But it still exists.
|
/run-common-test tidb-test=pr/562 |
plan/rule_decorrelate.go
Outdated
| originalExpr := sel.Conditions | ||
| sel.Conditions = remainedExpr | ||
| // There's no other correlated column. | ||
| if apply.extractCorColumnsBySchema(); len(eqCondWithCorCol) > 0 && len(apply.corCols) == 0 { |
There was a problem hiding this comment.
Keep the original indent is easier to review.
|
/run-all-tests |
plan/rule_decorrelate.go
Outdated
| } | ||
| } | ||
| originalExpr := sel.Conditions |
There was a problem hiding this comment.
Can we move this two lines in if len(eqCondWithCorCol) > 0?
|
LGTM |
zz-jason
added
status/LGT2
|
/run-all-tests |
|
/run-unit-test |
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)?
How has this PR been tested (mandatory)?
unit-test