planner: simplify contradictory inequality predicates | tidb-test=pr/2765#69324
planner: simplify contradictory inequality predicates | tidb-test=pr/2765#69324zimulala wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughInequality-only contradiction detection is added to the predicate simplification rule. New helpers scan predicate pairs for contradictory lower/upper bound comparisons on the same column with constant numeric values; contradictions replace the predicate set with a constant-false expression, producing ChangesInequality bounds contradiction in predicate simplification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
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 |
2acb254 to
856638e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69324 +/- ##
================================================
- Coverage 76.3192% 74.8501% -1.4691%
================================================
Files 2041 2034 -7
Lines 561552 571169 +9617
================================================
- Hits 428572 427521 -1051
- Misses 132076 143475 +11399
+ Partials 904 173 -731
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
856638e to
e8d6627
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/rule/rule_predicate_simplification.go (1)
411-414: 💤 Low valueConsider adding a brief comment explaining the comparison threshold difference.
The
cmp > 0vscmp >= 0distinction handles the subtle difference between inclusive and strict bounds:
a >= x AND a <= yallowsx == y(point match)- All other combinations (
>/<,>=/<,>/<=) require a non-empty open intervalA short inline comment would help future readers understand this nuance without re-deriving the logic.
📝 Suggested clarifying comment
if lowerType == greaterThanOrEqualPredicate && upperType == lessThanOrEqualPredicate { + // >= and <= can both be satisfied at a single point when lower == upper return cmp > 0 } + // At least one strict bound requires lower < upper for any value to exist in the range return cmp >= 0🤖 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 411 - 414, Add an inline comment before the return statement in the conditional block that checks if lowerType equals greaterThanOrEqualPredicate and upperType equals lessThanOrEqualPredicate to explain why cmp > 0 is used instead of cmp >= 0. The comment should clarify that when both bounds are inclusive (>= and <=), a point match where the lower bound equals the upper bound is allowed, requiring strict comparison, whereas other bound combinations with at least one strict inequality require non-strict comparison cmp >= 0 to ensure a valid non-empty interval exists.
🤖 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.
Nitpick comments:
In `@pkg/planner/core/rule/rule_predicate_simplification.go`:
- Around line 411-414: Add an inline comment before the return statement in the
conditional block that checks if lowerType equals greaterThanOrEqualPredicate
and upperType equals lessThanOrEqualPredicate to explain why cmp > 0 is used
instead of cmp >= 0. The comment should clarify that when both bounds are
inclusive (>= and <=), a point match where the lower bound equals the upper
bound is allowed, requiring strict comparison, whereas other bound combinations
with at least one strict inequality require non-strict comparison cmp >= 0 to
ensure a valid non-empty interval exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d9c9fcb4-7769-4d3a-b8b4-d1e7c8e251ce
📒 Files selected for processing (13)
br/pkg/task/BUILD.bazelbr/pkg/task/operator/BUILD.bazelpkg/executor/test/plancache/plan_cache_test.gopkg/planner/core/casetest/index/testdata/index_range_out.jsonpkg/planner/core/casetest/index/testdata/index_range_xut.jsonpkg/planner/core/casetest/partition/testdata/partition_pruner_out.jsonpkg/planner/core/casetest/partition/testdata/partition_pruner_xut.jsonpkg/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.gotests/integrationtest/r/agg_predicate_pushdown.resulttests/integrationtest/r/executor/partition/partition_boundaries.result
✅ Files skipped from review due to trivial changes (6)
- br/pkg/task/operator/BUILD.bazel
- pkg/planner/core/casetest/rule/testdata/predicate_simplification_out.json
- pkg/planner/core/casetest/partition/testdata/partition_pruner_xut.json
- tests/integrationtest/r/agg_predicate_pushdown.result
- pkg/planner/core/casetest/partition/testdata/partition_pruner_out.json
- tests/integrationtest/r/executor/partition/partition_boundaries.result
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/planner/core/casetest/index/testdata/index_range_xut.json
- pkg/planner/core/casetest/index/testdata/index_range_out.json
- pkg/planner/core/casetest/rule/testdata/predicate_simplification_xut.json
|
/retest |
This reverts commit e8d6627.
|
[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 |
|
@zimulala: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
What problem does this PR solve?
Issue Number: close #69058
Problem Summary:
For impossible predicates that only contain contradictory inequality bounds on
the same column, such as
a > 0 AND a < 0, predicate simplification did notreduce the plan to
TableDual. Equality contradictions, such asa > 0 AND a = 0, were already simplified.What changed and how does it work?
This PR extends predicate simplification to detect contradictions between
same-column numeric lower and upper bounds, including exclusive and inclusive
bound combinations:
a > x AND a < ya > x AND a <= ya >= x AND a < ya >= x AND a <= ywhen the lower bound is greater than the upper boundThe new simplification is intentionally narrow:
simplified by this path
When a contradiction is detected, the predicate list is replaced with a constant
false predicate, allowing the planner to produce
TableDual.Check List
Tests
Focused test:
Additional compile-only signal:
Integration tests:
cd tests/integrationtest ./run-tests.sh -t agg_predicate_pushdown ./run-tests.sh -t executor/partition/partition_boundaries -s ./integrationtest_tidb-serverSide effects
Documentation
Release note