refactor(fts): separate document identity from row addresses#7863
refactor(fts): separate document identity from row addresses#7863Xuanwo wants to merge 13 commits into
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:
📝 WalkthroughWalkthroughThe inverted index replaces ChangesInverted index document and search refactor
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant InvertedIndex
participant PartitionDocuments
participant Wand
participant PostingListReader
participant TopKCollector
InvertedIndex->>PartitionDocuments: load partition statistics and document state
InvertedIndex->>Wand: execute typed BM25 search
Wand->>PostingListReader: load and validate postings
Wand->>PartitionDocuments: resolve lengths and visibility
Wand->>TopKCollector: collect document keys and scores
TopKCollector->>InvertedIndex: return typed candidates
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/lance-index/src/scalar/inverted/documents.rs (1)
631-657: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid copying the entire address column in the bulk branch.
address_column.values().to_vec()materializes allnum_docsaddresses into a newVeceven though the bulk branch only indexes the (small) top-kdoc_ids. On large partitions this is a needless full-column copy (e.g. ~80 MB for 10M docs) on the final resolution path.values()already yields a&[u64]you can index directly; only the point branch needs an owned copy, and it can zip the slice by reference.♻️ Proposed change to drop the full-column copy
- let addresses = address_column.values().to_vec(); + let addresses = address_column.values(); if use_bulk && addresses.len() != self.num_docs { return Err(corrupt_docs( &self.path, format!( "{ROW_ID} bulk read returned {} rows, expected {}", addresses.len(), self.num_docs ), )); } if !use_bulk && addresses.len() != unique.len() { return Err(corrupt_docs( &self.path, format!( "{ROW_ID} point read returned {} rows, expected {}", addresses.len(), unique.len() ), )); } if use_bulk { Ok(doc_ids .iter() .map(|doc_id| addresses[doc_id.as_usize()]) .collect()) } else { let by_doc_id = unique - .into_iter() - .zip(addresses) + .iter() + .copied() + .zip(addresses.iter().copied()) .collect::<std::collections::HashMap<_, _>>();🤖 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/documents.rs` around lines 631 - 657, Update the address resolution logic around the address_column values in the bulk and point branches to avoid eagerly calling to_vec() for all rows. Keep the bulk path borrowing and indexing the slice returned by values(), while making only the point-read path create any owned collection it requires and preserve its existing length validation and mapping 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.
Nitpick comments:
In `@rust/lance-index/src/scalar/inverted/documents.rs`:
- Around line 631-657: Update the address resolution logic around the
address_column values in the bulk and point branches to avoid eagerly calling
to_vec() for all rows. Keep the bulk path borrowing and indexing the slice
returned by values(), while making only the point-read path create any owned
collection it requires and preserve its existing length validation and mapping
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: b3522c7f-5768-40e6-a6e6-f90cbd5d68c7
📒 Files selected for processing (7)
rust/lance-index/src/scalar/inverted.rsrust/lance-index/src/scalar/inverted/builder.rsrust/lance-index/src/scalar/inverted/documents.rsrust/lance-index/src/scalar/inverted/index.rsrust/lance-index/src/scalar/inverted/lazy_docset.rsrust/lance-index/src/scalar/inverted/scorer.rsrust/lance-index/src/scalar/inverted/wand.rs
💤 Files with no reviewable changes (1)
- rust/lance-index/src/scalar/inverted/lazy_docset.rs
BubbleCal
left a comment
BubbleCal
left a comment
There was a problem hiding this comment.
I found two performance concerns in the new document-address path.
| if mask.is_select_all() && self.live_docs.is_none() { | ||
| return DocVisibility::All; | ||
| } | ||
| let selected = (0..self.addresses.len()) |
There was a problem hiding this comment.
Could we avoid rebuilding visibility by scanning every document for each filtered query? Once the projection is loaded, every non-trivial mask still walks all addresses here, and this happens before the WAND work is moved to spawn_cpu. A sparse allow-list over a large partition therefore becomes O(num_docs) on the async executor on every query. A lazy or cached address-to-DocId lookup, or another mask-driven path, would preserve selective-filter behavior.
| .iter() | ||
| .map(|(_, doc_id)| *doc_id) | ||
| .collect::<Vec<_>>(); | ||
| let resolved = documents.resolve_addresses(&doc_ids).await?; |
There was a problem hiding this comment.
Could these per-partition address reads be resolved concurrently? When the projection is cold, resolve_addresses opens and reads each docs file, but this loop awaits winning partitions one at a time. A top-k spread across several partitions therefore pays the sum of their object-store round trips. Buffering these futures up to store.io_parallelism() would keep the final projection bounded while preserving parallel I/O.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rust/lance-index/src/scalar/inverted/documents.rs (2)
1578-1611: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the error variant, not just the message.
This test verifies corruption is surfaced "not fallback", so it should confirm the returned error is actually the corruption variant (e.g. matching
Error::CorruptFile/corrupt_file) in addition to the message substring. Message-only checks let a misclassified error (e.g.Error::IO) pass silently. Same applies to the other corruption tests here (document_column_nulls_are_reported_as_corruption,required_document_columns_validate_name_and_type,doc_lengths_validate_shape_total_and_memory).As per coding guidelines: "Assert on both the error variant and the message content in tests; do not check only
is_err()."🤖 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/documents.rs` around lines 1578 - 1611, Update invalid_or_mismatched_stats_are_corruption_not_fallback and the related corruption tests document_column_nulls_are_reported_as_corruption, required_document_columns_validate_name_and_type, and doc_lengths_validate_shape_total_and_memory to match the returned errors against the corruption variant (such as Error::CorruptFile/corrupt_file), while retaining the existing message-content assertions. Ensure each test verifies both classification and descriptive error text rather than relying only on is_err or substring checks.Source: Coding guidelines
767-793: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the two identical DocId→address mapping blocks.
The resident-projection branch (Lines 767-779) and the remapper branch (Lines 780-793) differ only in how
projectionis obtained; the mapping closure and error message are copy-pasted. Collapsing them removes the risk of the two error paths drifting apart later.♻️ Proposed refactor
- if let Some(projection) = self.projection.get() { - return doc_ids - .iter() - .map(|&doc_id| { - projection.address(doc_id).ok_or_else(|| { - corrupt_docs( - &self.path, - format!("candidate DocId {} is not live", doc_id.get()), - ) - }) - }) - .collect(); - } - if self.remapper.is_some() { - let projection = self.projection().await?; - return doc_ids - .iter() - .map(|&doc_id| { - projection.address(doc_id).ok_or_else(|| { - corrupt_docs( - &self.path, - format!("candidate DocId {} is not live", doc_id.get()), - ) - }) - }) - .collect(); - } + let resident = self.projection.get().cloned(); + let projection = match resident { + Some(projection) => Some(projection), + None if self.remapper.is_some() => Some(self.projection().await?), + None => None, + }; + if let Some(projection) = projection { + return doc_ids + .iter() + .map(|&doc_id| { + projection.address(doc_id).ok_or_else(|| { + corrupt_docs( + &self.path, + format!("candidate DocId {} is not live", doc_id.get()), + ) + }) + }) + .collect(); + }🤖 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/documents.rs` around lines 767 - 793, Deduplicate the DocId-to-address mapping in the surrounding method: first obtain the projection from the resident cache or, when a remapper exists, await projection(), then run one shared doc_ids.iter() mapping block using projection.address and the existing corrupt_docs error. Preserve the current branch behavior and return handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rust/lance-index/src/scalar/inverted/documents.rs`:
- Around line 1578-1611: Update
invalid_or_mismatched_stats_are_corruption_not_fallback and the related
corruption tests document_column_nulls_are_reported_as_corruption,
required_document_columns_validate_name_and_type, and
doc_lengths_validate_shape_total_and_memory to match the returned errors against
the corruption variant (such as Error::CorruptFile/corrupt_file), while
retaining the existing message-content assertions. Ensure each test verifies
both classification and descriptive error text rather than relying only on
is_err or substring checks.
- Around line 767-793: Deduplicate the DocId-to-address mapping in the
surrounding method: first obtain the projection from the resident cache or, when
a remapper exists, await projection(), then run one shared doc_ids.iter()
mapping block using projection.address and the existing corrupt_docs error.
Preserve the current branch behavior and return handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 3e119c22-61a5-4eb4-8150-e9cd30637a36
📒 Files selected for processing (2)
rust/lance-index/src/scalar/inverted/documents.rsrust/lance-index/src/scalar/inverted/index.rs
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
github-actions
Bot
added
the
A-format
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/wand.rs (1)
2009-2022: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale
DocSet-specific wording innorm_k_cachedoc comment.The comment still says "when the DocSet scores quantized," but
self.documentsis now a genericWandDocumentsadapter (legacy or modern) — the whole point of this refactor is that quantized scoring is no longerDocSet-specific. Consider rewording to reference the adapter/partition documents generically so the comment doesn't mislead readers into thinking only the legacy backing type supports this path.📝 Proposed wording fix
- /// when the DocSet scores quantized (256-document-block partitions) and the scorer + /// when the partition's documents score quantized (256-document-block partitions) and the scorer🤖 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/wand.rs` around lines 2009 - 2022, Update the doc comment for WandDocuments::norm_k_cache to replace the DocSet-specific wording with a generic reference to the WandDocuments adapter or its quantized document partitions. Preserve the explanation of the 256-document-block scoring and the scorer factorization, while making clear the cache applies to any supported backing implementation.
🧹 Nitpick comments (2)
rust/lance-index/src/scalar/inverted/index.rs (1)
362-362: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the 64 MiB read-budget threshold.
MAX_CONCURRENT_ADDRESS_READ_BYTESgates deferred address-read fan-out but carries no comment on what the value represents or why 64 MiB was chosen.📝 Suggested doc comment
+/// Upper bound on the total bytes of concurrent deferred address reads. Caps +/// `address_read_concurrency` so a top-k spread across many partitions keeps +/// its projection I/O bounded (~64 MiB) instead of scaling with partition count. const MAX_CONCURRENT_ADDRESS_READ_BYTES: usize = 64 * 1024 * 1024;As per coding guidelines: "Add doc comments to magic constants, thresholds, and non-obvious transformation functions, explaining what the value represents and why it was chosen."
🤖 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` at line 362, Document MAX_CONCURRENT_ADDRESS_READ_BYTES with a comment explaining that it limits deferred address-read fan-out to a 64 MiB concurrent read budget and why this threshold was selected. Keep the constant value and surrounding behavior unchanged.Source: Coding guidelines
rust/lance-index/src/scalar/inverted/wand.rs (1)
171-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
row_idfield name no longer describes what it holds.
to_candidate(doc.row_id)feeds the heap key'srow_idfield into a generic candidate mapper — for the modern adapter this is aDocId, not a row id. Keeping the field namedrow_idafter generalizinginto_candidatesoverCis imprecise and could mislead future readers into assuming it's always a physical row identifier.Please confirm the underlying
ScoredDoc/heap-key struct definition (not shown in this diff) and consider renamingrow_idto something likekey/document_keyto match the now-generic semantics.🤖 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/wand.rs` around lines 171 - 192, Confirm the underlying ScoredDoc/heap-key definition and rename its generic identifier field from row_id to an accurate name such as document_key, updating all constructors, accesses, and heap-related uses including into_candidates. Preserve the existing value passed to to_candidate while removing assumptions that it is always a physical row identifier.
🤖 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/wand.rs`:
- Around line 2009-2022: Update the doc comment for WandDocuments::norm_k_cache
to replace the DocSet-specific wording with a generic reference to the
WandDocuments adapter or its quantized document partitions. Preserve the
explanation of the 256-document-block scoring and the scorer factorization,
while making clear the cache applies to any supported backing implementation.
---
Nitpick comments:
In `@rust/lance-index/src/scalar/inverted/index.rs`:
- Line 362: Document MAX_CONCURRENT_ADDRESS_READ_BYTES with a comment explaining
that it limits deferred address-read fan-out to a 64 MiB concurrent read budget
and why this threshold was selected. Keep the constant value and surrounding
behavior unchanged.
In `@rust/lance-index/src/scalar/inverted/wand.rs`:
- Around line 171-192: Confirm the underlying ScoredDoc/heap-key definition and
rename its generic identifier field from row_id to an accurate name such as
document_key, updating all constructors, accesses, and heap-related uses
including into_candidates. Preserve the existing value passed to to_candidate
while removing assumptions that it is always a physical row identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: ebc6977c-1d10-4077-a9a5-f156a8d96e2f
📒 Files selected for processing (6)
docs/src/format/index/scalar/fts.mdrust/lance-index/src/scalar/inverted.rsrust/lance-index/src/scalar/inverted/builder.rsrust/lance-index/src/scalar/inverted/documents.rsrust/lance-index/src/scalar/inverted/index.rsrust/lance-index/src/scalar/inverted/wand.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 (1)
rust/lance-index/src/scalar/inverted/index.rs (1)
10892-10906: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBuild a modern fixture for modern-document tests.
load_global_scoring_test_indexunconditionally writes V1 partitions, sopartition.docs.modern().unwrap()panics. Parameterize the fixture format; use V2/V3 for these modern-only tests and retain V1 for the legacy regression.
rust/lance-index/src/scalar/inverted/index.rs#L10892-L10906: pass a modern-layout fixture before accessingpartition.docs.modern().rust/lance-index/src/scalar/inverted/index.rs#L10793-L10795: create the prewarm/resident-projection fixture with the same modern layout.🤖 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 10892 - 10906, Parameterize load_global_scoring_test_index by partition format, preserving V1 for the legacy regression while using V2/V3 for modern-only tests. Update the modern-document test at rust/lance-index/src/scalar/inverted/index.rs:10892-10906 and the prewarm/resident-projection fixture at rust/lance-index/src/scalar/inverted/index.rs:10793-10795 to request the modern layout before calling partition.docs.modern().
🤖 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 10892-10906: Parameterize load_global_scoring_test_index by
partition format, preserving V1 for the legacy regression while using V2/V3 for
modern-only tests. Update the modern-document test at
rust/lance-index/src/scalar/inverted/index.rs:10892-10906 and the
prewarm/resident-projection fixture at
rust/lance-index/src/scalar/inverted/index.rs:10793-10795 to request the modern
layout before calling partition.docs.modern().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: e864dd8a-25dc-4ccd-b2e9-6ff35eacc7ee
📒 Files selected for processing (1)
rust/lance-index/src/scalar/inverted/index.rs
…ccess-refactor-pr
2 participants
Context
Modern FTS postings use dense per-partition document IDs, while the existing
LazyDocSetalso owns physical row addresses, document lengths, visibility, and final projection. This mixes identity domains with different lifetimes and makes query-time ownership difficult to reason about.This refactor keeps dense
DocIdvalues through modern WAND scoring and filtering, loads document lengths and address projection independently, and resolves physical row addresses only for the final top-k. Legacy single-file indexes continue through the existing row-address path. We no longer write new legacy indexes, so their observable behavior and on-disk handling remain unchanged, and themetadata.lanceformat is unchanged.The fully prewarmed path additionally publishes query-ready document projections and validated postings, bypasses already-resident async singleflight futures, specializes unfiltered scoring, and skips redundant corpus-stat synchronization. Cold queries retain bounded, concurrent top-k address resolution.
Performance
The benchmark used the same 100M-row S3 corpus, 29 FTS segments, and 10-query top-10 panel on an
r7i.12xlargeinus-east-1. Results matched, and every fully prewarmed measured query reported zero object-store reads and writes. Values are candidate deltas versusmain; higher QPS and lower latency are better.Without explicit index prewarm
Each variant ran twice in reversed order for 5,000 measured queries per workload.
After
Dataset::prewarm_indexThe standard FTS prewarm API was used with a 192 GiB index cache. Each variant ran twice in reversed order for 20,000 measured queries per workload. Benchmark candidate
a03f98b36604adbdd1720c03256ca25eba8a97dcis production-code equivalent to PR head964bfe243e64de2382a5801389f7d5529a527806; baselinebb819936ca8090c602164e45907aef5db1c26a56is based onmainaea6ded4822780812703605a88554addb839c9e3.Verification
cargo test -p lance-index --lib(876 passed, 0 failed, 2 ignored)cargo clippy --all --tests --benches -- -D warningscargo fmt --all -- --checkBenchmark-only source and scripts are kept on separate branches and are not part of this PR.