feat(fts)!: add configurable posting block size#7466
Conversation
|
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
A-python
BubbleCal
force-pushed
the
yang/oss-1344-make-fts-index-block-size-configurable
branch
from
dd4ac88 to
d9f0acb
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
BubbleCal
force-pushed
the
yang/oss-1344-make-fts-index-block-size-configurable
branch
from
d9f0acb to
23e4810
Compare
BubbleCal
force-pushed
the
yang/oss-1344-make-fts-index-block-size-configurable
branch
from
23e4810 to
059ae90
Compare
…s-index-block-size-configurable
|
@claude reivew |
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
I think this needs a compatibility boundary before merge.
-
block_size=256changes the persisted posting-block layout, but the stored index version still looks like the existing FTS format. Older readers will ignore the new metadata/details field and try to decode the blocks as legacy 128-docBitPacker4xblocks. That can fail open as wrong FTS results or decode panics instead of cleanly ignoring the index. We should either bump the FTS/index version for non-legacy block sizes or reject writing 256 until older readers can be gated out. -
Legacy segments with no
block_sizeand newly written default-128 segments withblock_size=128are semantically identical, but the multi-segment details check compares the raw protobuf values. Mixed old/new default-128 segments can be rejected as inconsistent. The comparison should canonicalize missingblock_sizeto 128 before comparing.
bumped block_size=256 to version=3 |
|
Thanks for pushing the format-version boundary for I think there is still one blocking invariant gap in the MemWAL FTS path.
Later, MemWAL flush calls Could we validate the format/block-size invariant at the MemWAL config/metadata boundary and add a regression test for |
|
Format-change discussion / PMC vote thread for the V3 (256-doc block + patched-FOR frequency) format introduced here: #7606 |
… for 256-doc blocks Folds the v3 breaking changes into this PR so the format/scoring break lands in one place; 128-block indexes are untouched on disk and keep exact scoring bit-for-bit. - BM25 doc lengths for 256-doc-block partitions are quantized to a Lucene SmallFloat-style byte code (4 mantissa bits, exact below 8, <= 6.25% relative error, decode = bucket floor). The byte-norm slab bakes lazily per loaded DocSet, quartering the doc-length bytes the scoring path pulls through the cache (200M docs: 800MB -> 200MB). - 256-doc posting blocks drop the leading block-max-score f32. The impact skip data introduced by the stacked follow-up supplies a tighter per-block bound; until it lands, block-level pruning for 256 falls back to the list-level max score, which is a valid (looser) bound. Block layout: [first_doc u32][doc num_bits u8][docs][pfor freqs]; posting_block_score_prefix_len(block_size) keys every reader/writer. 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. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
BubbleCal
changed the title
…igurable Resolves the ngram.rs conflict by taking main's byte-bounded stream_spill_reader wholesale and dropping this branch's boxing refactor (NGramSpillStream alias), which main's rewrite superseded.
Threading a runtime block_size through PostingIterator replaced the old compile-time BLOCK_SIZE division (a shift) with real div instructions in doc()/next(), costing 11-14% on 3-word OR queries against legacy 128-block indexes. Block sizes are validated powers of two, so derive block indices with trailing_zeros shifts and masks instead.
`ensure_num_tokens_loaded` rebuilt the num_tokens-only `DocSet` from the cached arrow column on every call, copying the whole column (tens of MB per partition) once per query per partition. Materialize it once in a `OnceCell` like the full DocSet, and account for it in `deep_size_of`. ## Measured vs its merge-base Per-branch-tip wheels on the 200M-doc legacy index (338 partitions), 1000 queries × 8 concurrent, 400G index cache. The rebuild only happens while a partition's DocSet is deferred (not yet fully materialized), so the win shows on the not-yet-prewarmed path, and the fix is exactly neutral once the index is warm: | protocol | query | base | this PR | |---|---|---|---| | no prewarm, steady-state passes | single-term k10 | 0.304s / 27 qps | **0.076s / 105 qps (4.0×)** | | no prewarm, steady-state passes | 3-word OR k10 | 0.359s / 22 qps | **0.179s / 45 qps (2.0×)** | | prewarmed | single-term k10 | 0.027s | 0.027s (neutral) | | prewarmed | 3-word OR k10 / k100 | 0.133s / 0.258s | 0.133s / 0.257s (neutral) | In the stacked V3 setup (#7466 and its followers) partitions serve queries from the tokens-only DocSet indefinitely, where this same rebuild dominated hot single-term queries (250ms → 3ms when the cache landed there). Independent of the block-size/impact PR stack; applies to any FTS query. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Yang Cen <yang@lancedb.com>
…igurable Reconciles #7600's tokens_only DocSet cache with this branch's quantized_scoring flag: the cached num_tokens-only DocSet is built with quantized scoring applied inside the OnceCell init, matching how ensure_loaded stamps the full DocSet.
…igurable Resolves the process_document conflict against the list-FTS restructure (#7656): keep main's DocumentSource/process_text closure shape and thread this branch's posting block_size through the PostingListBuilder construction sites (position closure and the positionless resize_with).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesThe FTS inverted index now supports validated 128- and 256-document posting blocks, introduces V3 format metadata, updates compression and decoding paths, preserves legacy defaults, and propagates block size through builders, caches, scoring, bindings, and compatibility tests. FTS configuration and metadata
Posting implementation
Stream API cleanup
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 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 `@protos/index_old.proto`:
- Line 42: Add a protobuf comment directly above optional field block_size
explaining that absence indicates a legacy index and is treated as 128, while
presence indicates the explicitly configured block size.
In `@rust/lance-index/src/scalar/inverted/tokenizer.rs`:
- Around line 777-790: Refactor test_block_size_accepts_supported_values to use
rstest parameterization instead of a manual loop. Add named #[case] entries for
block sizes 128 and 256, accept the block_size case value as the test parameter,
and retain the existing parameter construction and serialization round-trip
assertions.
- Around line 36-39: Document LEGACY_BLOCK_SIZE and DEFAULT_BLOCK_SIZE with
clear Rust doc comments explaining their distinct roles: the former preserves
backward compatibility for existing indexes, while the latter defines the
default for newly created indexes; note that they currently share the value 128
intentionally.
- Around line 265-274: Update the serde_json::Value::Number branch in the
tokenizer deserialization logic so the as_u64() failure message includes the
original numeric value, matching the contextual error style of the other arm;
preserve the existing valid-value resolution and error propagation behavior.
In `@rust/lance/src/io/exec/pushdown_scan.rs`:
- Line 329: Document the public FragmentScanner::scan method with Rustdoc,
including its BoxStream<Result<RecordBatch>> return behavior and error
propagation. Add a compile-valid usage example that constructs or obtains a
FragmentScanner, invokes scan, and handles the streamed results, and link the
documentation to FragmentScanner::open and ScanConfig using intra-doc links.
🪄 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: e7724129-c6b6-43d1-af47-f9434b030486
📒 Files selected for processing (27)
docs/src/format/index/scalar/fts.mddocs/src/quickstart/full-text-search.mdjava/src/main/java/org/lance/index/scalar/InvertedIndexParams.javajava/src/test/java/org/lance/index/scalar/InvertedIndexParamsTest.javaprotos/index_old.protopython/python/lance/dataset.pypython/python/tests/compat/test_scalar_indices.pypython/python/tests/test_scalar_index.pypython/src/dataset.rsrust/compression/bitpacking/src/bitpacker_internal/bitpacker8x.rsrust/compression/bitpacking/src/bitpacker_internal/mod.rsrust/compression/bitpacking/src/lib.rsrust/lance-index/protos-cache/cache.protorust/lance-index/src/scalar/inverted.rsrust/lance-index/src/scalar/inverted/builder.rsrust/lance-index/src/scalar/inverted/cache_codec.rsrust/lance-index/src/scalar/inverted/encoding.rsrust/lance-index/src/scalar/inverted/index.rsrust/lance-index/src/scalar/inverted/iter.rsrust/lance-index/src/scalar/inverted/lazy_docset.rsrust/lance-index/src/scalar/inverted/tokenizer.rsrust/lance-index/src/scalar/inverted/wand.rsrust/lance/src/dataset/mem_wal/index.rsrust/lance/src/dataset/mem_wal/index/fts.rsrust/lance/src/dataset/mem_wal/memtable/flush.rsrust/lance/src/io/exec/filtered_read.rsrust/lance/src/io/exec/pushdown_scan.rs
| } | ||
|
|
||
| pub fn scan(self) -> Result<impl Stream<Item = Result<RecordBatch>> + 'static + Send> { | ||
| pub fn scan(self) -> Result<BoxStream<'static, Result<RecordBatch>>> { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the public scan API.
FragmentScanner::scan is public but has no Rust documentation, example, or links to related APIs. Add a compile-valid example and link to FragmentScanner::open and ScanConfig; keep the example synchronized with the BoxStream return type and error behavior.
As per coding guidelines, all public APIs must have documentation with examples, and documentation should link 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/io/exec/pushdown_scan.rs` at line 329, Document the public
FragmentScanner::scan method with Rustdoc, including its
BoxStream<Result<RecordBatch>> return behavior and error propagation. Add a
compile-valid usage example that constructs or obtains a FragmentScanner,
invokes scan, and handles the streamed results, and link the documentation to
FragmentScanner::open and ScanConfig using intra-doc links.
Source: Coding guidelines
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/tokenizer.rs (1)
463-466: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid bare
.unwrap()in library code.
default_fts_format_version_for_block_size(self.block_size)returns aResultthat only succeeds for validated block sizes.block_sizeispub(crate), so a future in-crate assignment of an unvalidated value would panic here. Use.expect("reason")(block size is guaranteed valid) at minimum.As per coding guidelines, "Avoid bare
.unwrap(); ... If unavoidable, use.expect(\"reason\")."🛡️ Suggested change
pub fn resolved_format_version(&self) -> InvertedListFormatVersion { self.format_version - .unwrap_or_else(|| default_fts_format_version_for_block_size(self.block_size).unwrap()) + .unwrap_or_else(|| { + default_fts_format_version_for_block_size(self.block_size) + .expect("block_size is validated to a supported value before this call") + }) }🤖 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/tokenizer.rs` around lines 463 - 466, Replace the bare unwrap in InvertedListFormatVersion::resolved_format_version with expect and a clear message stating that block_size must be valid when resolving the default FTS format version.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/tokenizer.rs`:
- Around line 463-466: Replace the bare unwrap in
InvertedListFormatVersion::resolved_format_version with expect and a clear
message stating that block_size must be valid when resolving the default FTS
format version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1ec16c8f-19ec-4a94-8b7a-b113ecffcc19
📒 Files selected for processing (2)
protos/index_old.protorust/lance-index/src/scalar/inverted/tokenizer.rs
BubbleCal
deleted the
yang/oss-1344-make-fts-index-block-size-configurable
branch
Builds on the merged configurable posting block size work in #7466. Format discussion: #7606. Store per-block `(freq, doc_len)` impact frontiers alongside compressed posting blocks, plus one level-1 entry per 32 blocks, and drive block-max WAND pruning from them instead of build-time scores that can become stale as index statistics drift. - Both 128- and 256-doc blocks use the same compact impact codec: quantized `u8` document-length norms, delta-encoded frequencies, and an omitted norm byte for the common `+1` delta. - Scorer-specific bounds bake once into cache-shared state; query-local caches reuse the slab without sharing stale bounds across different corpus statistics. - Impact data survives packed prewarm and persistent-cache round trips. Posting-list cache versions are bumped while older versions remain readable. - Packed posting views share both impact-derived state and the block-head cache introduced by #7466. - Malformed or missing impact entries fall back to conservative infinite bounds instead of enabling unsafe WAND skips. - Public posting cache-key struct literals remain source-compatible; impact-bearing entries use an internal namespaced key. - V3 indexes without impacts retain the finite BM25 ceiling, while custom scorers without a declared safe bound fall back to `INFINITY`. The query benchmark below predates this codec follow-up; the V3 scoring bounds are unchanged, but impact size and decode cost need to be remeasured. ## Benchmark Measured before this restack against #7466 using per-branch-tip wheels: 200M-doc V3/256 index, 24 partitions, 1000 warm queries at 8 concurrent requests. | query | #7466 list-max fallback | this PR | |---|---|---| | OR 3w k10 | 0.363s / 22 qps | **0.103s / 76 qps (3.5x)** | | OR 3w k100 | 0.392s / 20 qps | **0.198s / 40 qps (2.0x)** | | AND 3w k10 | 0.547s / 15 qps | **0.115s / 69 qps (4.8x)** | | AND 3w k100 | 0.558s / 14 qps | **0.245s / 33 qps (2.3x)** | ## Validation - `cargo test -p lance-index`: 773 passed, 2 ignored; doctest passed - After the final rebase to current `main`, `cargo test -p lance-index --lib scalar::inverted`: 249 passed - `cargo check --workspace --tests --benches` - `cargo clippy --all --tests --benches -- -D warnings` - `cargo fmt --all -- --check` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Full-text search now optionally uses impact skip data to improve block-max scoring and pruning when index partitions provide it. - Impacts are carried through compressed and packed posting data, with impacts-aware WAND routing and scorer-weighted bound caching. - **Bug Fixes** - Improved decoding/validation of impacts envelopes, including safe behavior for malformed, null, or truncated data. - More robust posting component extraction and cache-key isolation to prevent mixing impact vs non-impact partitions. - **Tests** - Expanded roundtrip, cache isolation, and backward/forward compatibility tests for impact-enabled and legacy postings. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Yang Cen <yang@lancedb.com>
2 participants
Feature
Linear: OSS-1344
What is the new feature?
FTS inverted index creation now accepts a
block_sizeparameter for compressed posting blocks. Supported values are128and256.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?
block_sizetoInvertedIndexParams, protobuf details, posting-list schema metadata, and cache headers.128as the default for newly created indexes.block_sizeas legacy128.512, with a clear validation error.BitPacker4xfor physical 128-value posting blocks andBitPacker8xfor physical 256-value posting blocks.block_size=256as experimental in public API docs because it may introduce breaking changes.block_size=128, since older wheels cannot read current-created physical 256 FTS posting blocks.Validation
cargo fmt --allcargo fmt --all --checkgit diff --checkCARGO_TARGET_DIR=/tmp/lance-target-a479-no512 cargo test -p lance-index block_size -- --nocaptureCARGO_TARGET_DIR=/tmp/lance-target-a479-no512 cargo clippy -p lance-index --tests -- -D warningsuv run make buildfrompython/uv run pytest python/tests/test_scalar_index.py::test_create_scalar_index_fts_block_sizefrompython/uv run ruff format --check python/tests/test_scalar_index.py python/lance/dataset.pyfrompython/uv run ruff check python/tests/test_scalar_index.py python/lance/dataset.pyfrompython/CARGO_TARGET_DIR=/tmp/lance-target-a479-merge-main cargo test -p lance-index block_size -- --nocaptureCARGO_TARGET_DIR=/tmp/lance-target-a479-merge-main cargo test -p lance-index test_256_posting_block_uses_single_physical_bitpack_chunk -- --nocaptureCARGO_TARGET_DIR=/tmp/lance-target-a479-merge-main cargo test -p lance-bitpackingCARGO_TARGET_DIR=/tmp/lance-target-a479-merge-main cargo clippy -p lance-bitpacking -p lance-index --tests -- -D warningsuv run ruff format --check python/tests/compat/test_scalar_indices.pyfrompython/uv run ruff check python/tests/compat/test_scalar_indices.pyfrompython/uv run pytest --run-compat -vvv -s python/tests/compat/test_scalar_indices.py::test_FtsIndex_downgrade --durations=30frompython/CARGO_TARGET_DIR=/tmp/lance-a479-target cargo test -p lance-index test_new_training_request_defaults_missing_block_size_to_128CARGO_TARGET_DIR=/tmp/lance-a479-target cargo test -p lance-index block_sizeuv run ruff format --check python/lance/dataset.pyfrompython/uv run ruff check python/lance/dataset.pyfrompython/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:
[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 feat(fts): impact skip data for posting lists #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_sizethroughPostingIteratorinitially replaced the compile-timeBLOCK_SIZEdivision(a shift) with real
divinstructions in thedoc()/next()hot loops,measured as +11-14% on 3-word OR against the 200M legacy index
(
PostingIterator::nextgrew from 16.5% to 23.6% of the profile). Blocksizes are validated powers of two, so the iterator now derives block indices
with
trailing_zerosshifts and masks; after that fix the legacy path is atparity 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%) fromPFOR 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.
Summary by CodeRabbit
block_size(128/256) to scalar index creation, including updated examples and APIs.v3) forblock_size=256, with quantized doc-length scoring for v3.block_size/format_versioncompatibility (invalid combinations now error).128.block_size, defaults, accepted values, and the experimental256/v3behavior.