planner: remove redundant branches in the OR list | tidb-test=pr/2470 #58962
Conversation
…ant-values-from-OR-list
…ant-values-from-OR-list
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
size/L
time-and-fate
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #58962 +/- ##
================================================
+ Coverage 73.0164% 73.4821% +0.4657%
================================================
Files 1685 1684 -1
Lines 465917 465785 -132
================================================
+ Hits 340196 342269 +2073
+ Misses 104790 102578 -2212
- Partials 20931 20938 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
/retest |
| hashCode := string(orItem.HashCode()) | ||
| // 2-1. If it's not a duplicate, we need to keep this predicate. | ||
| if _, ok := dedupMap[hashCode]; !ok { | ||
| dedupMap[hashCode] = struct{}{} |
There was a problem hiding this comment.
Minor: the value of the map can be anything and may be just a simple constant like True.
There was a problem hiding this comment.
Technically, yes. But I think using map[T]struct{} as a set has become a common practice in Golang.
https://stackoverflow.com/a/47544821
|
|
||
| func recursiveRemoveRedundantORBranch(sctx base.PlanContext, predicate expression.Expression) expression.Expression { | ||
| _, tp := FindPredicateType(sctx, predicate) | ||
| if tp != orPredicate { |
There was a problem hiding this comment.
This is a recursive function (indirectly through removeRedundantORBranch). If you intend to cover the And case (like the code in the function) then you just need a general logic for OR AND lists.
There was a problem hiding this comment.
I didn't intend to cover the AND case (since that's already covered by other existing logic), I want to handle OR lists nested in another AND list.
|
|
||
| drop table if exists t4; | ||
| create table t4(a int, b int, c int, d int, index iab(a,b), index iac(a,c), index iad(a,d)); | ||
| explain format=brief select /*+ use_index_merge(t4) */ * from t4 where a = 1 and (b = 2 or c = 4 or b = 12 or c = 5 or d = 6 or c = 4 or c = 5 or d = 6); |
There was a problem hiding this comment.
Can you add more nested AND/OR cases since the code does that?
There was a problem hiding this comment.
Updated.
I added several more cases in t1 since the logic of building index ranges has some extra functionality to simplify the ranges. So I think printing the expression in Selection can test the effect of this new rule more straightforwardly.
The t4 case is a simplified version of the original case from the user.
| @@ -186,6 +186,7 @@ func splitCNF(conditions []expression.Expression) []expression.Expression { | |||
| func applyPredicateSimplification(sctx base.PlanContext, predicates []expression.Expression) []expression.Expression { | |||
| simplifiedPredicate := shortCircuitLogicalConstants(sctx, predicates) | |||
| simplifiedPredicate = mergeInAndNotEQLists(sctx, simplifiedPredicate) | |||
| removeRedundantORBranch(sctx, simplifiedPredicate) | |||
There was a problem hiding this comment.
Any reason of the order of the sub-rule?
There was a problem hiding this comment.
There are no strong reasons. Actually, after I put this new sub-rule in a different order, the new test cases don't have any changes.
I have several minor considerations though:
- I try not to change the order of existing sub-rules.
mergeInAndNotEQLists()simplifies the predicates by merging some of them, i.e., constructing some new expressions. Probably there will be new redundant expressions after that sub-rule, so put the new sub-rule after it might be a good idea.pruneEmptyORBranches()just removes useless OR branches, and should not produce new redundant expressions. So probably it's useless to put the new sub-rule after it.
[LGTM Timeline notifier]Timeline:
|
…ant-values-from-OR-list
|
/retest |
What problem does this PR solve?
Issue Number: close #58998
What changed and how does it work?
In the rule
predicate_simplification, add a new step to remove redundant (duplicate) expressions from the OR list. It's more like the existingRemoveDupExprs()thanPropagateConstant()for AND lists.Specifically, it uses
HashCode()to check the redundancy, like we did inRemoveDupExprs()andgenerateANDIndexMerge4NormalIndex(). And it recursively does this for nested AND lists.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.