fix(fts): restore format version environment fallback#7855
Conversation
📝 WalkthroughWalkthroughFTS index creation now prioritizes explicit ChangesFTS format resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant IndexCreation
participant from_training_json
participant resolve_creation_format_version
participant Environment
IndexCreation->>from_training_json: create FTS index
from_training_json->>resolve_creation_format_version: pass explicit format_version
resolve_creation_format_version->>Environment: read LANCE_FTS_FORMAT_VERSION when unset
Environment-->>resolve_creation_format_version: version or invalid value
resolve_creation_format_version-->>from_training_json: resolved version or error
from_training_json-->>IndexCreation: create index or fail
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
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-index/src/scalar/inverted/tokenizer.rs-162-164 (1)
162-164: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument invalid environment handling consistently.
The migration guide states that invalid
LANCE_FTS_FORMAT_VERSIONvalues fail creation and that an explicitformat_versionwins even when the environment is invalid, but these public API docs omit those effects.
rust/lance-index/src/scalar/inverted/tokenizer.rs#L162-L164: document invalid-value failure and explicit precedence.rust/lance-index/src/scalar/inverted/tokenizer.rs#L822-L825: document invalid-value failure and explicit precedence.java/src/main/java/org/lance/index/scalar/InvertedIndexParams.java#L267-L268: document invalid-value failure and explicit precedence.python/python/lance/dataset.py#L3374-L3375: document invalid-value failure and explicit precedence.As per coding guidelines, “Add doc comments to public API elements that convey semantic meaning, valid values, and effects.”
🤖 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 162 - 164, Update the public API documentation for the format-version settings at rust/lance-index/src/scalar/inverted/tokenizer.rs:162-164 and :822-825, java/src/main/java/org/lance/index/scalar/InvertedIndexParams.java:267-268, and python/python/lance/dataset.py:3374-3375 to state that invalid LANCE_FTS_FORMAT_VERSION values fail index creation, while an explicitly supplied format_version takes precedence even when the environment variable is invalid.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.
Other comments:
In `@rust/lance-index/src/scalar/inverted/tokenizer.rs`:
- Around line 162-164: Update the public API documentation for the
format-version settings at
rust/lance-index/src/scalar/inverted/tokenizer.rs:162-164 and :822-825,
java/src/main/java/org/lance/index/scalar/InvertedIndexParams.java:267-268, and
python/python/lance/dataset.py:3374-3375 to state that invalid
LANCE_FTS_FORMAT_VERSION values fail index creation, while an explicitly
supplied format_version takes precedence even when the environment variable is
invalid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: c6931127-2653-44d2-872c-8bff5ac63ba0
📒 Files selected for processing (5)
docs/src/guide/migration.mdjava/src/main/java/org/lance/index/scalar/InvertedIndexParams.javapython/python/lance/dataset.pypython/python/tests/test_scalar_index.pyrust/lance-index/src/scalar/inverted/tokenizer.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Context
New FTS / inverted index creation defaults to format v4, but the rollout also stopped honoring
LANCE_FTS_FORMAT_VERSIONwhenformat_versionis omitted. Deployments that rely on this process-wide compatibility switch could therefore create indexes unreadable by older readers despite retaining the rollout setting.This restores the established precedence: an explicit creation parameter wins, otherwise the environment variable is used, and v4 remains the default when neither is present. Invalid environment values fail clearly. The fallback is limited to new-index training, so append, optimize, and other maintenance operations continue preserving each index's existing format.
Regression coverage exercises the precedence and error cases in isolated subprocesses, and the migration and binding documentation now describe the compatibility behavior.