perf(fts): resolve deferred row_ids via the cached whole ROW_ID column#7897
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:
📝 WalkthroughWalkthrough
ChangesDeferred row-id resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-index/src/scalar/inverted/index.rs`:
- Around line 8510-8612: Strengthen
test_bm25_search_many_partitions_resolves_exact_row_ids by instrumenting
docs-file ROW_ID reads and tracking per-partition access. Assert the first
bm25_search performs exactly one full-column read for each matching partition,
then run the second query and assert it performs no additional ROW_ID reads
while preserving the existing result assertions.
🪄 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: b6c37e72-e010-4bb5-a3b5-c99391c5573f
📒 Files selected for processing (2)
rust/lance-index/src/scalar/inverted/index.rsrust/lance-index/src/scalar/inverted/lazy_docset.rs
5d1d6b1 to
84b5cad
Compare
|
summary of the change:
Memory cost: 8 bytes per doc. For a 100M-doc index that is ~800 MB total (~2.5 MB per 312k-doc partition), and it is loaded lazily — only for partitions that actually produce search candidates. There are some future improvements
|
resolve_row_ids' slow path opened a fresh docs-file reader and issued scattered single-row read_ranges on every call, caching nothing - so any partition whose DocSet is not materialized pays a file open plus random single-row reads per query, forever. Under concurrency these per-row reads funnel through the shared scheduler/cache locks and serialize the whole search path. Use the existing row_ids_column() loader instead: one sequential read of the ROW_ID column, cached in the row_ids_col OnceCell, after which every resolve for the partition is an in-memory lookup. Costs ~8 bytes per doc resident (~2.5MB per 312k-doc partition), the same lazily-populated accounting semantics as num_tokens_col. Measured on a 320-core node, 100M-doc 42-language dataset, tier-100-200 5-term match_any, k=100, 960GiB index cache, single-node bench: - c1: resolve stage 237ms -> 0.1ms; qps 2.5 -> 3.2; mean 406 -> 316ms - c16, 300s: qps 4.1 -> 107.6 (26x), mean 3.9s -> 149ms Adds a many-partition regression test asserting exact row_id resolution.
84b5cad to
7e2acc6
Compare
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-index/src/scalar/inverted/index.rs`:
- Around line 8523-8525: Update the expected_row_ids initialization in the
matching-partition loop to use Vec::with_capacity with the known upper bound of
NUM_MATCHING_PARTITIONS multiplied by three, while preserving the existing push
and iteration 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: 4294d2f0-8f7f-40cb-9d9e-9309bedb9b34
📒 Files selected for processing (2)
rust/lance-index/src/scalar/inverted/index.rsrust/lance-index/src/scalar/inverted/lazy_docset.rs
…n test Address review: the exact-row-ids test would also pass with the previous scattered-read implementation. Wrap the index store with a docs-file ROW_ID read counter and assert: - one full-column ROW_ID read per resolving partition on the first query - zero scattered single-row reads - zero additional docs-file ROW_ID reads on a subsequent query - the non-matching partition never reads its ROW_ID column Also preallocate the expected_row_ids vector to its known bound.
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-index/src/scalar/inverted/index.rs`:
- Around line 8514-8518: Add #[cfg_attr(coverage, coverage(off))] to
DocsRowIdReadCounter and every associated test-only I/O wrapper struct and impl
block in this section, covering the ranges indicated by the comment. Do not
alter production code or the wrappers’ 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: 8c4fa95e-c41a-487b-91cd-0c6f7b8bdb54
📒 Files selected for processing (1)
rust/lance-index/src/scalar/inverted/index.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance-index/src/scalar/inverted/index.rs (1)
8551-8559: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winCount only true full-column reads.
read_rangeincrementsfull_column_readsfor any projection containingROW_ID, without checkingrange. A regression to partialROW_IDreads could therefore still satisfyfull_column_reads == 40, so the test does not fully verify the whole-column caching contract. Track partial reads separately and assert they remain zero; ideally also key counts by docs file to validate the non-matching partition guarantee.Also applies to: 8773-8785
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-index/src/scalar/inverted/index.rs` around lines 8551 - 8559, Update the read-range instrumentation around read_range so full_column_reads increments only when the requested range represents a true full-column read, not merely when projection contains ROW_ID. Add separate tracking for partial reads and assert they remain zero, preferably keyed by docs file, and apply the same correction to the corresponding logic at the second referenced location.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rust/lance-index/src/scalar/inverted/index.rs`:
- Around line 8551-8559: Update the read-range instrumentation around read_range
so full_column_reads increments only when the requested range represents a true
full-column read, not merely when projection contains ROW_ID. Add separate
tracking for partial reads and assert they remain zero, preferably keyed by docs
file, and apply the same correction to the corresponding logic at the second
referenced location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 276df044-e7aa-43ea-ba1c-7b3caa077762
📒 Files selected for processing (1)
rust/lance-index/src/scalar/inverted/index.rs
|
@LuQQiu I think the repeated-resolution problem is real, and the whole-column result is convincing for the benchmarked serving shape. My concern is the ownership and accounting contract. The scalar index is weighed when it is inserted, before any row-id column has been initialized. Moka keeps that insertion-time weight, so filling the Could we explore either of these directions?
Would the benchmark harness allow a quick A/B of the late-resolution approach? If that does not recover enough throughput, does the partition-scoped cache entry look feasible for this PR? |
…-id columns as weighed entries - Carry (partition slot, doc_id) through the global top-k heap and resolve row ids only for the final survivors: at most limit lookups per query instead of up to partitions * limit, and only partitions holding survivors load their ROW_ID column. - Store each partition's ROW_ID column as its own index-cache entry (DocRowIdsKey -> CachedDocRowIds) loaded through WeakLanceCache::get_or_insert_with_key: weighed at insert time so the cache accounts for the memory, independently evictable, and single-flight deduplicated on concurrent loads. The lazily populated OnceCell copy is removed.
The DocRowIdsKey index-cache entry is now the sole owner of a modern partition's doc_id->row_id column. ensure_loaded (and therefore prewarm) loads the column into the entry and keeps a resident DocSet of num_tokens + inv only; the masked non-flat wand path borrows the entry per query via a zero-copy view; top-k resolution always reads through the entry so the LRU sees real usage. Evicting the entry now actually frees the memory, and reloads are single-flight. Flat-path shape selection shares wand's own should_flat_search predicate so the two decisions cannot drift, and flat_search asserts it got a reverse-lookup-capable DocSet. Legacy and frag-reuse partitions keep their rewritten owned copies and do not populate the raw-column entry. into_builder copies an owned DocSet out of the entry instead of materializing one on the partition.
|
@Xuanwo Thanks for the careful review — we ended up doing both of your directions, plus one step further. 1. Late resolution (resolve after the global top-k merge) — adopted. The global heap now carries 2. Separate weighed cache entry — adopted, and we A/B'd the "without introducing whole-column residency" hypothesis on the 100M-doc bench: late resolution with scattered per-survivor reads (no column residency) caps at 19–24 qps vs ~108 blended / ~137 warm qps with the cached whole column — per-query file opens + random reads dominate regardless of how few rows are read. So the whole-column load stays, but as a Follow-up commit (985d7d3): the entry is now the column's only home. |
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
Thank you for working on this!
2 participants
Problem
DeferredDocSet::resolve_row_ids's slow path opens a fresh docs-file reader and issues scattered single-rowread_ranges(oned..d+1range per candidate doc) on every call, caching nothing. Any partition whose DocSet is not materialized (e.g. no prewarm, select-all queries wheredocs_for_wandloads onlynum_tokens) pays file-open + random single-row reads per query, forever.Under concurrency these per-row reads funnel through the shared scheduler/cache locks and effectively serialize the whole search path: in our benchmark the process showed hundreds of threads parked in
futex_waitwhile CPU sat below 50%, and throughput was capped ~4 qps regardless of concurrency or warmup.Fix
Two changes, addressing the review feedback on both work volume and memory accounting:
Resolve row ids after the global top-k merge. The global heap carries
(partition slot, doc_id)for deferred candidates; only the final survivors are resolved — at mostlimitlookups per query instead of up topartitions × limit, and only partitions holding survivors load their ROW_ID column at all.Store each partition's ROW_ID column as its own weighed index-cache entry (
DocRowIdsKey → CachedDocRowIds), loaded throughWeakLanceCache::get_or_insert_with_key: one sequential column read on first use, weighed at insert time so the cache accounts for the memory, independently evictable under pressure, and single-flight deduplicated on concurrent loads. The lazily populatedrow_ids_colOnceCell (invisible to cache accounting) is removed; no strong copy is retained on the index object.Memory cost when resident: ~8 bytes/doc per partition (~2.5 MB per 312k-doc partition; 800 MB for a 100M-doc index), now visible to
index_cache_size_bytescapacity accounting.Benchmarks
Single-node bench (320-core host, 100M-doc 42-language dataset on local NVMe, tier-100-200 common words, 5-term
match_any, k=100, 960 GiB index cache, distinct query words per query):The last row A/Bs the "late resolution without whole-column residency" hypothesis: resolving only the ≤k survivors via targeted scattered reads still caps at 19 qps — per-query file opens and random single-row reads dominate regardless of how few rows are read — so the cached whole-column load is what recovers throughput, and late resolution keeps its benefits on the work/memory side (fewer lookups, fewer partitions loading columns).
An ablation that skipped resolution entirely (returning raw doc ids) measured ~155 qps blended on the same run shape, so this change recovers most of the available headroom while returning correct row_ids.
Tests
test_bm25_search_many_partitions_resolves_exact_row_ids: 41 partitions (40 matching + 1 whose only token does not match); asserts the exact row_id set, one full-column ROW_ID read per resolving partition, zero scattered single-row reads, zero re-reads on subsequent queries, and no ROW_ID read for the non-matching partition.test_bm25_search_resolves_only_topk_survivors_and_accounts_cache: with k=3 over 40 matching partitions, only the partitions holding final survivors (≤3) load their ROW_ID column, and each loaded column is present in the index cache as its ownDocRowIdsentry.Follow-up in this PR: the cache entry becomes the column's only home
The first version of change 2 still had
ensure_loaded()(prewarm, masked wand) materialize a fullDocSetwith its own ownedrow_idscopy — so a prewarmed partition stored the column twice, and the second copy was invisible to accounting and un-evictable. The follow-up commit removes that copy entirely:DocSet.row_idsbecomesOwned(Vec<u64>) | Shared(ScalarBuffer<u64>)(mirroring the existingNumTokensidiom).Sharedis a zero-copy view of the cache entry and reports 0 toDeepSizeOf(the entry is weighed at insert).ensure_loaded()— and therefore prewarm — loads the column into theDocRowIdsKeyentry and keeps a residentDocSetof num_tokens + the derived reverse-lookupinvonly. Nothing long-lived references the entry's allocation, so evicting the entry actually frees the memory; the next borrower reloads it single-flight (index files are immutable, so a reload is always equivalent). This is the same prewarm/cache contract posting lists already have.mask.selected(row_id)inside the scoring loop) borrows the entry per query via aSharedview that drops when the query finishes.invinstead: shape selection now uses the exactshould_flat_searchpredicateWand::searchitself uses, so the two decisions cannot drift (a masked query scored without row_ids would silently skip mask filtering), plus a debug assert inflat_search.into_buildercopies an ownedDocSetout of the entry and never stashes it.Additional tests:
test_prewarm_fills_row_ids_cache_entry_without_a_second_copy(prewarm fills every partition's entry, the resident DocSet has no row_ids, and a warm query resolves with zero further docs-file reads) andtest_row_ids_resolution_reloads_after_eviction(with a no-retention cache — the always-evicted worst case — every query reloads the column and stays correct, proving nothing depends on a pinned copy).