feat(scanner): add external row-address mask prefilter - #7288
Conversation
19a9518 to
ed88757
Compare
|
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:
📝 WalkthroughWalkthroughChangesThe scanner accepts an optional Row-address prefilter
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/src/dataset/scanner.rs`:
- Around line 737-741: The external_row_mask field and its ANN execution
plumbing should use Arc<RowAddrMask> instead of cloning RowAddrMask values.
Update new_knn_exec, ANNIvfSubIndexExec construction and with_new_children, plus
ann, multivec_ann, knn_combined, and flat-scan paths, to clone the Arc cheaply
while preserving mask behavior. Revise the field documentation to mention both
vector-search prefiltering and use as the plain-scan row source via
use_external_mask.
- Around line 1205-1214: The documentation for with_row_addr_prefilter must
match new_filtered_read: describe the mask as applying to both vector-search and
plain scans as implemented, rather than claiming it only affects nearest
queries. Reword “ANDed” using clear wording such as “combined with” to satisfy
the typos check, while preserving the existing row-address and branch semantics.
- Around line 1211-1214: Reject configurations using with_row_addr_prefilter
when the dataset manifest uses stable row IDs. Add an early invalid-input or
not-supported validation in validate_options or create_plan, before applying
external_row_mask, while preserving the existing behavior for datasets without
stable row IDs.
In `@rust/lance/src/io/exec/knn.rs`:
- Line 1393: Reword the documentation comment for the external row-address
allow/block mask to replace “ANDed” with a spell-check-safe equivalent, while
preserving that the mask is combined with the prefilter using logical AND.
In `@rust/lance/src/io/exec/row_addr_mask.rs`:
- Line 10: Replace the flagged “ANDed” wording in the row address mask
documentation and the corresponding occurrences in knn.rs and scanner.rs with
typos-safe terminology, preserving the description of deletion vectors being
combined by DatasetPreFilter. Alternatively, add an appropriate allow-entry to
the typos configuration if retaining the terminology is required.
🪄 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: 1f78fb0f-93fb-46cb-bcb6-510b6431ea01
📒 Files selected for processing (4)
rust/lance/src/dataset/scanner.rsrust/lance/src/io/exec.rsrust/lance/src/io/exec/knn.rsrust/lance/src/io/exec/row_addr_mask.rs
| pub fn with_row_addr_prefilter(&mut self, mask: RowAddrMask) -> &mut Self { | ||
| self.external_row_mask = Some(mask); | ||
| self | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm stable-row-id detection API and that no guard exists for external_row_mask
rg -nP 'uses_stable_row_ids' rust/lance/src/dataset/scanner.rs rust/lance/src/dataset/*.rs
rg -nP 'external_row_mask' rust/lance/src/dataset/scanner.rsRepository: lance-format/lance
Length of output: 2501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the scanner code around the external row mask, validation, and mask application paths.
sed -n '700,780p' rust/lance/src/dataset/scanner.rs
printf '\n---\n'
sed -n '1188,1235p' rust/lance/src/dataset/scanner.rs
printf '\n---\n'
sed -n '2888,2995p' rust/lance/src/dataset/scanner.rs
printf '\n---\n'
sed -n '3908,4110p' rust/lance/src/dataset/scanner.rs
printf '\n---\n'
sed -n '4728,4825p' rust/lance/src/dataset/scanner.rs
printf '\n---\n'
sed -n '2488,2535p' rust/lance/src/dataset/scanner.rsRepository: lance-format/lance
Length of output: 23512
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether scanner validation already rejects external_row_mask for stable-row-id datasets.
rg -nC 3 'validate_options|create_plan|external_row_mask|uses_stable_row_ids' rust/lance/src/dataset/scanner.rsRepository: lance-format/lance
Length of output: 13474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the row-address mask types and the exec path that consumes them.
rg -nC 4 'struct RowAddrMask|enum RowAddrMask|RowAddrMaskFilterExec|mask_as_index_input|RowAddr' rust/lance/srcRepository: lance-format/lance
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrowly inspect the RowAddrMask-related execution path to see what column it filters and
# whether scanner-specific validation already rejects stable-row-id datasets.
rg -nC 3 'RowAddrMaskFilterExec|mask_as_index_input|with_row_addr_prefilter|uses_stable_row_ids\(\)' rust/lance/src/dataset/scanner.rs rust/lance/src/index/prefilter.rs rust/lance/src -g '!**/target/**' | head -n 220Repository: lance-format/lance
Length of output: 17644
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the RowAddrMask-related execution path to see what column it filters and
# whether scanner-specific validation already rejects stable-row-id datasets.
sed -n '1,260p' rust/lance/src/index/prefilter.rs
printf '\n---\n'
sed -n '2388,2538p' rust/lance/src/dataset/scanner.rs
printf '\n---\n'
rg -nC 2 'RowAddrMaskFilterExec|mask_as_index_input|with_row_addr_prefilter' rust/lance/src -g '!**/target/**'Repository: lance-format/lance
Length of output: 24350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the row-address mask execution semantics to see whether it always interprets
# the input as physical row addresses or can also operate on stable row ids.
sed -n '1,220p' rust/lance/src/io/exec/row_addr_mask.rsRepository: lance-format/lance
Length of output: 5564
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' rust/lance/src/io/exec/row_addr_mask.rsRepository: lance-format/lance
Length of output: 5564
Reject with_row_addr_prefilter on stable-row-id datasets. This path reads _rowid, which only matches physical row addresses when stable row ids are disabled. On manifest.uses_stable_row_ids() datasets it can silently filter the wrong rows, so add an early invalid_input/not_supported check in validate_options or create_plan.
🤖 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/src/dataset/scanner.rs` around lines 1211 - 1214, Reject
configurations using with_row_addr_prefilter when the dataset manifest uses
stable row IDs. Add an early invalid-input or not-supported validation in
validate_options or create_plan, before applying external_row_mask, while
preserving the existing behavior for datasets without stable row IDs.
Source: Coding guidelines
ed88757 to
7c82510
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/src/dataset/scanner.rs (1)
2927-3008: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCritical:
external_row_maskis silently ignored on plain scans for legacy-storage datasets.
filtered_read(lines 3013-3050) dispatches tolegacy_filtered_readwhenself.dataset.is_legacy_storage()is true, andlegacy_filtered_readhas no awareness ofexternal_row_maskat all — it never builds anindex_inputfrom the mask and nothing wraps its output afterward. Contrast this with the vector-search paths (vector_search's flat fallback andknn_combined), which explicitly re-wrap their scan output withRowAddrMaskFilterExecregardless of which internal path executed. For the plain-scan path (create_plan→filtered_read_source→filtered_read), there is no such fallback wrap — masking is enforced only throughnew_filtered_read'suse_external_mask/mask_as_index_inputmechanism.The net effect: on a legacy-storage-format dataset, calling
Scanner::with_row_addr_prefilter(mask)followed by a plain (non-nearest) scan will return unmasked results, silently contradicting the updated doc ("On a plain scan the mask is used directly as the row source.").🐛 Proposed minimal safeguard (reject the unsupported combination)
fn validate_options(&self) -> Result<()> { if self.batch_readahead == 0 { return Err(Error::invalid_input_source( "batch_readahead must be greater than 0, got 0".into(), )); } + + if self.external_row_mask.is_some() + && self.nearest.is_none() + && self.dataset.is_legacy_storage() + { + return Err(Error::not_supported( + "with_row_addr_prefilter is not supported for plain scans on datasets using the legacy storage format".to_string(), + )); + }A more complete fix would be to implement mask support inside
legacy_filtered_read(or to wrapfiltered_read_source's output withRowAddrMaskFilterExecunconditionally, mirroring the vector-search paths) instead of just rejecting the combination.🤖 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/src/dataset/scanner.rs` around lines 2927 - 3008, Prevent plain scans from silently ignoring external_row_mask on legacy-storage datasets. In filtered_read, detect the combination of self.dataset.is_legacy_storage() and self.external_row_mask and return an explicit unsupported-operation error before dispatching to legacy_filtered_read. Preserve existing behavior for legacy scans without a mask and non-legacy masked scans through new_filtered_read.
🧹 Nitpick comments (2)
rust/lance/src/dataset/scanner.rs (2)
1210-1226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for the new external row-address mask feature.
This PR stack introduces a significant, correctness-sensitive feature (external
RowAddrMaskprefiltering) spanning three files, but no tests are visible in the diff for any layer: the core filtering primitives, the KNN mask-propagation wiring, or the publicScanner::with_row_addr_prefilterAPI and its downstream effects (plain scan, ANN, flat/appended-fragment vector search). Per coding guidelines, every feature should have corresponding tests.
rust/lance/src/dataset/scanner.rs#L1210-L1226: add tests exercisingScanner::with_row_addr_prefilterend-to-end for (a) a plain scan, (b) an indexed vector search, (c) a flat/unindexed vector search, and (d) appended-fragment vector search (knn_combined), verifying only masked rows are returned in each case.rust/lance/src/io/exec/row_addr_mask.rs#L33-L156: add unit tests forapply_mask(null_rowidhandling, allow/block selection) andMaskAndLoader::load(intersection with and without an inner loader).rust/lance/src/io/exec/knn.rs#L1104-L1981: add a test verifyingANNIvfSubIndexExec::executecorrectly intersectsexternal_maskwith an existing filter-derived prefilter loader (not just the no-prefilter case).🤖 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/src/dataset/scanner.rs` around lines 1210 - 1226, Add comprehensive tests for external RowAddrMask filtering: in rust/lance/src/dataset/scanner.rs lines 1210-1226, cover Scanner::with_row_addr_prefilter for plain scans, indexed vector searches, flat/unindexed searches, and appended-fragment knn_combined searches, verifying only masked rows return; in rust/lance/src/io/exec/row_addr_mask.rs lines 33-156, test apply_mask null _rowid handling and allow/block selection plus MaskAndLoader::load with and without an inner loader; in rust/lance/src/io/exec/knn.rs lines 1104-1981, test ANNIvfSubIndexExec::execute intersects external_mask with an existing filter-derived prefilter loader.Source: Coding guidelines
3052-3076: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate serialize/stream/OneShotExec boilerplate between
mask_as_index_inputandu64s_as_take_input.Both functions build an
IndexExprResult, computefragments_covered/format, serialize into a batch, and wrap it in aOneShotExecviaRecordBatchStreamAdapter— identical tail logic duplicated verbatim. Extracting a shared helper (e.g.fn index_result_as_index_input(&self, index_result: IndexExprResult) -> Result<Arc<dyn ExecutionPlan>>) would remove the duplication and reduce the risk of the two implementations diverging over time.♻️ Proposed refactor
+ fn index_result_as_index_input( + &self, + index_result: IndexExprResult, + ) -> Result<Arc<dyn ExecutionPlan>> { + let fragments_covered = self.dataset.fragment_bitmap.as_ref().clone(); + let format = self.index_expr_result_format(); + let batch = index_result.serialize(&fragments_covered, format)?; + let schema = batch.schema(); + let stream = futures::stream::once(async move { Ok(batch) }); + let stream = Box::pin(RecordBatchStreamAdapter::new(schema, stream)); + Ok(Arc::new(OneShotExec::new(stream))) + } + fn mask_as_index_input(&self, mask: &RowAddrMask) -> Result<Arc<dyn ExecutionPlan>> { - let index_result = IndexExprResult::exact(mask.clone()); - let fragments_covered = self.dataset.fragment_bitmap.as_ref().clone(); - let format = self.index_expr_result_format(); - let batch = index_result.serialize(&fragments_covered, format)?; - let schema = batch.schema(); - let stream = futures::stream::once(async move { Ok(batch) }); - let stream = Box::pin(RecordBatchStreamAdapter::new(schema, stream)); - Ok(Arc::new(OneShotExec::new(stream))) + self.index_result_as_index_input(IndexExprResult::exact(mask.clone())) } fn u64s_as_take_input(&self, u64s: Vec<u64>) -> Result<Arc<dyn ExecutionPlan>> { let row_addrs = RowAddrTreeMap::from_iter(u64s); let row_addr_mask = RowAddrMask::from_allowed(row_addrs); - let index_result = IndexExprResult::exact(row_addr_mask); - let fragments_covered = self.dataset.fragment_bitmap.as_ref().clone(); - let format = self.index_expr_result_format(); - let batch = index_result.serialize(&fragments_covered, format)?; - let schema = batch.schema(); - let stream = futures::stream::once(async move { Ok(batch) }); - let stream = Box::pin(RecordBatchStreamAdapter::new(schema, stream)); - Ok(Arc::new(OneShotExec::new(stream))) + self.index_result_as_index_input(IndexExprResult::exact(row_addr_mask)) }🤖 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/src/dataset/scanner.rs` around lines 3052 - 3076, Extract the shared serialization and execution-plan construction from mask_as_index_input and u64s_as_take_input into a helper such as index_result_as_index_input, accepting an IndexExprResult and returning the OneShotExec plan. Update both callers to build their respective IndexExprResult and delegate to the helper, preserving existing behavior.
🤖 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/src/dataset/scanner.rs`:
- Around line 2927-3008: Prevent plain scans from silently ignoring
external_row_mask on legacy-storage datasets. In filtered_read, detect the
combination of self.dataset.is_legacy_storage() and self.external_row_mask and
return an explicit unsupported-operation error before dispatching to
legacy_filtered_read. Preserve existing behavior for legacy scans without a mask
and non-legacy masked scans through new_filtered_read.
---
Nitpick comments:
In `@rust/lance/src/dataset/scanner.rs`:
- Around line 1210-1226: Add comprehensive tests for external RowAddrMask
filtering: in rust/lance/src/dataset/scanner.rs lines 1210-1226, cover
Scanner::with_row_addr_prefilter for plain scans, indexed vector searches,
flat/unindexed searches, and appended-fragment knn_combined searches, verifying
only masked rows return; in rust/lance/src/io/exec/row_addr_mask.rs lines
33-156, test apply_mask null _rowid handling and allow/block selection plus
MaskAndLoader::load with and without an inner loader; in
rust/lance/src/io/exec/knn.rs lines 1104-1981, test ANNIvfSubIndexExec::execute
intersects external_mask with an existing filter-derived prefilter loader.
- Around line 3052-3076: Extract the shared serialization and execution-plan
construction from mask_as_index_input and u64s_as_take_input into a helper such
as index_result_as_index_input, accepting an IndexExprResult and returning the
OneShotExec plan. Update both callers to build their respective IndexExprResult
and delegate to the helper, preserving existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 774a77ab-63af-4315-b074-31a60d5f31e5
📒 Files selected for processing (4)
rust/lance/src/dataset/scanner.rsrust/lance/src/io/exec.rsrust/lance/src/io/exec/knn.rsrust/lance/src/io/exec/row_addr_mask.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/lance/src/dataset/scanner.rs (2)
1210-1222: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd a compiling rustdoc example and API links.
with_row_addr_prefilteris a new public API, but its documentation has no example or links toRowAddrMask,RowAddrTreeMap, or the relevant scan methods. Add a synchronizedno_runexample using the actual signature.🤖 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/src/dataset/scanner.rs` around lines 1210 - 1222, Update the rustdoc for with_row_addr_prefilter to include a compiling no_run example using its actual signature, and add API links for RowAddrMask, RowAddrTreeMap, and the relevant scanner methods. Keep the example synchronized with the current public API and existing documentation behavior.Source: Coding guidelines
3975-3979: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAdd Scanner-level regression tests for the new mask contract.
The supplied tests cover primitive filtering and loader composition, but not
Scanner::with_row_addr_prefilter. Add coverage for plain allow/block scans with SQL refinement, legacy rejection, flat fallback, and ANN appended/unindexed fragments.Also applies to: 4129-4133, 4784-4790, 4844-4850
🤖 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/src/dataset/scanner.rs` around lines 3975 - 3979, Add Scanner-level regression tests covering the row-address mask contract through Scanner::with_row_addr_prefilter: validate allow/block scans with SQL refinement, rejection of legacy inputs, flat fallback behavior, and ANN scans spanning appended and unindexed fragments. Exercise the relevant scanner paths, including the RowAddrMaskFilterExec application, and assert both returned rows and expected errors for unsupported cases.Source: Coding guidelines
rust/lance/src/io/exec/row_addr_mask.rs (1)
109-121: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReplace panic-based extraction in both library paths.
rust/lance/src/io/exec/row_addr_mask.rs#L109-L121: replacechildren.pop().expect(...)with explicit error handling.rust/lance/src/dataset/scanner.rs#L2991-L2993: replaceexternal_row_mask.as_deref().unwrap()with pattern matching or contextual error propagation.🤖 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/src/io/exec/row_addr_mask.rs` around lines 109 - 121, Replace panic-based extraction in RowAddrMaskFilterExec::with_new_children at rust/lance/src/io/exec/row_addr_mask.rs:109-121 with explicit error handling that returns a DataFusionResult error if the child is unavailable; retain the existing validation for exactly one child. Also update the external_row_mask access in rust/lance/src/dataset/scanner.rs:2991-2993 to use pattern matching or contextual error propagation instead of unwrap, handling the missing-mask case without panicking.Source: Coding guidelines
🧹 Nitpick comments (2)
rust/lance/src/io/exec/row_addr_mask.rs (2)
147-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the actual
_rowidtype in the error.Report
row_id_column.data_type()alongside the expectedUInt64type so malformed execution plans are diagnosable.🤖 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/src/io/exec/row_addr_mask.rs` around lines 147 - 148, Update the error construction in the row ID validation flow to include row_id_column.data_type() alongside the expected UInt64 type. Preserve the existing DataFusionError::Internal failure path and type-checking behavior.Source: Coding guidelines
50-56: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAvoid deep-copying the shared mask on every load.
MaskAndLoaderreceives a sharedArc<RowAddrMask>, soArc::unwrap_or_clonenormally takes the clone branch and copies the potentially large mask even wheninneris absent.Arc::unwrap_or_cloneonly unwraps when the reference is unique. (doc.rust-lang.org) Consider preserving shared ownership in the loader contract, or adding a shared fast path; only materialize a new mask for intersections.🤖 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/src/io/exec/row_addr_mask.rs` around lines 50 - 56, Update MaskAndLoader::load to avoid materializing a cloned RowAddrMask when inner is absent: preserve or return the shared Arc-backed mask through the loader contract, and only create an owned mask when applying the intersection with inner.load().await. Keep the existing intersection behavior unchanged.Source: MCP tools
🤖 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/src/io/exec/row_addr_mask.rs`:
- Around line 244-259: Extend mask_and_loader_with_inner_intersects to cover
block-list × allow-list and block-list × block-list combinations in addition to
the existing allow-list × allow-list case. Construct the corresponding
RowAddrMask values, load them through MaskAndLoader, and assert selected and
unselected addresses to verify the expected intersection algebra.
- Around line 215-222: Strengthen apply_mask_missing_rowid_column_errs by
matching the returned DataFusionError::Internal variant and asserting the
expected missing-column message instead of only checking is_err(). Add a
separate test covering a present rowid column with an unsupported data type,
verifying that apply_mask returns the expected Internal error and message for
the wrong-type branch.
---
Outside diff comments:
In `@rust/lance/src/dataset/scanner.rs`:
- Around line 1210-1222: Update the rustdoc for with_row_addr_prefilter to
include a compiling no_run example using its actual signature, and add API links
for RowAddrMask, RowAddrTreeMap, and the relevant scanner methods. Keep the
example synchronized with the current public API and existing documentation
behavior.
- Around line 3975-3979: Add Scanner-level regression tests covering the
row-address mask contract through Scanner::with_row_addr_prefilter: validate
allow/block scans with SQL refinement, rejection of legacy inputs, flat fallback
behavior, and ANN scans spanning appended and unindexed fragments. Exercise the
relevant scanner paths, including the RowAddrMaskFilterExec application, and
assert both returned rows and expected errors for unsupported cases.
In `@rust/lance/src/io/exec/row_addr_mask.rs`:
- Around line 109-121: Replace panic-based extraction in
RowAddrMaskFilterExec::with_new_children at
rust/lance/src/io/exec/row_addr_mask.rs:109-121 with explicit error handling
that returns a DataFusionResult error if the child is unavailable; retain the
existing validation for exactly one child. Also update the external_row_mask
access in rust/lance/src/dataset/scanner.rs:2991-2993 to use pattern matching or
contextual error propagation instead of unwrap, handling the missing-mask case
without panicking.
---
Nitpick comments:
In `@rust/lance/src/io/exec/row_addr_mask.rs`:
- Around line 147-148: Update the error construction in the row ID validation
flow to include row_id_column.data_type() alongside the expected UInt64 type.
Preserve the existing DataFusionError::Internal failure path and type-checking
behavior.
- Around line 50-56: Update MaskAndLoader::load to avoid materializing a cloned
RowAddrMask when inner is absent: preserve or return the shared Arc-backed mask
through the loader contract, and only create an owned mask when applying the
intersection with inner.load().await. Keep the existing intersection behavior
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: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 90670792-f928-4db7-8cee-3313ab4151d4
📒 Files selected for processing (2)
rust/lance/src/dataset/scanner.rsrust/lance/src/io/exec/row_addr_mask.rs
36a3b67 to
4198c4e
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/lance/src/dataset/scanner.rs (1)
5623-5717: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd test coverage for stable row ids and block-list masks.
The new integration tests only exercise
RowAddrMask::from_allowed(...)onTestVectorDataset::new(LanceFileVersion::Stable, false)(stable row ids disabled). Two gaps worth closing:
- No test uses
enable_stable_row_ids: true, despitewith_row_addr_prefilter's doc explicitly claiming both row-address and stable-row-id spaces are "handled" — this is exactly the scenario a past review flagged as a correctness risk.- No test uses
RowAddrMask::from_block(...)at the scanner level.mask_as_index_input→IndexExprResult::exact(mask.clone())is a new code path for block masks specifically (previously only allow-masks flowed throughu64s_as_take_input); the row_addr_mask.rs unit tests cover block masks only forRowAddrMaskFilterExec/MaskAndLoader, not for the plain-scanFilteredReadExecindex-input serialization.🤖 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/src/dataset/scanner.rs` around lines 5623 - 5717, Extend the row-address scanner tests around row_addr_mask_plain_scan_allow_with_refine and row_addr_mask_ann_search_only_allowed to construct TestVectorDataset with enable_stable_row_ids: true and verify allowed row IDs remain correct. Add scanner-level coverage using RowAddrMask::from_block(...) through the plain-scan path, asserting the returned rows match the block mask, including any refine behavior needed to exercise FilteredReadExec index-input serialization.
🤖 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 `@rust/lance/src/dataset/scanner.rs`:
- Around line 5623-5717: Extend the row-address scanner tests around
row_addr_mask_plain_scan_allow_with_refine and
row_addr_mask_ann_search_only_allowed to construct TestVectorDataset with
enable_stable_row_ids: true and verify allowed row IDs remain correct. Add
scanner-level coverage using RowAddrMask::from_block(...) through the plain-scan
path, asserting the returned rows match the block mask, including any refine
behavior needed to exercise FilteredReadExec index-input serialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 63da2cce-8392-4b14-8a20-6bc24eb1f5f8
📒 Files selected for processing (4)
rust/lance/src/dataset/scanner.rsrust/lance/src/io/exec.rsrust/lance/src/io/exec/knn.rsrust/lance/src/io/exec/row_addr_mask.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/src/dataset/scanner.rs (1)
1222-1224: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new public builder with a compiling example.
Scanner::with_row_addr_prefilteris a public API but has no///documentation. Describe the mask’s allow/block and_rowidsemantics, plain/ANN behavior, and add a compiling example with links toRowAddrMaskand related scanner methods.As per coding guidelines: “Document all public APIs with examples and links to relevant structs and methods; keep examples synchronized with actual signatures.”
🤖 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/src/dataset/scanner.rs` around lines 1222 - 1224, Document the public Scanner::with_row_addr_prefilter builder with /// comments covering RowAddrMask allow/block behavior, _rowid semantics, and differences between plain and ANN scans. Add a compiling example using the actual method signature, linking to RowAddrMask and related Scanner methods with intra-doc links, and place the documentation directly above the method.Source: Coding guidelines
🧹 Nitpick comments (1)
rust/lance/src/dataset/scanner.rs (1)
5632-5636: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive the
rstestcases readable names. Replace#[values(false, true)]with named cases in both tests, e.g.#[case::without_stable_row_ids(false)]and#[case::with_stable_row_ids(true)], so failures identify the row-ID mode.🤖 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/src/dataset/scanner.rs` around lines 5632 - 5636, Update both affected rstest functions, including row_addr_mask_plain_scan_allow_block_refine, by replacing #[values(false, true)] with named #[case] entries for false and true. Name the cases without_stable_row_ids and with_stable_row_ids so test failures identify the row-ID mode.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/src/dataset/scanner.rs`:
- Around line 1222-1224: Document the public Scanner::with_row_addr_prefilter
builder with /// comments covering RowAddrMask allow/block behavior, _rowid
semantics, and differences between plain and ANN scans. Add a compiling example
using the actual method signature, linking to RowAddrMask and related Scanner
methods with intra-doc links, and place the documentation directly above the
method.
---
Nitpick comments:
In `@rust/lance/src/dataset/scanner.rs`:
- Around line 5632-5636: Update both affected rstest functions, including
row_addr_mask_plain_scan_allow_block_refine, by replacing #[values(false, true)]
with named #[case] entries for false and true. Name the cases
without_stable_row_ids and with_stable_row_ids so test failures identify the
row-ID mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 47923168-701a-4a1a-bc0c-100436bfc520
📒 Files selected for processing (1)
rust/lance/src/dataset/scanner.rs
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/src/dataset/scanner.rs (3)
5683-5687: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFormat the new test assertions with rustfmt.
cargo fmt --all -- --checkcurrently fails.
rust/lance/src/dataset/scanner.rs#L5683-L5687: format the chainedcolumn_by_name(...).as_primitive::<Int32Type>()assertion.rust/lance/src/dataset/scanner.rs#L5731-L5735: format the multiline ANN allowlist assertion.🤖 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/src/dataset/scanner.rs` around lines 5683 - 5687, Run rustfmt on the new test assertions in rust/lance/src/dataset/scanner.rs at lines 5683-5687 and 5731-5735: format the chained column_by_name(...).as_primitive::<Int32Type>() assertion and the multiline ANN allowlist assertion to match cargo fmt output.Source: Pipeline failures
1210-1225: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new public builder with links and an example.
with_row_addr_prefilteris public but has no usage example or Rustdoc links to relevant APIs such as [RowAddrMask] and the scan execution methods.As per coding guidelines: “Document all public APIs with examples and links 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/src/dataset/scanner.rs` around lines 1210 - 1225, Expand the Rustdoc for with_row_addr_prefilter to include links to RowAddrMask and the relevant scan execution methods, plus a concise example showing how to construct and apply the mask before running a scan. Keep the existing behavior and explanation of dataset _rowid semantics unchanged.Source: Coding guidelines
2935-2947: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat mask-backed plain scans as filtered during source planning.
filtered_read_sourcestill derives projection andscan_range_before_filterfrom the originalfilter_plan. Consequently, mask-only scans can applylimit/offsetbefore masking, and exact scalar-index predicates can be rewritten here into a refine filter after their predicate columns were omitted from the projection. This can return too few rows or fail evaluating the refine expression.Carry
use_external_mask/effective_filterinto source planning: disable pre-mask range pushdown and include refine columns. Add regressions for a maskedlimit/offsetscan and a masked exact scalar-index predicate projecting an unrelated column.🤖 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/src/dataset/scanner.rs` around lines 2935 - 2947, The source-planning path still uses the original filter plan for external-mask plain scans, allowing limit/offset pushdown before masking and omitting columns needed by rewritten refine predicates. Update filtered_read_source and its projection/scan-range planning to use use_external_mask and effective_filter, disabling pre-mask range pushdown while retaining refine columns; add regressions covering masked limit/offset scans and masked exact scalar-index predicates projecting an unrelated column.
🤖 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/src/dataset/scanner.rs`:
- Around line 5683-5687: Run rustfmt on the new test assertions in
rust/lance/src/dataset/scanner.rs at lines 5683-5687 and 5731-5735: format the
chained column_by_name(...).as_primitive::<Int32Type>() assertion and the
multiline ANN allowlist assertion to match cargo fmt output.
- Around line 1210-1225: Expand the Rustdoc for with_row_addr_prefilter to
include links to RowAddrMask and the relevant scan execution methods, plus a
concise example showing how to construct and apply the mask before running a
scan. Keep the existing behavior and explanation of dataset _rowid semantics
unchanged.
- Around line 2935-2947: The source-planning path still uses the original filter
plan for external-mask plain scans, allowing limit/offset pushdown before
masking and omitting columns needed by rewritten refine predicates. Update
filtered_read_source and its projection/scan-range planning to use
use_external_mask and effective_filter, disabling pre-mask range pushdown while
retaining refine columns; add regressions covering masked limit/offset scans and
masked exact scalar-index predicates projecting an unrelated column.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 40a759b0-0578-417d-822f-652ceae85d48
📒 Files selected for processing (1)
rust/lance/src/dataset/scanner.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 `@rust/lance/src/dataset/scanner.rs`:
- Around line 2947-2970: Update the scan validation around use_external_mask and
the full-text-search setup to reject any request combining
with_row_addr_prefilter with full_text_search. Return a descriptive error at the
API boundary before FTS planning or prefilter_source can run, while preserving
existing behavior for plain and vector scans.
🪄 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: QUIET
Plan: Pro Plus
Run ID: 2933c7e5-1ee8-4779-a7ef-77acc4a0d9f5
📒 Files selected for processing (3)
rust/lance/src/dataset/scanner.rsrust/lance/src/io/exec/filtered_read.rsrust/lance/src/io/exec/row_addr_mask.rs
4ac24a7 to
cb2a120
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
b72c16f to
1520ee2
Compare
|
@coderabbitai review |
1520ee2 to
a0ef305
Compare
|
@coderabbitai review |
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-changes
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2c6672d to
466c1c2
Compare
Lets callers pass a serialized RowAddrMask as an allow/block prefilter into vector, full-text, and plain scans, reusing the scanner's retrieval plan. The mask feeds the KNN prefilter source on the ANN branch, the FTS prefilter so BM25 top-k is computed over masked rows, and FilteredReadExec as the row source for plain scans; a new RowAddrMaskFilterExec honors the mask on the flat/unindexed-fragment branch. Addresses lance-format#6852.
Adds `row_addr_allowlist` / `row_addr_blocklist` to `Dataset.scanner`, plus a `ScannerBuilder.row_addr_prefilter()` setter, so a caller can restrict a scan, a KNN search, or a full-text search to a precomputed set of row addresses. Both are serialized RowAddrTreeMap payloads rather than objects. Two Python extension modules each link their own copy of the lance crates and cannot share a Rust value, but they can agree on this encoding, so the mask may be built by a different module than the one that runs the scan. The bytes are decoded through RowAddrMask::from_serialized_parts, so no binding reimplements the allow/block combination.
The cross-column compound scorer is a separate exec from the same-column one and builds its own prefilter, so the external row-address mask has to reach it independently. Without it the scorer returns rows the caller excluded.
466c1c2 to
c372d13
Compare
lance-gatekeeper
Bot
removed
the
K-changes
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-approved
lance-gatekeeper
Bot
removed
the
K-approved
lance-gatekeeper
Bot
added
the
K-approved
3 participants
Addresses #6852.
What
Adds
Scanner::with_row_addr_prefilter(RowAddrMask), letting callers pass aprecomputed row-address allow/block mask as a prefilter into vector and plain
scans, reusing the scanner's existing retrieval plan rather than re-deriving it.
Motivation
Some pipelines precompute a set of eligible rows out-of-band (e.g. a stored
bitmap of rows belonging to a logical subset / dataset) and want to run KNN or a
plain scan restricted to that set -- without expressing it as a SQL filter. A
multi-hundred-thousand-element
IN (...)is impractical to build and parse;passing the row set directly is far cheaper.
How
The mask threads into the existing prefilter machinery at three points:
PreFilterSourceintonew_knn_exec, ANDed withany deletion/SQL prefilter via a
MaskAndLoader.RowAddrMaskFilterExecfiltersscan output by
_rowid, so rows appended after the index build are honored.FilteredReadExecindex input, so only masked rows are read; a SQL filter becomes a refine on top.
Deletions are still applied by
DatasetPreFilter; illegal addresses are ignored.Status
Draft, pending API agreement on #6852. Behavior is exercised by an out-of-tree
PyO3 binding's test suite (based on v7.0.0, this PR is rebased);
cargo check -p lanceandcargo fmtare clean. I'dappreciate guidance on the public API shape before finalizing in-tree tests.