-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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: remove redundant branches in the OR list | tidb-test=pr/2470 #58962
Changes from all commits
3ac93d0
c0f9f08
1917ed0
0d626be
100e9c7
f9c2a04
2e3d1b7
d3f6f32
9bb337d
126458a
fc9e6ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
pruneEmptyORBranches(sctx, simplifiedPredicate) | ||
simplifiedPredicate = splitCNF(simplifiedPredicate) | ||
return simplifiedPredicate | ||
|
@@ -424,6 +425,51 @@ func shortCircuitLogicalConstants(sctx base.PlanContext, predicates []expression | |
return finalResult | ||
} | ||
|
||
// removeRedundantORBranch recursively iterates over a list of predicates, try to find OR lists and remove redundant in | ||
// each OR list. | ||
// It modifies the input slice in place. | ||
func removeRedundantORBranch(sctx base.PlanContext, predicates []expression.Expression) { | ||
for i, predicate := range predicates { | ||
predicates[i] = recursiveRemoveRedundantORBranch(sctx, predicate) | ||
} | ||
} | ||
|
||
func recursiveRemoveRedundantORBranch(sctx base.PlanContext, predicate expression.Expression) expression.Expression { | ||
_, tp := FindPredicateType(sctx, predicate) | ||
if tp != orPredicate { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return predicate | ||
} | ||
orFunc := predicate.(*expression.ScalarFunction) | ||
orList := expression.SplitDNFItems(orFunc) | ||
|
||
dedupMap := make(map[string]struct{}, len(orList)) | ||
newORList := make([]expression.Expression, 0, len(orList)) | ||
|
||
for _, orItem := range orList { | ||
_, tp := FindPredicateType(sctx, orItem) | ||
// 1. If it's an AND predicate, we recursively call removeRedundantORBranch() on it. | ||
if tp == andPredicate { | ||
andFunc := orItem.(*expression.ScalarFunction) | ||
andList := expression.SplitCNFItems(andFunc) | ||
removeRedundantORBranch(sctx, andList) | ||
newORList = append(newORList, expression.ComposeCNFCondition(sctx.GetExprCtx(), andList...)) | ||
} else { | ||
// 2. Otherwise, we check if it's a duplicate predicate by checking HashCode(). | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically, yes. But I think using |
||
newORList = append(newORList, orItem) | ||
} else if expression.IsMutableEffectsExpr(orItem) { | ||
// 2-2. If it's a duplicate, but it's nondeterministic or has side effects, we also need to keep it. | ||
newORList = append(newORList, orItem) | ||
} | ||
// 2-3. Otherwise, we remove it. | ||
} | ||
} | ||
return expression.ComposeDNFCondition(sctx.GetExprCtx(), newORList...) | ||
} | ||
|
||
// Name implements base.LogicalOptRule.<1st> interface. | ||
func (*PredicateSimplification) Name() string { | ||
return "predicate_simplification" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason of the order of the sub-rule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.