fix(fts): use v3 as the capability gate#7866
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughFTS format v4 support is removed. Format selection now uses v2 by default, v3 for code analysis or 256-document posting blocks, and preserves existing index formats during maintenance operations. Java, Python, Rust, tests, and migration documentation reflect the v1–v3 compatibility rules. ChangesFTS format migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant IndexCreator
participant InvertedIndexParams
participant CreationResolver
participant FTSMetadata
IndexCreator->>InvertedIndexParams: provide analyzer, block size, and optional format_version
InvertedIndexParams->>CreationResolver: resolve default and environment override
CreationResolver->>FTSMetadata: write compatible v2 or v3 format metadata
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
A-python
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/index.rs-5007-5011 (1)
5007-5011: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert that optimization preserves the actual FTS parameters.
The cases cover both block sizes and correctly expect V3, but the test never verifies that
analyzer("code"),block_size,split_identifiers, andpreserve_originalremain unchanged afteroptimize_indices. A regression that resets the 256-document block size or drops analyzer settings would still pass.This is required by the PR objective to preserve analyzer configuration and posting block size across optimization. As per coding guidelines, every feature must have corresponding tests.
Also applies to: 5034-5035, 5047-5053
🤖 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/index.rs` around lines 5007 - 5011, Enhance test_optimize_empty_code_fts_index_preserves_params to inspect the optimized FTS index after optimize_indices and assert that analyzer("code"), the parameterized block_size, split_identifiers, and preserve_original match their configured values. Keep the existing V3 assertion and both block-size cases.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 `@docs/src/guide/migration.md`:
- Around line 11-28: Update the FTS format migration guidance to explicitly
state that format v4 indexes are no longer supported or openable, and that
existing v4 indexes must be rebuilt or recreated using format v2 or v3. Place
this near the format-selection or existing-index operation guidance without
changing the other format behavior descriptions.
In `@rust/lance-index/src/scalar/inverted/builder.rs`:
- Around line 3455-3486: Convert test_v3_128_reuses_v2_physical_layout from
nested with_position/with_impacts loops to an rstest parameterized test. Add
named #[case] entries for all four boolean combinations so each case runs and
reports independently, while preserving the existing schema and metadata
assertions.
---
Other comments:
In `@rust/lance/src/index.rs`:
- Around line 5007-5011: Enhance
test_optimize_empty_code_fts_index_preserves_params to inspect the optimized FTS
index after optimize_indices and assert that analyzer("code"), the parameterized
block_size, split_identifiers, and preserve_original match their configured
values. Keep the existing V3 assertion and both block-size cases.
🪄 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: e0b24fe2-6fa4-4908-b891-f7f082111df9
📒 Files selected for processing (17)
docs/src/guide/migration.mdjava/src/main/java/org/lance/index/scalar/InvertedIndexParams.javajava/src/test/java/org/lance/index/scalar/InvertedIndexParamsTest.javapython/python/lance/dataset.pypython/python/tests/test_scalar_index.pypython/src/dataset.rsrust/lance-index/src/scalar/inverted.rsrust/lance-index/src/scalar/inverted/builder.rsrust/lance-index/src/scalar/inverted/encoding.rsrust/lance-index/src/scalar/inverted/impact.rsrust/lance-index/src/scalar/inverted/index.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/memtable/flush.rsrust/lance/src/index.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
wjones127
left a comment
wjones127
left a comment
There was a problem hiding this comment.
I have some minor comments on the docs, but it looks functionally correct.
## Summary - remove FTS format v4 and make v3 the maximum supported capability version - use v3 for the code analyzer or 256-document posting blocks, while keeping text analysis with 128-document blocks on v2 - allow v3 with both 128- and 256-document blocks; reject code analysis on v1/v2 - update Rust, Python, Java, mem-wal handling, tests, and migration documentation ## Root cause The v4 bump conflated reader capability with a new physical file format. Code analyzer configuration is persisted in `InvertedIndexDetails` and changes token generation, not the posting or position schema. Configurable posting block size was already represented by v3 and persisted in metadata. For 128-document blocks, v3 uses the same fields and codecs as v2. Since FTS v3 has not been formally released, v3 can be the capability gate for both code analyzer metadata and 256-document posting blocks without introducing v4. ## Compatibility contract - v1/v2: 128-document posting blocks only; no code analyzer - v3: code analyzer supported; 128- and 256-document posting blocks supported - analyzer configuration: index details metadata - posting block size: physical encoding metadata - v4: unsupported; no alias or migration for the unreleased format ## Validation - `cargo fmt --all` - focused `lance-index` tokenizer, physical-layout, merge, metadata, and plugin tests - focused `lance` code-analyzer optimize, mem-wal metadata, and flush tests - `./mvnw -Dtest=InvertedIndexParamsTest test` - 17 focused Python FTS tests - `uv run make format` - `uv run make lint` (cherry picked from commit 647567d)
2 participants
Summary
Root cause
The v4 bump conflated reader capability with a new physical file format. Code analyzer configuration is persisted in
InvertedIndexDetailsand changes token generation, not the posting or position schema. Configurable posting block size was already represented by v3 and persisted in metadata. For 128-document blocks, v3 uses the same fields and codecs as v2.Since FTS v3 has not been formally released, v3 can be the capability gate for both code analyzer metadata and 256-document posting blocks without introducing v4.
Compatibility contract
Validation
cargo fmt --alllance-indextokenizer, physical-layout, merge, metadata, and plugin testslancecode-analyzer optimize, mem-wal metadata, and flush tests./mvnw -Dtest=InvertedIndexParamsTest testuv run make formatuv run make lint