fix(index): normalize same-column scalar index predicates#6782
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
The metrics/logging added in #6759 makes this change easier to observe and validate: after same-column predicate normalization, the scalar index planner should select a much narrower candidate part range for bounded range queries. |
|
@Xuanwo Could you help review this when you have time? Thanks. |
f72b67e to
99ed91e
Compare
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
- The range merge stops after pairing a bound with the first opposite bound instead of deriving the tightest range across the same-key conjunction. Filters with duplicate lower or upper bounds can still evaluate a wider bounded branch plus extra single-sided branches, leaving part of the broad-branch cost this change is meant to remove.
- The normalization pass repeatedly clones, flattens, linearly scans, and removes terms while the existing guard limits nesting depth rather than total AND terms. Large balanced filters can make scalar-index planning consume quadratic CPU and elevated memory before execution starts.
e7c32a4 to
94911e1
Compare
Reuse ScalarIndexExpr::optimize to normalize same-key scalar predicates. - merge compatible ranges into their tightest intersection - preserve needs_recheck and fragment coverage semantics - remove redundant exact IS NOT NULL predicates - process AND regions in a single pass and rebuild balanced trees - preserve OR, NOT, NULL, and incompatible-range boundaries
94911e1 to
b98cfd8
Compare
📝 WalkthroughWalkthroughScalar index expression optimization now performs conservative, metadata-aware range rewrites, preserves ChangesScalar Index Optimization
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Thanks for the suggestion. I’ve refactored the changes to extend The additional behaviors are now implemented as optimizer rules within each AND region, including redundant |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/lance-index/src/scalar/expression.rs (3)
1624-1635: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPublic
optimize()doc lacks an example and cross-links.The docstring on this public entry point describes range-merge/recheck/IS-NOT-NULL/NOT semantics but has no usage example and doesn't link (
[...]) to the helper methods/types it relies on (e.g.ScalarIndexSearch,try_merge_range).
As per coding guidelines, "All public APIs must have documentation with examples, and documentation should link to relevant structs and methods."🤖 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 `@rust/lance-index/src/scalar/expression.rs` around lines 1624 - 1635, The public ScalarIndexExpr::optimize documentation needs a usage example and links to the relevant implementation symbols. Extend its doc comment with a concise Rust example demonstrating optimization and intra-documentation links to ScalarIndexSearch and try_merge_range, while preserving the existing behavioral description.Source: Coding guidelines
1624-1837: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftConsider extracting the new AND-region normalization logic into a submodule.
optimize_with_context,optimize_and_tree,optimize_and_leaves,collect_and_leaves,rebuild_and_tree, and the key/merge helpers add a self-contained optimizer pass inline into an already very large file (tests alone run past line 5400). As per coding guidelines, "Extract substantial new logic such as bin packing or scheduling into dedicated submodules instead of inlining it into large files."🤖 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 `@rust/lance-index/src/scalar/expression.rs` around lines 1624 - 1837, Extract the AND-region normalization pass from ScalarIndexExpr into a dedicated submodule, including optimize_with_context, optimize_and_tree, optimize_and_leaves, collect_and_leaves, rebuild_and_tree, the query-key helpers, and try_merge_range. Keep ScalarIndexExpr::optimize as the public entry point and preserve the existing optimization behavior and interfaces while relocating the implementation out of the large expression module.Source: Coding guidelines
1711-1727: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
collect_and_leavesskips optimization for nestedNotleaves underAnd.other => leaves.push(other)bypassesoptimize_with_context, soAnd(Not(And(...range...)), ...)never merges the inner range. Route the catch-all throughoptimize_with_contextso every leaf gets the same context-aware rewrite.🤖 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 `@rust/lance-index/src/scalar/expression.rs` around lines 1711 - 1727, The catch-all branch in collect_and_leaves currently inserts leaf expressions without optimization. Update it to pass other through optimize_with_context using is_not_null_elidable before adding it to leaves, while preserving the existing recursive And handling and independently optimized Or handling.
🤖 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.
Outside diff comments:
In `@rust/lance-index/src/scalar/expression.rs`:
- Around line 1624-1635: The public ScalarIndexExpr::optimize documentation
needs a usage example and links to the relevant implementation symbols. Extend
its doc comment with a concise Rust example demonstrating optimization and
intra-documentation links to ScalarIndexSearch and try_merge_range, while
preserving the existing behavioral description.
- Around line 1624-1837: Extract the AND-region normalization pass from
ScalarIndexExpr into a dedicated submodule, including optimize_with_context,
optimize_and_tree, optimize_and_leaves, collect_and_leaves, rebuild_and_tree,
the query-key helpers, and try_merge_range. Keep ScalarIndexExpr::optimize as
the public entry point and preserve the existing optimization behavior and
interfaces while relocating the implementation out of the large expression
module.
- Around line 1711-1727: The catch-all branch in collect_and_leaves currently
inserts leaf expressions without optimization. Update it to pass other through
optimize_with_context using is_not_null_elidable before adding it to leaves,
while preserving the existing recursive And handling and independently optimized
Or handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 024970c2-f816-4409-8f35-d7dcf2426179
📒 Files selected for processing (1)
rust/lance-index/src/scalar/expression.rs
575c4d6 to
418c2d4
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance-index/src/scalar/expression.rs (1)
1625-1638: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a doc example for the public
optimize()API.The doc explains the NOT/OR/range/null-elimination semantics well and links to
SargableQuery::RangeandScalarIndexSearch, but has no runnable example. Per the guideline that public APIs need documentation with examples, consider adding a short# Examplesblock showingScalarIndexExpr::And(...).optimize()merging two ranges.As per coding guidelines, "All public APIs must have documentation with examples, and documentation should link to relevant structs and methods" (
**/*.{rs,md,py}).🤖 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 `@rust/lance-index/src/scalar/expression.rs` around lines 1625 - 1638, Add a runnable # Examples section to the public ScalarIndexExpr::optimize documentation, demonstrating ScalarIndexExpr::And(...) with two compatible range queries and asserting that optimize() merges them. Reuse the existing SargableQuery::Range and ScalarIndexSearch types and constructors visible in the module, while preserving the current semantic documentation.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@rust/lance-index/src/scalar/expression.rs`:
- Around line 1625-1638: Add a runnable # Examples section to the public
ScalarIndexExpr::optimize documentation, demonstrating ScalarIndexExpr::And(...)
with two compatible range queries and asserting that optimize() merges them.
Reuse the existing SargableQuery::Range and ScalarIndexSearch types and
constructors visible in the module, while preserving the current semantic
documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 58a79994-c1fe-44f9-86aa-4420ab1528a8
📒 Files selected for processing (1)
rust/lance-index/src/scalar/expression.rs
418c2d4 to
25552dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance-index/src/scalar/expression.rs (1)
1706-1734: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRange accumulator only ever retries merges against the first same-key leaf, missing intersections across mixed fragment-coverage groups.
range_positionsmaps eachScalarIndexQueryKeyto a single fixed position (the first leaf seen for that key). Whentry_merge_rangefails against that position (e.g. differingfragment_bitmap), the new leaf is pushed as a separate entry, but the map entry is not updated to also try that new leaf on subsequent occurrences of the same key. Concretely, with three same-key rangesL1(frag=None),L2(frag=A),L3(frag=A)in that order:L2fails to merge withL1(fragment mismatch) and is pushed standalone;L3then only retries againstL1(still mismatched) and is pushed standalone too —L2andL3, which sharefragment_bitmap=Aand could merge into one tighter range, are never tried against each other.This is the same class of issue raised previously by Xuanwo ("stops after pairing a bound with the first opposite bound instead of deriving the tightest range across the same-key conjunction"), reappearing for the fragment-coverage-grouped case rather than the plain 2-range case. The existing
test_optimize_respects_fragment_coverage_when_merging_rangestest only exercises the 2-leaf case and doesn't catch this.♻️ Proposed fix: track all unmerged candidate positions per key
- let mut range_positions = HashMap::new(); + let mut range_positions: HashMap<ScalarIndexQueryKey, Vec<usize>> = HashMap::new(); for leaf in leaves { ... if let Some(key) = leaf.range_query_key() { - match range_positions.entry(key) { - Entry::Vacant(entry) => { - entry.insert(optimized.len()); - optimized.push(leaf); - } - Entry::Occupied(entry) => { - if !optimized[*entry.get()].try_merge_range(&leaf) { - optimized.push(leaf); - } - } - } + let positions = range_positions.entry(key).or_default(); + let merged = positions + .iter() + .any(|&pos| optimized[pos].try_merge_range(&leaf)); + if !merged { + positions.push(optimized.len()); + optimized.push(leaf); + } } else { optimized.push(leaf); } }Please add a 3-leaf test with mixed fragment-coverage groups (as above) alongside the fix, since the current tests don't cover this ordering.
🤖 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 `@rust/lance-index/src/scalar/expression.rs` around lines 1706 - 1734, Update the range accumulation logic in the optimizer around range_positions so each ScalarIndexQueryKey tracks all unmerged candidate positions, not only the first leaf. When try_merge_range fails due to differing fragment coverage, retain the new leaf as a candidate and attempt subsequent same-key ranges against it as well as existing candidates, while preserving separate groups that cannot merge. Add a three-leaf test covering L1 with no fragment bitmap followed by L2 and L3 sharing fragment bitmap A, asserting L2 and L3 merge while remaining separate from L1.
🤖 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 `@rust/lance-index/src/scalar/expression.rs`:
- Around line 1676-1682: Update optimize_and_tree to avoid the runtime .expect()
when rebuilding the AND tree. Preserve the invariant check with debug_assert!
that leaves is non-empty, then handle the Option returned by rebuild_and_tree
without introducing a release-build panic, using the existing Self return
contract.
---
Outside diff comments:
In `@rust/lance-index/src/scalar/expression.rs`:
- Around line 1706-1734: Update the range accumulation logic in the optimizer
around range_positions so each ScalarIndexQueryKey tracks all unmerged candidate
positions, not only the first leaf. When try_merge_range fails due to differing
fragment coverage, retain the new leaf as a candidate and attempt subsequent
same-key ranges against it as well as existing candidates, while preserving
separate groups that cannot merge. Add a three-leaf test covering L1 with no
fragment bitmap followed by L2 and L3 sharing fragment bitmap A, asserting L2
and L3 merge while remaining separate from L1.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6fa5a374-3fa4-40f9-ba4f-387d5868a683
📒 Files selected for processing (1)
rust/lance-index/src/scalar/expression.rs
3 participants
Summary
This PR builds on #7477 instead of introducing a separate scalar-index normalization path.
#7477 added
ScalarIndexExpr::optimize(), merged half-open range predicates, and applied the optimizer before scalar-index execution. This PR extends that optimizer with the remaining rules and edge-case handling originally proposed in #6782:IS NOT NULLterm when another exact same-key predicate is already null-intolerantThis refactoring follows the review request to describe the behavior as optimizer rules and reuse the optimizer introduced by #7477.
Behavior
Within one contiguous AND region:
SargableQuery::Rangepredicates with the same column, index name, and index type are intersectedneeds_recheck = trueif any input range requires recheck, preserving perf: merge half-open range queries on the same BTree index #7477 behaviorx >= 200 AND x <= 100remain a valid empty range representationIS NOT NULLpredicate is removed only when another exact same-key predicate is null-intolerantIS NOT NULLIS NOT NULLelimination is disabled there to preserve SQL NULL semanticsComplexity
Each AND region is flattened once and processed with keyed accumulator positions in a single pass. This replaces the previous pairwise scan with expected O(n) leaf processing and derives the tightest range across planner-produced compatible terms.
The implementation moves leaves into stable output slots instead of cloning complete
ScalarIndexSearchvalues. The AND tree is rebuilt in balanced form to avoid deep planner-generated conjunctions.Validation
Coverage includes:
needs_recheckpropagationIS NOT NULLeliminationINpredicates with and without NULL!=predicatesoptimize()exampleValidated locally with:
cargo test -p lance-index --libcargo test -p lance-index --doc 'scalar::expression::ScalarIndexExpr::optimize'cargo test -p lance io::exec::scalar_index --libcargo test -p lance io::exec::count_pushdown --libcargo clippy -p lance-index --all-targets -- -D warningsSummary by CodeRabbit
NOTexpressions to preserve SQL NULL semantics, including more conservativeIS NOT NULLremoval.