planner: simplify null-safe equality predicates#68589
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
release-note
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis PR implements simplification of null-safe equality predicates by rewriting ChangesNull-Safe Equality Simplification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/planner/core/rule/rule_predicate_simplification.go`:
- Around line 567-572: When handling ast.LogicOr in the switch, the current
early return when buildNullSafeEqualFromOR(sctx, sf) returns rewritten==false
skips simplifying OR children; instead, if rewritten is false you must descend
into sf's child predicates, recursively simplify each child (using the same
predicate-simplification routine that operates on sf.Args/children) and
rebuild/return the OR node with simplified children; only return early when a
null-safe rewrite was applied (rewritten==true) and you replace the whole node.
Ensure you reference and update sf (the ScalarFunc/OR node) and call the
existing per-predicate simplifier rather than returning predicate unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 14643399-6d9d-424f-a672-77ed638b019c
📒 Files selected for processing (4)
pkg/planner/core/casetest/rule/testdata/predicate_simplification_in.jsonpkg/planner/core/casetest/rule/testdata/predicate_simplification_out.jsonpkg/planner/core/casetest/rule/testdata/predicate_simplification_xut.jsonpkg/planner/core/rule/rule_predicate_simplification.go
| switch sf.FuncName.L { | ||
| case ast.LogicOr: | ||
| nullSafeEqual, rewritten := buildNullSafeEqualFromOR(sctx, sf) | ||
| if !rewritten { | ||
| return predicate | ||
| } |
There was a problem hiding this comment.
Missing recursive descent for non-matching OR nodes
When top-level OR doesn’t match the 2-branch pattern, this path returns early and skips recursive simplification of its children. That can miss valid nested null-safe rewrites.
Suggested fix
case ast.LogicOr:
nullSafeEqual, rewritten := buildNullSafeEqualFromOR(sctx, sf)
- if !rewritten {
- return predicate
- }
- if expression.MaybeOverOptimized4PlanCache(sctx.GetExprCtx(), predicate) {
- sctx.GetSessionVars().StmtCtx.SetSkipPlanCache("null-safe equality predicate simplification is triggered")
+ if rewritten {
+ if expression.MaybeOverOptimized4PlanCache(sctx.GetExprCtx(), predicate) {
+ sctx.GetSessionVars().StmtCtx.SetSkipPlanCache("null-safe equality predicate simplification is triggered")
+ }
+ return nullSafeEqual
}
- return nullSafeEqual
+ dnfItems := expression.SplitDNFItems(sf)
+ changed := false
+ evalCtx := sctx.GetExprCtx().GetEvalCtx()
+ for i, item := range dnfItems {
+ newItem := simplifyNullSafeEqualPredicate(sctx, item)
+ if !newItem.Equal(evalCtx, item) {
+ dnfItems[i] = newItem
+ changed = true
+ }
+ }
+ if changed {
+ return expression.ComposeDNFCondition(sctx.GetExprCtx(), dnfItems...)
+ }
+ return predicate🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/rule/rule_predicate_simplification.go` around lines 567 -
572, When handling ast.LogicOr in the switch, the current early return when
buildNullSafeEqualFromOR(sctx, sf) returns rewritten==false skips simplifying OR
children; instead, if rewritten is false you must descend into sf's child
predicates, recursively simplify each child (using the same
predicate-simplification routine that operates on sf.Args/children) and
rebuild/return the OR node with simplified children; only return early when a
null-safe rewrite was applied (rewritten==true) and you replace the whole node.
Ensure you reference and update sf (the ScalarFunc/OR node) and call the
existing per-predicate simplifier rather than returning predicate unchanged.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68589 +/- ##
================================================
- Coverage 76.3752% 75.4296% -0.9457%
================================================
Files 2039 2021 -18
Lines 562884 566161 +3277
================================================
- Hits 429904 427053 -2851
- Misses 132062 139103 +7041
+ Partials 918 5 -913
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
What problem does this PR solve?
Issue Number: close #63741
Problem Summary:
TiDB does not simplify
(i = j) OR (i IS NULL AND j IS NULL)into the equivalent null-safe equality predicatei <=> j. The unsimplified predicate remains as a compoundORselection in both the default planner and cascades test output.What changed and how does it work?
This PR adds a narrow predicate simplification rule for the column-column pattern:
(col1 = col2) OR (col1 IS NULL AND col2 IS NULL)->col1 <=> col2ORbranch order and reversedIS NULLargument order.Regression coverage is added to the predicate simplification rule casetest outputs for both default and cascades paths.
Check List
Tests
Commands:
./tools/check/failpoint-go-test.sh pkg/planner/core/casetest/rule -run TestPredicateSimplification -count=1git diff --check origin/master...HEADSide effects
Documentation
Release note
Summary by CodeRabbit
(a = b) OR (a IS NULL AND b IS NULL)into null-safe equality operations, enabling more efficient query execution and reducing execution time for affected queries.