perf(index): bulk conjunction path for FTS AND and phrase queries#7624
Conversation
4732787 to
fec0a88
Compare
fc1af4b to
22e3522
Compare
## Feature Linear: [OSS-1344](https://linear.app/lancedb/issue/OSS-1344/make-fts-index-block-size-configurable) ### What is the new feature? FTS inverted index creation now accepts a `block_size` parameter for compressed posting blocks. Supported values are `128` and `256`. ### Why do we need this feature? The posting block size was previously fixed at `128`, which made the block-max granularity impossible to tune for different datasets and query profiles. ### How does it work? - Adds `block_size` to `InvertedIndexParams`, protobuf details, posting-list schema metadata, and cache headers. - Uses `128` as the default for newly created indexes. - Treats older serialized params, schema metadata, and cache entries that omit `block_size` as legacy `128`. - Rejects unsupported values, including `512`, with a clear validation error. - Uses Lance-owned `BitPacker4x` for physical 128-value posting blocks and `BitPacker8x` for physical 256-value posting blocks. - Marks `block_size=256` as experimental in public API docs because it may introduce breaking changes. - Keeps position-stream packing on the legacy 128-value block format. - Keeps downgrade compatibility tests on explicit legacy `block_size=128`, since older wheels cannot read current-created physical 256 FTS posting blocks. - Threads the configured block size through FTS build, read, iterator, WAND, cache, and MemWAL flush paths. - Exposes the parameter in Python and Java FTS index creation APIs, with docs and focused tests. ## Validation - `cargo fmt --all` - `cargo fmt --all --check` - `git diff --check` - `CARGO_TARGET_DIR=/tmp/lance-target-a479-no512 cargo test -p lance-index block_size -- --nocapture` - `CARGO_TARGET_DIR=/tmp/lance-target-a479-no512 cargo clippy -p lance-index --tests -- -D warnings` - `uv run make build` from `python/` - `uv run pytest python/tests/test_scalar_index.py::test_create_scalar_index_fts_block_size` from `python/` - `uv run ruff format --check python/tests/test_scalar_index.py python/lance/dataset.py` from `python/` - `uv run ruff check python/tests/test_scalar_index.py python/lance/dataset.py` from `python/` - `CARGO_TARGET_DIR=/tmp/lance-target-a479-merge-main cargo test -p lance-index block_size -- --nocapture` - `CARGO_TARGET_DIR=/tmp/lance-target-a479-merge-main cargo test -p lance-index test_256_posting_block_uses_single_physical_bitpack_chunk -- --nocapture` - `CARGO_TARGET_DIR=/tmp/lance-target-a479-merge-main cargo test -p lance-bitpacking` - `CARGO_TARGET_DIR=/tmp/lance-target-a479-merge-main cargo clippy -p lance-bitpacking -p lance-index --tests -- -D warnings` - `uv run ruff format --check python/tests/compat/test_scalar_indices.py` from `python/` - `uv run ruff check python/tests/compat/test_scalar_indices.py` from `python/` - `uv run pytest --run-compat -vvv -s python/tests/compat/test_scalar_indices.py::test_FtsIndex_downgrade --durations=30` from `python/` - `CARGO_TARGET_DIR=/tmp/lance-a479-target cargo test -p lance-index test_new_training_request_defaults_missing_block_size_to_128` - `CARGO_TARGET_DIR=/tmp/lance-a479-target cargo test -p lance-index block_size` - `uv run ruff format --check python/lance/dataset.py` from `python/` - `uv run ruff check python/lance/dataset.py` from `python/` Not run locally: Java focused test / spotless check, because this machine has no Java Runtime installed (`Unable to locate a Java Runtime`). --- ## Update: all V3 breaking changes consolidated here Per review direction, every breaking change for the 256-doc block format now lands in this single PR (the follow-up stack #7602/#7603/#7604/#7624/#7625/#7629 carries none). On top of the configurable block size and PFOR frequency encoding, this PR now also includes: - **Quantized doc-length scoring (Lucene norm semantics), 256-doc blocks only.** BM25 doc lengths are quantized to a SmallFloat-style byte code (4 mantissa bits: 0-7 exact, <= 6.25% relative error, decode = bucket floor). The byte-norm slab bakes lazily per loaded DocSet and quarters the doc-length bytes scoring pulls through the cache (200M docs: 800MB -> 200MB). 128-block indexes keep exact-length scoring bit-for-bit. Measured top-k overlap vs exact scoring on the (score-clustered, synthetic) mmlb corpus: 98.1% mean for phrase, 89.7% for 3-word AND; corpora with more score spread shift less. - **256-doc posting blocks drop the leading block-max-score f32** (~1.5G on a 200M-doc index; 131G -> 130G). Block layout: `[first_doc u32][doc num_bits u8][docs][pfor freqs]`; `posting_block_score_prefix_len(block_size)` keys every reader/writer. The impact skip data from the stacked #7602 supplies a tighter per-block bound; until it lands, 256-block block-max pruning falls back to the (valid, looser) list-level max score. **BREAKING:** 256-doc-block (v3) indexes must be rebuilt; v3 is unreleased so no migration is provided. BM25 scores on v3 differ from exact-length BM25 by the norm quantization, matching Lucene's norm semantics. The format discussion #7606 documents the final layout and scoring semantics. Additional validation for this update: bulk-vs-classic A/B under quantized scoring is score-identical (both paths quantize identically); the full stack's warm benchmarks vs Lucene 10.4 on mmlb-200m: OR k10 0.0249s/318qps and OR k100 0.0467s/170qps (both ahead of Lucene sliced), AND k10 0.0443s (1.29x), AND k100 0.0883s (1.94x). --- ## Standalone results vs main (per-branch-tip wheels) **Legacy (128) read-path parity.** Threading a runtime `block_size` through `PostingIterator` initially replaced the compile-time `BLOCK_SIZE` division (a shift) with real `div` instructions in the `doc()`/`next()` hot loops, measured as +11-14% on 3-word OR against the 200M legacy index (`PostingIterator::next` grew from 16.5% to 23.6% of the profile). Block sizes are validated powers of two, so the iterator now derives block indices with `trailing_zeros` shifts and masks; after that fix the legacy path is at parity with main: 3-word OR k10 0.131s (main 0.132-0.134s), k100 0.255-0.256s (main 0.256-0.257s), single-term 0.025s (main 0.027s) across 3 warm passes on the 200M legacy index, 400G cache. **block_size=256 index size** (5M-doc controlled build, same wheel, `with_position=false`): postings shrink **3.33 GiB → 2.62 GiB (−21%)** from PFOR frequencies + no per-block max-score prefix + half the block headers. Index build 192s → 210s (+10%, PFOR encode cost). Top-10 overlap vs the 128 exact-length scoring on 10 3-word OR queries: 95% mean (5/10 identical sets; the rest differ by 1-2 near-tie docs, from the quantized-norm scoring). **Query wins for 256 land in the stacked PRs.** A 256 index without impact skip data prunes on the (valid, looser) list-level max and is *slower* than 128 — e.g. classic AND k10 0.547s until #7602's impacts restore block-granular bounds (0.115s), and #7603/#7604/#7624/#7625/#7629 take the same index to 0.025s OR k10 / 0.045s AND k10. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added configurable FTS posting `block_size` (128/256) to scalar index creation, including updated examples and APIs. * Enabled FTS format version 3 (`v3`) for `block_size=256`, with quantized doc-length scoring for v3. * **Bug Fixes** * Enforced `block_size`/`format_version` compatibility (invalid combinations now error). * Persisted and restored FTS metadata for format version and posting block size, with legacy indexes defaulting to `128`. * **Documentation** * Updated full-text-search and quickstart guides and parameter docs for `block_size`, defaults, accepted values, and the experimental `256`/`v3` behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Yang Cen <yang@lancedb.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
22e3522 to
6122151
Compare
📝 WalkthroughWalkthroughPackedDelta positions can now be decoded per document range. Compressed posting iteration uses this seek state for candidate phrase checks, and ChangesCompressed conjunction search
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WandSearch
participant and_bulk_search
participant PostingIterator
participant seek_packed_doc_positions
participant check_exact_positions_bulk
WandSearch->>and_bulk_search: dispatch compressed AND or phrase query
and_bulk_search->>PostingIterator: intersect decompressed block slices
PostingIterator->>seek_packed_doc_positions: decode candidate positions
and_bulk_search->>check_exact_positions_bulk: check phrase alignment
check_exact_positions_bulk-->>and_bulk_search: return phrase match
and_bulk_search-->>WandSearch: emit scored top-k candidates
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
6122151 to
6c76a45
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Xuanwo
left a comment
There was a problem hiding this comment.
I found two issues that need to be addressed before this is safe to merge.
| && !self.lead.is_empty() | ||
| && self.lead.iter().all(|posting| posting.is_compressed()) | ||
| { | ||
| return self.and_bulk_search(params, mask, metrics); |
There was a problem hiding this comment.
This makes the generic bulk path the default for every effective clause count. The stacked #7625 reports 4/5-clause regressions against the classic path and consequently narrows auto to 2/3 clauses, so this PR would knowingly make regressive query shapes the default until the follow-up lands.
| ) | ||
| .expect("shared position stream doc decoding should succeed"); | ||
| Some(PositionCursor::new( | ||
| PositionValues::Borrowed(&compressed.position_doc_scratch[..]), |
There was a problem hiding this comment.
position_cursor(&self) now returns a slice borrowed from position_doc_scratch, but another safe call on the same iterator clears and refills that Vec while the first cursor can remain live. This permits safe code to create overlapping shared and mutable access, causing undefined behavior even though the current callers happen to invoke it only once per iterator.
AND and phrase queries previously leapfrogged doc-at-a-time through boxed PostingIterator::next calls (~61% of the AND profile) and phrase checks decoded a whole 256-doc position block per candidate (~39% of the phrase profile). - and_bulk_search: block-max window pruning plus a k-pointer merge over decompressed block slices; per-candidate advance cost drops to a few loads. Results are identical to the classic loop (LANCE_FTS_BULK_AND=0 opts out). Phrase queries ride the same path. - seek_packed_doc_positions: PackedDelta full groups are self-describing ([num_bits u8][16*num_bits bytes]), so group offsets are recovered by hopping headers; decode only the 1-2 groups overlapping the candidate doc's delta range, with a lazily-built group index, memoized unpacked group, and a decoded-tail cache per block. - check_exact_positions_bulk: allocation-free slop=0 alignment check on the decoded scratch slices for parked lead clauses. Warm mmlb benchmarks, 8 concurrent queries: AND\@200M k10 0.114->0.060s, k100 0.240->0.118s; phrase\@50m 3-word k10 0.335->0.210s, 2-word k10 0.098->0.042s. All steps verified score-identical to the classic path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6c76a45 to
6311353
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/encoding.rs`:
- Around line 787-818: In
rust/lance-index/src/scalar/inverted/encoding.rs:787-818, update the position
decoder around the group-offset construction and BitPacker4x decompression to
bounds-check each header and packed payload before indexing or slicing,
returning contextual corruption errors instead of panicking. In
rust/lance-index/src/scalar/inverted/wand.rs:743-771, propagate decoder Results
through the cursor and phrase-check APIs and remove the expect-based failure
handling so malformed position data remains recoverable end to end.
In `@rust/lance-index/src/scalar/inverted/wand.rs`:
- Around line 3947-3970: Extend the existing phrase-position parity tests around
test_packed_position_cursors_use_independent_scratch to cover both PackedDelta
and VarintDocDelta storage, including cursor recycling behavior. Add a nonzero
phrase_slop bulk phrase case to the existing tests spanning the related
phrase-position branches, reusing and parameterizing the current test flow
rather than duplicating test logic.
🪄 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: af06a6e0-b84a-4299-a14f-1f0f563c8e9b
📒 Files selected for processing (2)
rust/lance-index/src/scalar/inverted/encoding.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 (2)
rust/lance-index/src/scalar/inverted/wand.rs (2)
2551-2649: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument the raw-pointer window invariant with inline SAFETY comments.
WindowList.docs/freqsare raw pointers intoCompressedState.doc_ids/freqs, kept valid for the whole window only because the block cursor doesn't move and position decoding touches separate fields. This invariant is explained once in the function doc comment but not at the individualunsafe { *win.docs.add(...) }deref sites; a future change that reallocatesdoc_ids/freqsmid-window (e.g. during position handling) would silently produce UB with no compiler signal.Add short
// SAFETY:comments at the deref sites (or at minimum wherewinsis populated) referencing the block-immutability invariant, so future edits touchingCompressedStatedon't accidentally break it.🤖 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 2551 - 2649, Add concise `// SAFETY:` comments at the raw-pointer dereference sites using `WindowList.docs` and `freqs`, or when populating `wins`, documenting that these pointers reference the current `CompressedState` block and remain valid because block cursors do not move and position decoding uses separate fields. Anchor the comments to the `WindowList` construction and each `unsafe` dereference without changing behavior.Source: Coding guidelines
3413-3417: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
MAX_INLINE_CLAUSESthreshold and fallback rationale.The
16-clause cap and thecheck_exact_positions()fallback aren't explained inline. As per coding guidelines, magic constants/thresholds and guard/fallback paths should carry a short comment on what the value represents and why it was chosen, and when the fallback triggers.✏️ Suggested doc comment
+ // Cap chosen to keep the cursor array on the stack; queries with more + // clauses are rare enough that falling back to the allocating path + // (`check_exact_positions`) is an acceptable trade-off. const MAX_INLINE_CLAUSES: usize = 16; let num_clauses = self.lead.len(); if num_clauses > MAX_INLINE_CLAUSES { + // More clauses than fit inline: fall back to the classic checker. return self.check_exact_positions(); }🤖 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 3413 - 3417, The check_exact_positions_bulk method needs inline documentation for its MAX_INLINE_CLAUSES threshold and fallback path. Add a brief comment explaining that 16 is the maximum number of clauses handled by the bulk inline path, why that limit exists, and that exceeding it routes to check_exact_positions().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-index/src/scalar/inverted/wand.rs`:
- Around line 2551-2649: Add concise `// SAFETY:` comments at the raw-pointer
dereference sites using `WindowList.docs` and `freqs`, or when populating
`wins`, documenting that these pointers reference the current `CompressedState`
block and remain valid because block cursors do not move and position decoding
uses separate fields. Anchor the comments to the `WindowList` construction and
each `unsafe` dereference without changing behavior.
- Around line 3413-3417: The check_exact_positions_bulk method needs inline
documentation for its MAX_INLINE_CLAUSES threshold and fallback path. Add a
brief comment explaining that 16 is the maximum number of clauses handled by the
bulk inline path, why that limit exists, and that exceeding it routes to
check_exact_positions().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d74c817f-3922-4796-9ae1-523edc245d6f
📒 Files selected for processing (2)
rust/lance-index/src/scalar/inverted/encoding.rsrust/lance-index/src/scalar/inverted/wand.rs
Built on the MAXSCORE work merged in #7603; this is the next PR in the Lucene-parity series. This PR now includes the 2/3-clause SIMD and frequency-bound work previously stacked as #7625.
Performance issue
Top-k AND and phrase queries previously leapfrogged doc-at-a-time through boxed
PostingIterator::nextcalls. Phrase checks additionally decoded a whole 256-doc position block per candidate and allocated cursor vectors per candidate.What changed
Runtime selection
LANCE_FTS_BULK_ANDaccepts:auto(default): use bulk for 2/3 effective posting clauses and classic for all other widths.onor legacy1: force bulk for every eligible compressed AND/phrase query; 4+ clauses use the generic kernel.offor legacy0: always use the classic conjunction loop.The clause count is measured after tokenization, deduplication, and expansion. Invalid values warn and fall back to
auto.The 4/5-clause experiments did not show a consistent specialized-kernel win, and generic bulk regressed against classic in several tiers. Therefore no 4/5-clause specialized kernels are retained and
autoremains limited to 2/3 clauses.Measured performance
Per-branch-tip wheels, 1000 queries × 8 concurrent. AND used the warm, fully prewarmed 200M-doc V3/256 index; phrase used the 50M-doc V3/256 positions index.
The AWS 4/5-clause follow-up used an
r7i.24xlarge, the 200M English-only MMLB dataset, a V3/256 index with 338 partitions, k=100, c32, a 600 GB cache, and synchronous full prewarm.Compatibility
This is a query-time implementation change. It does not change the FTS index format or index version.
Verification
cargo test -p lance-index,cargo clippy --all --tests --benches -- -D warnings, andcargo fmt --all -- --checkpassed across the reviewed stack.Summary by CodeRabbit
LANCE_FTS_BULK_AND(auto by default).