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
row-address scanner tests around row_addr_mask_plain_scan_allow_with_refine and row_addr_mask_ann_search_only_allowed
|
@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.
|
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 (2)
rust/lance/src/io/exec/filtered_read.rs (1)
2300-2348: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMissing dedicated unit-test case for the new
index_input.is_some()guard.
test_with_fetch_limit_pushdownenumerates cases 1-6 forwith_fetch, but none construct a plan withindex_inputset, so the exact branch this diff adds (|| self.index_input.is_some()) isn't directly exercised at this unit level — coverage for it currently only comes indirectly via the scanner-levelrow_addr_mask_plain_scan_with_limittest. Consider adding a case here that builds a plan with a non-Noneindex_inputand assertswith_fetchreturnsNone, to guard this specific branch against regression.As per coding guidelines: "Every bugfix and feature must have corresponding tests."
🤖 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/filtered_read.rs` around lines 2300 - 2348, The with_fetch limit-pushdown tests lack direct coverage for the index_input guard. Extend test_with_fetch_limit_pushdown with a case that constructs a plan having a non-None index_input while both filters are absent, then assert with_fetch returns None; leave the existing cases unchanged.Source: Coding guidelines
rust/lance/src/dataset/scanner.rs (1)
3061-3072: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLegacy-storage mask guard also wrongly blocks FTS queries, not just plain scans.
The guard is
self.nearest.is_none() && self.external_row_mask.is_some(), butnearest.is_none()is also true for full-text-search queries (nearest andfull_text_queryare mutually exclusive).plan_flat_match_query(andprefilter_sourcewhen called fromfts()) unconditionally callsfiltered_readfor unindexed fragments, so any FTS query combined withwith_row_addr_prefilter()on a legacy-storage dataset will be rejected with "not supported for plain scans on legacy-storage datasets" — even though FTS masking is fully wired viaMatchQueryExec/PhraseQueryExec::with_external_maskand theRowAddrMaskFilterExecwrap inplan_flat_match_query, neither of which depends onnew_filtered_read's mask-as-index-input mechanism that this guard is meant to protect. The comment itself confirms only vector search is meant to be exempted, but FTS uses the exact same "downstream wrap" mechanism and should be exempted too.Use
self.use_external_mask()(which already excludes bothnearestandfull_text_query) instead of the ad-hocnearest.is_none()check.🐛 Proposed fix
- if self.nearest.is_none() && self.external_row_mask.is_some() { + if self.use_external_mask() { return Err(Error::not_supported( "with_row_addr_prefilter is not supported for plain scans on \ legacy-storage datasets" .to_string(), )); }🤖 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 3061 - 3072, Update the legacy-storage mask guard near the scanner planning logic to use self.use_external_mask() instead of checking self.nearest.is_none(). Preserve the external-row-mask condition so the guard only rejects masked scans that require the unsupported plain-scan path, while allowing FTS and vector queries handled through downstream mask wrapping.
🧹 Nitpick comments (1)
rust/lance/src/dataset/scanner.rs (1)
1213-1244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc omits the FTS + mask combination.
The doc describes vector-search and plain-scan behavior but never mentions full-text search, even though FTS masking is fully implemented (
MatchQueryExec/PhraseQueryExec::with_external_mask,RowAddrMaskFilterExecwrap inplan_flat_match_query) and covered byrow_addr_mask_fts_search_only_allowed. As written, a reader of this public API doc would have no indication FTS is supported.As per coding guidelines: "Document all public APIs with examples and links to relevant structs and methods."
✏️ Proposed doc addition
/// The mask is keyed in the dataset's `_rowid` space, so build it from the /// same dataset you query. That space is the row address when stable row ids /// are disabled and the stable row id when they are enabled; both are handled /// (index prefilter and filtered read branch on `uses_stable_row_ids`), so no /// caller-side translation is needed either way. + /// + /// On a full-text search, the mask restricts BM25 scoring on indexed + /// fragments and is applied to flat (unindexed) matches before top-k + /// consumption, so results are limited to masked rows on both branches.🤖 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 1213 - 1244, Update the public `with_row_addr_prefilter` documentation to explicitly describe full-text search behavior: explain that the mask is applied to FTS results, including match and phrase queries, and mention the relevant FTS filtering path (`plan_flat_match_query`/`RowAddrMaskFilterExec`). Add a concise FTS usage example or links to the relevant query methods while preserving the existing vector-search and plain-scan 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/src/dataset/scanner.rs`:
- Around line 3061-3072: Update the legacy-storage mask guard near the scanner
planning logic to use self.use_external_mask() instead of checking
self.nearest.is_none(). Preserve the external-row-mask condition so the guard
only rejects masked scans that require the unsupported plain-scan path, while
allowing FTS and vector queries handled through downstream mask wrapping.
In `@rust/lance/src/io/exec/filtered_read.rs`:
- Around line 2300-2348: The with_fetch limit-pushdown tests lack direct
coverage for the index_input guard. Extend test_with_fetch_limit_pushdown with a
case that constructs a plan having a non-None index_input while both filters are
absent, then assert with_fetch returns None; leave the existing cases unchanged.
---
Nitpick comments:
In `@rust/lance/src/dataset/scanner.rs`:
- Around line 1213-1244: Update the public `with_row_addr_prefilter`
documentation to explicitly describe full-text search behavior: explain that the
mask is applied to FTS results, including match and phrase queries, and mention
the relevant FTS filtering path
(`plan_flat_match_query`/`RowAddrMaskFilterExec`). Add a concise FTS usage
example or links to the relevant query methods while preserving the existing
vector-search and plain-scan documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 812fbd44-9377-45fd-a4ee-613d6d61ded6
📒 Files selected for processing (5)
rust/lance/src/dataset/scanner.rsrust/lance/src/io/exec/filtered_read.rsrust/lance/src/io/exec/fts.rsrust/lance/src/io/exec/row_addr_mask.rsrust/lance/src/io/exec/utils.rs
- with_row_addr_prefilter doc now describes full-text search behavior (mask combined into the FTS prefilter; unindexed flat branch masked via RowAddrMaskFilterExec) and links full_text_search. - Legacy-storage mask guard keys on use_external_mask() instead of nearest.is_none(), so FTS and vector masked scans (handled via their own prefilter wrapping) are no longer wrongly rejected on legacy storage; only the unsupported plain-scan mask path is rejected. - test_with_fetch_limit_pushdown gains a case asserting with_fetch returns None when an index_input is present with no filters (the external-mask shape).
|
@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)
3794-3805: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply the mask before flat FTS top-k selection.
FlatMatchQueryExecreceives limited FTS parameters beforeRowAddrMaskFilterExecremoves masked rows. Excluded high-scoring rows can consume result slots, hiding lower-scoring allowed matches. Wrapplanbefore creatingFlatMatchQueryExec, and add a flat-only FTS test where the only allowed match ranks belowk.Proposed fix
- let flat_match_plan: Arc<dyn ExecutionPlan> = Arc::new(FlatMatchQueryExec::new( + let plan: Arc<dyn ExecutionPlan> = if let Some(mask) = self.external_row_mask.clone() { + Arc::new(RowAddrMaskFilterExec::new(plan, mask)) + } else { + plan + }; + Ok(Arc::new(FlatMatchQueryExec::new( self.dataset.clone(), query.clone(), params.clone(), plan, - )); - // Unindexed fragments never reach the index-side prefilter, so apply the - // external row-address mask to the flat FTS results here (mirrors the ANN - // flat branch). Applied before the caller's top-k so masked-out rows do - // not consume result slots. - if let Some(mask) = self.external_row_mask.clone() { - return Ok(Arc::new(RowAddrMaskFilterExec::new(flat_match_plan, mask))); - } - Ok(flat_match_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 3794 - 3805, Apply external_row_mask to the FTS execution plan before constructing FlatMatchQueryExec, rather than wrapping its limited results afterward. Update the flat FTS branch around FlatMatchQueryExec::new to wrap plan with RowAddrMaskFilterExec when a mask exists, then pass the masked plan into FlatMatchQueryExec; add a flat-only FTS test where an excluded higher-ranked match would otherwise consume k and verify the lower-ranked allowed match is returned.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 3794-3805: Apply external_row_mask to the FTS execution plan
before constructing FlatMatchQueryExec, rather than wrapping its limited results
afterward. Update the flat FTS branch around FlatMatchQueryExec::new to wrap
plan with RowAddrMaskFilterExec when a mask exists, then pass the masked plan
into FlatMatchQueryExec; add a flat-only FTS test where an excluded
higher-ranked match would otherwise consume k and verify the lower-ranked
allowed match is returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: b61b5bb8-c894-45dd-a9cb-0e93fcb7fdb8
📒 Files selected for processing (2)
rust/lance/src/dataset/scanner.rsrust/lance/src/io/exec/filtered_read.rs
|
@coderabbitai Skipping this one — the premise doesn't hold against the current code. |
|
Tip For best results, initiate chat on the files or code changes.
I’m withdrawing the comment; placing the mask earlier would only be a potential scoring-cost optimization, not a correctness fix. 🐇💭 ✏️ Learnings added
|
2 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.