ranger: enhance range extraction for complex OR filters#68446
ranger: enhance range extraction for complex OR filters#68446hawkingrei wants to merge 5 commits into
Conversation
ti-chi-bot
Bot
added
release-note
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an intersection helper for CNF item ranges, invokes it early in DNF-aware detachment to return intersected DetachRangeResult (with ColumnValues), updates expected plan snapshots, and adds a test for complex OR filter range extraction. ChangesComplex OR Filter Range Extraction
Sequence DiagramsequenceDiagram
participant Detacher as DetachCondAndBuildRangeForIndex
participant Intersector as intersectCNFItemWithBaseRange
participant Result as DetachRangeResult
Detacher->>Intersector: attempt intersection(baseRes, bestCNFItemRes, newConditions, originalConditions)
Intersector->>Result: return(updated Ranges, AccessConds, RemainedConds) or empty
Detacher->>Result: copy ColumnValues and return (on success)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/util/ranger/detacher.go (1)
282-283: ⚡ Quick winExplain the
RemainedCondsinvariant here.This recomputation is only obviously safe if the reader knows the original DNF is already preserved in
baseRes.RemainedConds, so a brief comment here would make future refactors much less likely to break the exact-filter contract. As per coding guidelines "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs, and SHOULD NOT restate what the code already makes clear."🤖 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/util/ranger/detacher.go` around lines 282 - 283, Add a concise explanatory comment above the recomputation of RemainedConds that states the invariant: baseRes.RemainedConds is expected to already hold a DNF-equivalent representation of the original filters (the exact-filter contract), so it's safe to compute remainedConds := removeConditions(sctx.ExprCtx.GetEvalCtx(), newConditions, baseRes.AccessConds) and merge with AppendConditionsIfNotExist into baseRes.RemainedConds without altering DNF semantics; mention the role of removeConditions, AppendConditionsIfNotExist, baseRes.AccessConds and that future refactors must preserve this DNF invariant to avoid breaking exact-filter behavior.pkg/util/ranger/ranger_test.go (1)
2513-2546: 🏗️ Heavy liftAdd a planner-level regression for the scan shape too.
This locks the detacher output, but not the end-to-end contract from Issue
#68408: the optimizer should keep using one ordered index-range scan instead of falling back to the fragmented lookup path. A smallEXPLAINassertion alongside this test would protect that cross-layer behavior if planner changes later stop consuming these narrower ranges.🤖 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/util/ranger/ranger_test.go` around lines 2513 - 2546, The test TestRangeExtractionForComplexORFilter currently asserts only ranger detacher outputs; add a planner-level regression by running an EXPLAIN on the same query and asserting the plan uses a single IndexRangeScan (or equivalent ordered index-range operator) rather than a IndexLookUp/IndexMerge/PointGet path; after building selection and before or after the ranger assertions, execute tk.MustQuery("explain "+sql).Rows() (or equivalent) and assert the returned plan text contains the planner operator name you expect (e.g., "IndexRangeScan" or the project’s exact planner phrase) to lock the scan-shape contract; reference TestRangeExtractionForComplexORFilter, DetachCondAndBuildRangeForIndex, and the sql variable when locating where to add this EXPLAIN assertion.
🤖 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/util/ranger/detacher.go`:
- Around line 282-283: Add a concise explanatory comment above the recomputation
of RemainedConds that states the invariant: baseRes.RemainedConds is expected to
already hold a DNF-equivalent representation of the original filters (the
exact-filter contract), so it's safe to compute remainedConds :=
removeConditions(sctx.ExprCtx.GetEvalCtx(), newConditions, baseRes.AccessConds)
and merge with AppendConditionsIfNotExist into baseRes.RemainedConds without
altering DNF semantics; mention the role of removeConditions,
AppendConditionsIfNotExist, baseRes.AccessConds and that future refactors must
preserve this DNF invariant to avoid breaking exact-filter behavior.
In `@pkg/util/ranger/ranger_test.go`:
- Around line 2513-2546: The test TestRangeExtractionForComplexORFilter
currently asserts only ranger detacher outputs; add a planner-level regression
by running an EXPLAIN on the same query and asserting the plan uses a single
IndexRangeScan (or equivalent ordered index-range operator) rather than a
IndexLookUp/IndexMerge/PointGet path; after building selection and before or
after the ranger assertions, execute tk.MustQuery("explain "+sql).Rows() (or
equivalent) and assert the returned plan text contains the planner operator name
you expect (e.g., "IndexRangeScan" or the project’s exact planner phrase) to
lock the scan-shape contract; reference TestRangeExtractionForComplexORFilter,
DetachCondAndBuildRangeForIndex, and the sql variable when locating where to add
this EXPLAIN assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b7e58928-70fd-432d-a68b-912d0594b0ea
📒 Files selected for processing (2)
pkg/util/ranger/detacher.gopkg/util/ranger/ranger_test.go
hawkingrei
force-pushed
the
issue-68408-range-or-filter
branch
2 times, most recently
from
63a6fe0 to
0365bd3
Compare
ti-chi-bot
Bot
added
size/L
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68446 +/- ##
================================================
- Coverage 77.2762% 76.4846% -0.7917%
================================================
Files 2010 1992 -18
Lines 555477 557695 +2218
================================================
- Hits 429252 426551 -2701
- Misses 125305 131097 +5792
+ Partials 920 47 -873
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hawkingrei
force-pushed
the
issue-68408-range-or-filter
branch
from
0365bd3 to
5a9e414
Compare
|
[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 |
|
/test check-dev2 |
|
@hawkingrei: The specified target(s) for Use DetailsIn response to this:
Instructions 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. |
|
/retest |
|
/test check-dev2 |
|
@hawkingrei: The specified target(s) for Use DetailsIn response to this:
Instructions 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. |
hawkingrei
changed the title
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/util/ranger/detacher.go`:
- Around line 292-314: The code unconditionally removes both
bestCNFItemRes.rangeResult.AccessConds and
originalConditions[bestCNFItemRes.offset] from baseRes.RemainedConds which drops
required residual predicates when bestCNFItemRes.RemainedConds is non-empty;
change the logic to only remove the original DNF condition
(originalConditions[offset]) from baseRes.RemainedConds when
bestCNFItemRes.RemainedConds is empty, otherwise preserve that original
condition (but you may still remove the rangeResult.AccessConds if they are
represented by the access range). Concretely, update the block that computes
coveredConds and mutates baseRes.RemainedConds to check
bestCNFItemRes.RemainedConds (the detach result) and only call removeConditions
to remove the DNF original condition when that slice is empty; keep using
removeConditions, AppendConditionsIfNotExist, newConditions, and
baseRes.AccessConds as before for computing and appending remainedConds.
🪄 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: a2ada7a9-1806-4532-9f7f-6a68d7bedc15
📒 Files selected for processing (1)
pkg/util/ranger/detacher.go
| coveredConds := bestCNFItemRes.rangeResult.AccessConds | ||
| if bestCNFItemRes.offset >= 0 && bestCNFItemRes.offset < len(originalConditions) { | ||
| coveredConds = AppendConditionsIfNotExist( | ||
| sctx.ExprCtx.GetEvalCtx(), | ||
| coveredConds, | ||
| []expression.Expression{originalConditions[bestCNFItemRes.offset]}, | ||
| ) | ||
| } | ||
| // The composite CNF item is now represented by the intersected range. Remove | ||
| // both its access conditions and original DNF condition from RemainedConds to | ||
| // avoid redundant filters while keeping exact semantics. | ||
| baseRes.RemainedConds = removeConditions( | ||
| sctx.ExprCtx.GetEvalCtx(), | ||
| baseRes.RemainedConds, | ||
| coveredConds, | ||
| ) | ||
| // baseRes.RemainedConds must keep a DNF-equivalent exact-filter contract | ||
| // for conditions not covered by baseRes.AccessConds. After removing the | ||
| // covered composite item, append only the new conditions that are neither | ||
| // covered by that item nor already represented by the final access range. | ||
| remainedConds := removeConditions(sctx.ExprCtx.GetEvalCtx(), newConditions, coveredConds) | ||
| remainedConds = removeConditions(sctx.ExprCtx.GetEvalCtx(), remainedConds, baseRes.AccessConds) | ||
| baseRes.RemainedConds = AppendConditionsIfNotExist(sctx.ExprCtx.GetEvalCtx(), baseRes.RemainedConds, remainedConds) |
There was a problem hiding this comment.
Keep residual filters when the selected CNF item still has RemainedConds.
extractBestCNFItemRanges can pick a DNF/CNF item whose own detach result still needs exact filtering. This helper unconditionally removes both bestCNFItemRes.rangeResult.AccessConds and originalConditions[offset] from baseRes.RemainedConds, so the early return can drop a required residual predicate and admit rows outside the original condition.
🐛 Proposed fix
- coveredConds := bestCNFItemRes.rangeResult.AccessConds
- if bestCNFItemRes.offset >= 0 && bestCNFItemRes.offset < len(originalConditions) {
+ coveredConds := bestCNFItemRes.rangeResult.AccessConds
+ if len(bestCNFItemRes.rangeResult.RemainedConds) == 0 &&
+ bestCNFItemRes.offset >= 0 && bestCNFItemRes.offset < len(originalConditions) {
coveredConds = AppendConditionsIfNotExist(
sctx.ExprCtx.GetEvalCtx(),
coveredConds,
[]expression.Expression{originalConditions[bestCNFItemRes.offset]},
)
}
@@
- remainedConds := removeConditions(sctx.ExprCtx.GetEvalCtx(), newConditions, coveredConds)
+ remainedConds := removeConditions(sctx.ExprCtx.GetEvalCtx(), newConditions, coveredConds)
+ remainedConds = AppendConditionsIfNotExist(
+ sctx.ExprCtx.GetEvalCtx(),
+ remainedConds,
+ bestCNFItemRes.rangeResult.RemainedConds,
+ )
remainedConds = removeConditions(sctx.ExprCtx.GetEvalCtx(), remainedConds, baseRes.AccessConds)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| coveredConds := bestCNFItemRes.rangeResult.AccessConds | |
| if bestCNFItemRes.offset >= 0 && bestCNFItemRes.offset < len(originalConditions) { | |
| coveredConds = AppendConditionsIfNotExist( | |
| sctx.ExprCtx.GetEvalCtx(), | |
| coveredConds, | |
| []expression.Expression{originalConditions[bestCNFItemRes.offset]}, | |
| ) | |
| } | |
| // The composite CNF item is now represented by the intersected range. Remove | |
| // both its access conditions and original DNF condition from RemainedConds to | |
| // avoid redundant filters while keeping exact semantics. | |
| baseRes.RemainedConds = removeConditions( | |
| sctx.ExprCtx.GetEvalCtx(), | |
| baseRes.RemainedConds, | |
| coveredConds, | |
| ) | |
| // baseRes.RemainedConds must keep a DNF-equivalent exact-filter contract | |
| // for conditions not covered by baseRes.AccessConds. After removing the | |
| // covered composite item, append only the new conditions that are neither | |
| // covered by that item nor already represented by the final access range. | |
| remainedConds := removeConditions(sctx.ExprCtx.GetEvalCtx(), newConditions, coveredConds) | |
| remainedConds = removeConditions(sctx.ExprCtx.GetEvalCtx(), remainedConds, baseRes.AccessConds) | |
| baseRes.RemainedConds = AppendConditionsIfNotExist(sctx.ExprCtx.GetEvalCtx(), baseRes.RemainedConds, remainedConds) | |
| coveredConds := bestCNFItemRes.rangeResult.AccessConds | |
| if len(bestCNFItemRes.rangeResult.RemainedConds) == 0 && | |
| bestCNFItemRes.offset >= 0 && bestCNFItemRes.offset < len(originalConditions) { | |
| coveredConds = AppendConditionsIfNotExist( | |
| sctx.ExprCtx.GetEvalCtx(), | |
| coveredConds, | |
| []expression.Expression{originalConditions[bestCNFItemRes.offset]}, | |
| ) | |
| } | |
| // The composite CNF item is now represented by the intersected range. Remove | |
| // both its access conditions and original DNF condition from RemainedConds to | |
| // avoid redundant filters while keeping exact semantics. | |
| baseRes.RemainedConds = removeConditions( | |
| sctx.ExprCtx.GetEvalCtx(), | |
| baseRes.RemainedConds, | |
| coveredConds, | |
| ) | |
| // baseRes.RemainedConds must keep a DNF-equivalent exact-filter contract | |
| // for conditions not covered by baseRes.AccessConds. After removing the | |
| // covered composite item, append only the new conditions that are neither | |
| // covered by that item nor already represented by the final access range. | |
| remainedConds := removeConditions(sctx.ExprCtx.GetEvalCtx(), newConditions, coveredConds) | |
| remainedConds = AppendConditionsIfNotExist( | |
| sctx.ExprCtx.GetEvalCtx(), | |
| remainedConds, | |
| bestCNFItemRes.rangeResult.RemainedConds, | |
| ) | |
| remainedConds = removeConditions(sctx.ExprCtx.GetEvalCtx(), remainedConds, baseRes.AccessConds) | |
| baseRes.RemainedConds = AppendConditionsIfNotExist(sctx.ExprCtx.GetEvalCtx(), baseRes.RemainedConds, remainedConds) |
🤖 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/util/ranger/detacher.go` around lines 292 - 314, The code unconditionally
removes both bestCNFItemRes.rangeResult.AccessConds and
originalConditions[bestCNFItemRes.offset] from baseRes.RemainedConds which drops
required residual predicates when bestCNFItemRes.RemainedConds is non-empty;
change the logic to only remove the original DNF condition
(originalConditions[offset]) from baseRes.RemainedConds when
bestCNFItemRes.RemainedConds is empty, otherwise preserve that original
condition (but you may still remove the rangeResult.AccessConds if they are
represented by the access range). Concretely, update the block that computes
coveredConds and mutates baseRes.RemainedConds to check
bestCNFItemRes.RemainedConds (the detach result) and only call removeConditions
to remove the DNF original condition when that slice is empty; keep using
removeConditions, AppendConditionsIfNotExist, newConditions, and
baseRes.AccessConds as before for computing and appending remainedConds.
|
@hawkingrei: The following test 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. |
1 participant
What problem does this PR solve?
Issue Number: close #68408
Problem Summary:
For a complex OR filter on an index such as
(c14, c25, c1), range extraction could keep a wide first-column range like[1748604343,1748604343]instead of intersecting it with the more selective composite DNF item ranges onc25andc1.What changed and how does it work?
This PR keeps the fix local to
pkg/util/ranger:Check List
Tests
Test commands:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Refactor
Tests