fix(mem-wal): refill scans after shadow filtering#7917
Conversation
|
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:
📝 WalkthroughWalkthroughMemWAL scan planning validates ChangesMemWAL scan limit refill
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
rust/lance/src/dataset/mem_wal/scanner/planner.rs-107-109 (1)
107-109: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a synchronized usage example for this public builder method.
The documentation links to
plan_scanbut has no example showingwith_overfetch_factorin a builder chain.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/mem_wal/scanner/planner.rs` around lines 107 - 109, Add a Rust doc-test example to the public builder method with_overfetch_factor, showing it used in a builder chain with its current signature and a subsequent plan_scan call. Keep the example synchronized with the actual builder types and link relevant methods or structs using valid intra-doc links.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.
Other comments:
In `@rust/lance/src/dataset/mem_wal/scanner/planner.rs`:
- Around line 107-109: Add a Rust doc-test example to the public builder method
with_overfetch_factor, showing it used in a builder chain with its current
signature and a subsequent plan_scan call. Keep the example synchronized with
the actual builder types and link relevant methods or structs using valid
intra-doc links.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 59c11888-1be0-4184-9dfb-afc3447c4b6b
📒 Files selected for processing (1)
rust/lance/src/dataset/mem_wal/scanner/planner.rs
e8b9a2d to
f5cf83a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rust/lance/src/dataset/mem_wal/scanner/planner.rs (1)
1001-1002: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the standard in-memory test URI. These tests do not reopen datasets or exercise flushed on-disk generations, so temporary directories add unnecessary filesystem setup.
rust/lance/src/dataset/mem_wal/scanner/planner.rs#L1001-L1002: replace the temporary directory-derived base URI withmemory://.rust/lance/src/dataset/mem_wal/scanner/planner.rs#L2149-L2150: replace the temporary directory-derived base URI withmemory://.As per coding guidelines, use plain
"memory://"URIs in 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/dataset/mem_wal/scanner/planner.rs` around lines 1001 - 1002, Replace the temporary-directory-derived base URI with the plain "memory://" URI in both affected test setup sites in rust/lance/src/dataset/mem_wal/scanner/planner.rs (lines 1001-1002 and 2149-2150), removing the unnecessary tempfile setup while preserving the tests’ existing behavior.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.
Inline comments:
In `@rust/lance/src/dataset/mem_wal/scanner/planner.rs`:
- Around line 198-205: Add a plan-level test covering the planner branch in the
scan-building flow where n_needed is Some(n), is_active is false, and
has_block_filter is false, and assert the finite fetch is pushed to the source
scan. Use a limited source without a block filter and inspect the generated plan
rather than only result rows, ensuring the test fails if Some(n) is not passed
to build_source_scan.
- Around line 107-109: Add a synchronized # Examples doctest to the public
builder documented by the comments near Self::plan_scan, showing construction,
setting a valid over-fetch factor, and invoking the builder’s scan-planning API
so it compiles; link referenced types and methods with intra-doc links
consistent with the existing vector and full-text planner documentation.
---
Nitpick comments:
In `@rust/lance/src/dataset/mem_wal/scanner/planner.rs`:
- Around line 1001-1002: Replace the temporary-directory-derived base URI with
the plain "memory://" URI in both affected test setup sites in
rust/lance/src/dataset/mem_wal/scanner/planner.rs (lines 1001-1002 and
2149-2150), removing the unnecessary tempfile setup while preserving the tests’
existing behavior.
🪄 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: abd47088-155e-4259-95d0-7819a2cb25d9
📒 Files selected for processing (1)
rust/lance/src/dataset/mem_wal/scanner/planner.rs
dee43d3 to
1c50531
Compare
hamersaw
left a comment
hamersaw
left a comment
There was a problem hiding this comment.
Great find! Using LocalLimitExec is a better approach for scan operations. The only change I'm proposing is, if the overfetch_factor configuration on LsmScanPlanner is not used anywhere (besides validation). Lets just go ahead and remove it, along wiht the with_overfetch_factor and stop passing it into the LsmScanBuilder::plan_scan. We still need it on vector + FTS searches, but it's moot on the regular scan path.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Addressed in 0424b95: removed |
0424b95 to
11de4d9
Compare
11de4d9 to
5729733
Compare
|
Thanks for the PR! |
2 participants
Summary
PkBlockFilterExecLocalLimitExecpull until it hasoffset + limitlive rows or reaches EOFCloses #7916
Why
A stale prefix could consume an on-disk source's finite limit before cross-generation shadow filtering removed it. The scan then returned fewer rows than requested even though later live rows existed. A finite over-fetch factor cannot make that exact.
Block-filtered scans now use the existing execution tree as a streaming refill:
LocalLimitExecstays abovePkBlockFilterExec, continues pulling until enough rows survive, and cancels the child once the source contribution is full. Sources without a block filter keep the physical limit pushdown.Testing
cargo test -p lance --lib test_lsm_scan_limit_cargo test -p lance --lib dataset::mem_wal::scanner::cargo test -p lance --lib dataset::mem_wal::cargo fmt --allcargo clippy --all --tests --benches -- -D warningsCompatibility
No public API, file format, or dependency changes.
Classification
This fixes silently incomplete query results and should be labeled
critical-fix. The linked issue uses the requiredbug:title prefix, but GitHub rejected label mutation for the contributor account.