feat(index): add FTS document granularity#7788
Open
Xuanwo wants to merge 11 commits into
Open
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe PR adds row- and list-element document granularity to FTS across Java, Python, protobuf, Rust indexing, MemWAL, scanner planning, query execution, and result schemas. List-element searches preserve FTS document granularity
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
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
Xuanwo
marked this pull request as ready for review
July 15, 2026 14:43
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
rust/lance/src/dataset/mem_wal/memtable/scanner/builder.rs (2)
104-110: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve existing
FtsQuerystruct-literal compatibility.Adding this required public field breaks downstream callers constructing
FtsQuerydirectly. Introduce granularity through a non-breaking replacement/deprecation path instead.As per coding guidelines, “Never break public API signatures; deprecate old APIs and add a replacement instead.”
🤖 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/dataset/mem_wal/memtable/scanner/builder.rs` around lines 104 - 110, Preserve public struct-literal compatibility for FtsQuery by removing the newly required document_granularity field from the existing struct shape. Introduce document granularity through a non-breaking replacement or deprecated API path, updating FtsQuery-related construction and access logic to use that path without requiring downstream callers to add a field.Source: Coding guidelines
104-110: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftRequired fields added to public structs break existing callers.
rust/lance/src/dataset/mem_wal/memtable/scanner/builder.rs#L104-L110: expose document granularity through a non-breaking replacement/deprecation path rather than requiring a new struct-literal field.rust/lance-index/src/scalar/inverted/wand.rs#L1250-L1252: preserveDocCandidatecompatibility and update the omittedand_bulk_searchinitializer at Line 3136.As per coding guidelines, never break public API signatures; deprecate old APIs and add a replacement instead.
🤖 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/dataset/mem_wal/memtable/scanner/builder.rs` around lines 104 - 110, The new public FtsQuery.document_granularity field breaks existing struct literals; replace it with a non-breaking accessor or replacement construction path while preserving the default row-level granularity. In rust/lance/src/dataset/mem_wal/memtable/scanner/builder.rs lines 104-110, update FtsQuery accordingly. In rust/lance-index/src/scalar/inverted/wand.rs lines 1250-1252, preserve DocCandidate’s existing public construction compatibility and update the omitted and_bulk_search initializer at line 3136.Source: Coding guidelines
python/python/lance/query.py (1)
108-152: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
document_granularitybefore accessing.value.Passing a string or another invalid value currently raises an incidental
AttributeError. Use a shared validator for both constructors and raise a descriptiveTypeErrorcontaining the value and type.Proposed fix
+def _document_granularity_value(value: DocumentGranularity) -> str: + if not isinstance(value, DocumentGranularity): + raise TypeError( + "document_granularity must be a DocumentGranularity; " + f"got value={value!r}, type={type(value).__name__}" + ) + return value.value + ... - document_granularity=document_granularity.value, + document_granularity=_document_granularity_value( + document_granularity + ), ... - document_granularity=document_granularity.value, + document_granularity=_document_granularity_value( + document_granularity + ),As per coding guidelines, “Validate inputs and reject invalid values with descriptive errors at API boundaries” and “Include full context in error messages, including variable names, values, sizes, and types where relevant.”
Also applies to: 159-188
🤖 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 `@python/python/lance/query.py` around lines 108 - 152, Validate document_granularity at the start of both query constructors, before accessing document_granularity.value. Introduce or reuse one shared validator that accepts only DocumentGranularity values and raises TypeError for invalid inputs, including the variable name, received value, and value type in the message. Apply the same validation to the match-query constructor and the constructor at the additional referenced range.Source: Coding guidelines
java/src/main/java/org/lance/ipc/FullTextQuery.java (1)
67-131: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd Javadoc for the new granularity APIs.
Document the
ROWdefault, accepted enum semantics, list-column requirement forLIST_ELEMENT, and resulting document-coordinate behavior.As per coding guidelines, “Copy Rust documentation about defaults, constraints, and invariants into Javadoc for binding classes,” and “Document all public APIs with examples and links.”
Also applies to: 227-229, 314-316
🤖 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 `@java/src/main/java/org/lance/ipc/FullTextQuery.java` around lines 67 - 131, Add Javadoc to the new DocumentGranularity overloads in FullTextQuery.match and FullTextQuery.phrase, documenting the ROW default, each accepted enum’s semantics, the requirement that LIST_ELEMENT targets list columns, and the resulting document-coordinate behavior. Include usage examples and links consistent with the Rust documentation, while leaving existing overload behavior unchanged; apply the same documentation to the additional granularity APIs noted in the comment.Source: Coding guidelines
rust/lance-index/src/scalar/inverted/parser.rs (1)
190-234: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTest explicit and invalid
document_granularityJSON.The parser tests only cover the missing-field default. Add cases proving
"list_element"is propagated for both query types and an invalid value returnsError::InvalidInput.As per coding guidelines, “All Rust bug fixes and features must include corresponding tests.”
🤖 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/parser.rs` around lines 190 - 234, The parser tests for test_from_json_match and test_from_json_phrase only cover the omitted document_granularity default. Extend both query-type cases with explicit "list_element" JSON and assert DocumentGranularity::ListElement is propagated, and add invalid document_granularity cases for each query type that assert from_json returns Error::InvalidInput.Source: Coding guidelines
rust/lance/src/io/exec/fts.rs (2)
1365-1449: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not build the mixed-search scorer with default query parameters.
FlatMatchQueryExecstoresself.paramsbut discards it here. UsingFtsSearchParams::new()loses effective fuzzy expansion settings before the scorer is published to the indexed branch, producing incorrect mixed-search scores.Proposed fix
pub fn new_with_document_granularity(...) -> Self { let schema = fts_schema(document_granularity); + let params = MatchQueryExec::effective_params(&query, params); ... pub fn new_with_segments_and_document_granularity(...) -> Self { let schema = fts_schema(document_granularity); + let params = MatchQueryExec::effective_params(&query, params); ... fn execute(...) -> DataFusionResult<SendableRecordBatchStream> { let query = self.query.clone(); + let params = self.params.clone(); ... build_global_bm25_scorer( &indices, &query_tokens, - &FtsSearchParams::new(), + ¶ms, )Also applies to: 1537-1601
🤖 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/fts.rs` around lines 1365 - 1449, Preserve the caller-provided FtsSearchParams when constructing the mixed-search scorer in FlatMatchQueryExec, including the constructor path around new_with_document_granularity and the corresponding path near new_with_segments_and_document_granularity. Replace the default-parameter construction with self.params so effective fuzzy expansion settings reach the scorer used by the indexed branch.
195-230: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle
limit == 0before peeking the heap.Some(0)is a valid FTS limit, and this branch will panic on the first document because the heap stays empty.🤖 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/fts.rs` around lines 195 - 230, Handle a zero limit before entering the candidate-processing logic in the search flow containing the `candidates` heap and `searches` stream. Return an empty result for `params.limit == Some(0)` (or otherwise skip document processing) so the `candidates.peek()` branch is never reached with an empty heap; preserve existing behavior for positive and unlimited limits.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 `@java/src/main/java/org/lance/DocumentGranularity.java`:
- Around line 16-34: Add Javadoc to the public DocumentGranularity API showing
InvertedIndexParams.builder().documentGranularity(...) usage, and include links
to the relevant InvertedIndexParams type and builder method. Place the
documentation on the enum or its public usage point without changing enum
behavior.
In `@protos/index_old.proto`:
- Around line 55-57: Update the documentation comment for posting_format_version
to explicitly define the compatibility behavior when the optional field is
absent, including whether the value is inferred from block_size, defaults to a
fixed legacy version, or follows another established fallback; retain the
description of the present field’s meaning.
In `@python/python/lance/dataset.py`:
- Around line 3208-3213: Update the index parameter handling in create_index so
INVERTED and FTS configurations do not require kwargs["document_granularity"].
Reuse the value configured in index_type.parameters when present, otherwise
apply the existing row-level default, while preserving explicit kwargs behavior
and other index types.
In `@rust/lance-index/src/scalar/inverted.rs`:
- Around line 153-164: Update the index-details conversion in the surrounding
index creation function to propagate failures through its existing Result return
type: replace the unwrap on prost_types::Any::from_msg(&details) with the
appropriate ? handling, preserving the CreatedIndex construction and existing
error propagation.
In `@rust/lance-index/src/scalar/inverted/lazy_docset.rs`:
- Around line 401-433: Update resolve_document_keys to check frag_reuse_index
before resolving document keys, call ensure_loaded() when a remapper exists, and
apply the resulting fragment-reuse remapping to returned row IDs and element
coordinates. Preserve the existing coordinate_rank handling and targeted-read
behavior when no remapper is configured.
In `@rust/lance-index/src/scalar/inverted/query.rs`:
- Around line 317-319: Add doc comments to the public document-granularity field
and related builders/accessors, documenting that Row is the default, ListElement
requires a list target, and element searches return document coordinates. Update
the symbols at the referenced field and associated public APIs while preserving
their existing behavior.
In `@rust/lance-index/src/scalar/inverted/tokenizer.rs`:
- Around line 403-412: Rename the Tokenizer builder setter document_granularity
to with_document_granularity, and rename the getter get_document_granularity to
document_granularity. Update all call sites to use these FTS API names and do
not retain or introduce a get_-prefixed getter.
In `@rust/lance/src/dataset/mem_wal/index.rs`:
- Around line 1434-1447: Update the assertions in the registry lookup test to
verify each result’s document_granularity(), and preferably its selected index
name, rather than only column_name(). Cover both get_fts_by_column("tags") and
get_fts_by_column_and_granularity("tags", DocumentGranularity::ListElement),
ensuring the field-ID and column lookup APIs select their intended indexes.
- Around line 140-148: The metadata decoding branch in the inverted-index
details extraction must reject corrupt persisted data instead of falling back to
InvertedIndexParams::default(). Update the Err path in the index_details
handling to return a contextual Error::corrupt_file containing the index name,
details type, and decode failure; retain default construction only when
index_details is absent.
In `@rust/lance/src/dataset/mem_wal/memtable/scanner/exec/fts.rs`:
- Around line 117-124: Update the missing-index error in the FTS scanner’s index
lookup to include both the column and query.document_granularity variable names
with their values. Preserve the existing invalid-input error behavior and
clarify that the failure concerns the requested granularity.
- Around line 774-833: Extend test_element_document_fts_index_search with rstest
parameterized cases covering null list items, entirely null collections, empty
collections, and a null tags column. Build each case through the existing
list-element FTS setup, and assert the full nested coordinate list for every hit
rather than only the first coordinate, preserving the expected document IDs and
coordinates for each scenario.
In `@rust/lance/src/dataset/mem_wal/scanner/fts_search.rs`:
- Around line 74-79: Update query_document_granularity to recursively inspect
Boolean and Boost queries, derive their shared Match/Phrase DocumentGranularity,
and reject or otherwise handle mixed row/element targets instead of defaulting
compounds to Row. Preserve direct Match/Phrase behavior, ensure element-target
queries retain the active element index and _doc_index coordinates, and add
tests covering compound element queries.
In `@rust/lance/src/index/create.rs`:
- Around line 47-59: Update resolved_inverted_params to map both serde_json
parsing and final InvertedIndexParams deserialization failures to
Error::invalid_input. Include params.params and explicit context identifying
whether the failure occurred during parsing or validation, while preserving
successful merging and default behavior.
- Around line 193-198: Replace the expect-based downcasts in the
resolved_inverted_params call and the corresponding code around the second
referenced location with ok_or_else returning Error::internal, then propagate
failures using ?. Preserve the existing invariant messages and successful
ScalarIndexParams behavior while eliminating panic paths from these library code
paths.
In `@rust/lance/src/index/scalar.rs`:
- Around line 472-486: Update the error mapping in the InvertedIndexDetails
handling: change the decode failure from Error::io to Error::corrupt_file
because it indicates invalid persisted metadata, while keeping the re-encoding
failure as Error::io only if it represents an actual I/O error; otherwise
classify it as the appropriate internal serialization error variant. Preserve
the existing error context messages and normalization flow.
In `@rust/lance/src/index/scalar/inverted.rs`:
- Around line 879-896: Update resolve_element_document_field to use rstest
parameters for the two invalid resolve_fts_field inputs, and assert each error
is the expected invalid-input error variant as well as matching its existing
message text. Keep the successful tags assertions unchanged.
- Around line 372-378: Update the FTS path validation in the surrounding
field-resolution logic, including the branches near find_child_case_insensitive
and lines 418-432, to return Error::invalid_input instead of Error::index for
missing root or child fields. Preserve the existing error messages and lookup
behavior.
- Around line 139-164: Update the RecordBatch access around the row-id and
source-column handling to use column_by_name(), validate both columns’ types,
and return contextual errors containing the actual field name and DataType
instead of allowing as_primitive() to panic. In string_value, replace the Utf8,
LargeUtf8, and Utf8View downcast expect() calls with fallible validation that
returns contextual errors. Preserve null handling and successful value
extraction.
In `@rust/lance/src/io/exec/fts.rs`:
- Around line 1195-1199: Update the validation error in the FTS document-column
handling branch to include the actual Arrow data type alongside the column name.
Use the available schema or array type value in the DataFusionError message
while preserving the existing guidance about expanding nested List inputs.
- Around line 267-303: Update batch_document_keys and batch_scored_document_keys
to retrieve required ROW_ID and SCORE_COL columns via column_by_name() instead
of indexed access. Return a contextual execution Error when either column is
missing, while preserving the existing type conversions and scoring behavior for
valid schemas.
- Around line 114-126: Update FtsDocumentExec::with_new_children to destructure
the children collection directly when requiring exactly one child, eliminating
the unwrap and including the received child count in the
DataFusionError::Internal message for invalid input.
---
Outside diff comments:
In `@java/src/main/java/org/lance/ipc/FullTextQuery.java`:
- Around line 67-131: Add Javadoc to the new DocumentGranularity overloads in
FullTextQuery.match and FullTextQuery.phrase, documenting the ROW default, each
accepted enum’s semantics, the requirement that LIST_ELEMENT targets list
columns, and the resulting document-coordinate behavior. Include usage examples
and links consistent with the Rust documentation, while leaving existing
overload behavior unchanged; apply the same documentation to the additional
granularity APIs noted in the comment.
In `@python/python/lance/query.py`:
- Around line 108-152: Validate document_granularity at the start of both query
constructors, before accessing document_granularity.value. Introduce or reuse
one shared validator that accepts only DocumentGranularity values and raises
TypeError for invalid inputs, including the variable name, received value, and
value type in the message. Apply the same validation to the match-query
constructor and the constructor at the additional referenced range.
In `@rust/lance-index/src/scalar/inverted/parser.rs`:
- Around line 190-234: The parser tests for test_from_json_match and
test_from_json_phrase only cover the omitted document_granularity default.
Extend both query-type cases with explicit "list_element" JSON and assert
DocumentGranularity::ListElement is propagated, and add invalid
document_granularity cases for each query type that assert from_json returns
Error::InvalidInput.
In `@rust/lance/src/dataset/mem_wal/memtable/scanner/builder.rs`:
- Around line 104-110: Preserve public struct-literal compatibility for FtsQuery
by removing the newly required document_granularity field from the existing
struct shape. Introduce document granularity through a non-breaking replacement
or deprecated API path, updating FtsQuery-related construction and access logic
to use that path without requiring downstream callers to add a field.
- Around line 104-110: The new public FtsQuery.document_granularity field breaks
existing struct literals; replace it with a non-breaking accessor or replacement
construction path while preserving the default row-level granularity. In
rust/lance/src/dataset/mem_wal/memtable/scanner/builder.rs lines 104-110, update
FtsQuery accordingly. In rust/lance-index/src/scalar/inverted/wand.rs lines
1250-1252, preserve DocCandidate’s existing public construction compatibility
and update the omitted and_bulk_search initializer at line 3136.
In `@rust/lance/src/io/exec/fts.rs`:
- Around line 1365-1449: Preserve the caller-provided FtsSearchParams when
constructing the mixed-search scorer in FlatMatchQueryExec, including the
constructor path around new_with_document_granularity and the corresponding path
near new_with_segments_and_document_granularity. Replace the default-parameter
construction with self.params so effective fuzzy expansion settings reach the
scorer used by the indexed branch.
- Around line 195-230: Handle a zero limit before entering the
candidate-processing logic in the search flow containing the `candidates` heap
and `searches` stream. Return an empty result for `params.limit == Some(0)` (or
otherwise skip document processing) so the `candidates.peek()` branch is never
reached with an empty heap; preserve existing behavior for positive and
unlimited limits.
🪄 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: a1b25f55-a83e-405d-b5c7-a81305585098
📒 Files selected for processing (36)
java/lance-jni/src/blocking_dataset.rsjava/lance-jni/src/blocking_scanner.rsjava/src/main/java/org/lance/DocumentGranularity.javajava/src/main/java/org/lance/index/scalar/InvertedIndexParams.javajava/src/main/java/org/lance/ipc/FullTextQuery.javajava/src/test/java/org/lance/index/scalar/InvertedIndexParamsTest.javajava/src/test/java/org/lance/ipc/FullTextQueryTest.javajava/src/test/java/org/lance/ipc/LanceScannerFullTextSearchTest.javaprotos/index_old.protopython/python/lance/dataset.pypython/python/lance/query.pypython/python/tests/test_scalar_index.pypython/src/dataset.rsrust/lance-core/src/datatypes/schema.rsrust/lance-index/src/scalar/inverted.rsrust/lance-index/src/scalar/inverted/builder.rsrust/lance-index/src/scalar/inverted/index.rsrust/lance-index/src/scalar/inverted/lazy_docset.rsrust/lance-index/src/scalar/inverted/parser.rsrust/lance-index/src/scalar/inverted/query.rsrust/lance-index/src/scalar/inverted/tokenizer.rsrust/lance-index/src/scalar/inverted/wand.rsrust/lance-index/src/traits.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/dataset/mem_wal/memtable/scanner/builder.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/fts.rsrust/lance/src/dataset/mem_wal/scanner/fts_search.rsrust/lance/src/dataset/scanner.rsrust/lance/src/index/append.rsrust/lance/src/index/create.rsrust/lance/src/index/scalar.rsrust/lance/src/index/scalar/inverted.rsrust/lance/src/io/exec/fts.rsrust/lance/tests/query/inverted.rs
👮 Files not reviewed due to content moderation or server errors (6)
- rust/lance-index/src/traits.rs
- rust/lance/tests/query/inverted.rs
- rust/lance-index/src/scalar/inverted/builder.rs
- rust/lance/src/dataset/scanner.rs
- rust/lance/src/dataset/mem_wal/index/fts.rs
- rust/lance-index/src/scalar/inverted/index.rs
Xuanwo
changed the title
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)
1252-1252: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the new public field’s semantics.
Add a doc comment explaining that
doc_indexcontains root-to-leaf list ordinals for element documents, while an empty vector represents row-granular documents. As per coding guidelines, public API elements must document semantic meaning and valid states.🤖 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` at line 1252, Add a Rust doc comment directly above the public field doc_index in the relevant struct, specifying that it stores root-to-leaf list ordinals for element documents and that an empty vector represents row-granular documents.Source: Coding guidelines
1738-1738: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve list-element coordinates when emitting candidates.
All five WAND result paths currently set
doc_indextoVec::new(). That is correct only for row documents; for list-element indexes it drops the required root-to-leaf coordinates, causing downstream_doc_indexresults to be empty instead of values such asvec![0]andvec![1]. Populate the coordinate from the underlying document metadata in every path while retaining the empty vector for row-granular results.Also applies to: 1867-1867, 2365-2365, 3138-3138
🤖 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` at line 1738, Update all five WAND candidate-emission paths, including the code around the visible doc_index initialization and the locations at the other referenced lines, to derive doc_index from the underlying document metadata. Preserve an empty vector for row-granular indexes, but retain root-to-leaf coordinates for list-element indexes so downstream results include values such as [0] and [1].
🤖 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`:
- Line 1252: Add a Rust doc comment directly above the public field doc_index in
the relevant struct, specifying that it stores root-to-leaf list ordinals for
element documents and that an empty vector represents row-granular documents.
- Line 1738: Update all five WAND candidate-emission paths, including the code
around the visible doc_index initialization and the locations at the other
referenced lines, to derive doc_index from the underlying document metadata.
Preserve an empty vector for row-granular indexes, but retain root-to-leaf
coordinates for list-element indexes so downstream results include values such
as [0] and [1].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6d8e0d02-d180-4a08-963e-156214d474d8
📒 Files selected for processing (1)
rust/lance-index/src/scalar/inverted/wand.rs
…cument-phase1 # Conflicts: # rust/lance/src/index/scalar/inverted.rs # rust/lance/src/io/exec/fts.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/lance/src/dataset/scanner.rs (1)
1272-1286: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate
full_text_searchdocs for the actual API and granularity behavior.The example calls a nonexistent two-argument overload, while the method accepts
FullTextSearchQuery. Document theListElementmode,_doc_indexoutput, and link toFullTextSearchQueryandDocumentGranularity.🤖 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/dataset/scanner.rs` around lines 1272 - 1286, Update the documentation for Scanner::full_text_search to construct and pass a FullTextSearchQuery rather than using the nonexistent two-argument form. Document ListElement granularity behavior, including the generated _doc_index output, and add links to FullTextSearchQuery and DocumentGranularity.Source: Coding guidelines
rust/lance-index/src/scalar/inverted/index.rs (2)
6449-6463: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject gaps in persisted coordinate columns.
Probing ranks until the first missing column silently drops later coordinates. For example,
_doc_index_0and_doc_index_2would load only rank 0, producing incomplete_doc_indexresults while potentially still passing the list-element presence check. Validate that coordinate columns are contiguous, or persist and verify the expected rank count, and return a corruption error on gaps.🤖 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 6449 - 6463, Update the coordinate-column loading loop in from_columns_with_doc_indices to detect missing ranks without silently truncating later columns: after the first absent doc_index_storage_column(rank), verify that no subsequent coordinate columns exist or otherwise validate the expected contiguous rank count. Return the established corruption error when a gap is found, while preserving successful loading for contiguous columns.
146-162: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd synchronized rustdoc examples and cross-links for the new public surface.
rust/lance-index/src/scalar/inverted/index.rs#L146-L162: document the public FTS schemas andfts_schema.rust/lance-index/src/scalar/inverted/index.rs#L949-L959: documentbm25_search_documents.rust/lance-index/src/scalar/inverted/index.rs#L1296-L1321: documentload_params.rust/lance-index/src/scalar/inverted/index.rs#L6756-L6778: document coordinate-storage helpers.rust/lance/src/index/scalar/inverted.rs#L88-L104: documentcoordinate_rankanddocuments_from_batch.rust/lance/src/index/scalar/inverted.rs#L780-L790: document the granularity-awareload_segmentscontract.rust/lance/src/index/scalar/inverted.rs#L855-L862: documentload_segment_params.As per coding guidelines, “Document all public APIs with examples and links to relevant structs and methods; keep examples synchronized with actual signatures.”
🤖 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 146 - 162, Add synchronized rustdoc examples and relevant cross-links for the public APIs at rust/lance-index/src/scalar/inverted/index.rs:146-162, 949-959, 1296-1321, and 6756-6778, documenting the FTS schemas, fts_schema, bm25_search_documents, load_params, and coordinate-storage helpers; also document coordinate_rank, documents_from_batch, granularity-aware load_segments, and load_segment_params at rust/lance/src/index/scalar/inverted.rs:88-104, 780-790, and 855-862. Keep every example aligned with the current function signatures and link related structs and methods.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 `@rust/lance-index/src/scalar/inverted/index.rs`:
- Around line 1298-1318: The load_params method must only use the TOKENS_FILE
legacy path when opening METADATA_FILE returns a NotFound error; propagate all
other storage errors. Replace the generic missing-"params" error and serde
parsing failures for METADATA_FILE with Error::corrupt_file messages that
include the metadata filename and params key context.
In `@rust/lance/src/io/exec/fts.rs`:
- Around line 430-432: Add Rustdoc to the public
FtsIndexMetrics::record_scorer_build method documenting that elapsed is recorded
in milliseconds and applies only to the fallback scorer path, linking its metric
setup to FtsIndexMetrics::new. Include a synchronized usage example that
demonstrates invoking the method with a duration.
---
Outside diff comments:
In `@rust/lance-index/src/scalar/inverted/index.rs`:
- Around line 6449-6463: Update the coordinate-column loading loop in
from_columns_with_doc_indices to detect missing ranks without silently
truncating later columns: after the first absent doc_index_storage_column(rank),
verify that no subsequent coordinate columns exist or otherwise validate the
expected contiguous rank count. Return the established corruption error when a
gap is found, while preserving successful loading for contiguous columns.
- Around line 146-162: Add synchronized rustdoc examples and relevant
cross-links for the public APIs at
rust/lance-index/src/scalar/inverted/index.rs:146-162, 949-959, 1296-1321, and
6756-6778, documenting the FTS schemas, fts_schema, bm25_search_documents,
load_params, and coordinate-storage helpers; also document coordinate_rank,
documents_from_batch, granularity-aware load_segments, and load_segment_params
at rust/lance/src/index/scalar/inverted.rs:88-104, 780-790, and 855-862. Keep
every example aligned with the current function signatures and link related
structs and methods.
In `@rust/lance/src/dataset/scanner.rs`:
- Around line 1272-1286: Update the documentation for Scanner::full_text_search
to construct and pass a FullTextSearchQuery rather than using the nonexistent
two-argument form. Document ListElement granularity behavior, including the
generated _doc_index output, and add links to FullTextSearchQuery and
DocumentGranularity.
🪄 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: e9a3b2ac-b1ab-4e53-b59c-fea45bc9f4c5
📒 Files selected for processing (11)
python/python/lance/dataset.pyrust/lance-index/src/scalar/inverted/index.rsrust/lance/src/dataset/mem_wal/memtable/flush.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/fts.rsrust/lance/src/dataset/scanner.rsrust/lance/src/dataset/tests/dataset_index.rsrust/lance/src/index/append.rsrust/lance/src/index/create.rsrust/lance/src/index/scalar.rsrust/lance/src/index/scalar/inverted.rsrust/lance/src/io/exec/fts.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/lance/src/dataset/scanner.rs (1)
1272-1286: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate
full_text_searchdocs for the actual API and granularity behavior.The example calls a nonexistent two-argument overload, while the method accepts
FullTextSearchQuery. Document theListElementmode,_doc_indexoutput, and link toFullTextSearchQueryandDocumentGranularity.🤖 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/dataset/scanner.rs` around lines 1272 - 1286, Update the documentation for Scanner::full_text_search to construct and pass a FullTextSearchQuery rather than using the nonexistent two-argument form. Document ListElement granularity behavior, including the generated _doc_index output, and add links to FullTextSearchQuery and DocumentGranularity.Source: Coding guidelines
rust/lance-index/src/scalar/inverted/index.rs (2)
6449-6463: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject gaps in persisted coordinate columns.
Probing ranks until the first missing column silently drops later coordinates. For example,
_doc_index_0and_doc_index_2would load only rank 0, producing incomplete_doc_indexresults while potentially still passing the list-element presence check. Validate that coordinate columns are contiguous, or persist and verify the expected rank count, and return a corruption error on gaps.🤖 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 6449 - 6463, Update the coordinate-column loading loop in from_columns_with_doc_indices to detect missing ranks without silently truncating later columns: after the first absent doc_index_storage_column(rank), verify that no subsequent coordinate columns exist or otherwise validate the expected contiguous rank count. Return the established corruption error when a gap is found, while preserving successful loading for contiguous columns.
146-162: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd synchronized rustdoc examples and cross-links for the new public surface.
rust/lance-index/src/scalar/inverted/index.rs#L146-L162: document the public FTS schemas andfts_schema.rust/lance-index/src/scalar/inverted/index.rs#L949-L959: documentbm25_search_documents.rust/lance-index/src/scalar/inverted/index.rs#L1296-L1321: documentload_params.rust/lance-index/src/scalar/inverted/index.rs#L6756-L6778: document coordinate-storage helpers.rust/lance/src/index/scalar/inverted.rs#L88-L104: documentcoordinate_rankanddocuments_from_batch.rust/lance/src/index/scalar/inverted.rs#L780-L790: document the granularity-awareload_segmentscontract.rust/lance/src/index/scalar/inverted.rs#L855-L862: documentload_segment_params.As per coding guidelines, “Document all public APIs with examples and links to relevant structs and methods; keep examples synchronized with actual signatures.”
🤖 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 146 - 162, Add synchronized rustdoc examples and relevant cross-links for the public APIs at rust/lance-index/src/scalar/inverted/index.rs:146-162, 949-959, 1296-1321, and 6756-6778, documenting the FTS schemas, fts_schema, bm25_search_documents, load_params, and coordinate-storage helpers; also document coordinate_rank, documents_from_batch, granularity-aware load_segments, and load_segment_params at rust/lance/src/index/scalar/inverted.rs:88-104, 780-790, and 855-862. Keep every example aligned with the current function signatures and link related structs and methods.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 `@rust/lance-index/src/scalar/inverted/index.rs`:
- Around line 1298-1318: The load_params method must only use the TOKENS_FILE
legacy path when opening METADATA_FILE returns a NotFound error; propagate all
other storage errors. Replace the generic missing-"params" error and serde
parsing failures for METADATA_FILE with Error::corrupt_file messages that
include the metadata filename and params key context.
In `@rust/lance/src/io/exec/fts.rs`:
- Around line 430-432: Add Rustdoc to the public
FtsIndexMetrics::record_scorer_build method documenting that elapsed is recorded
in milliseconds and applies only to the fallback scorer path, linking its metric
setup to FtsIndexMetrics::new. Include a synchronized usage example that
demonstrates invoking the method with a duration.
---
Outside diff comments:
In `@rust/lance-index/src/scalar/inverted/index.rs`:
- Around line 6449-6463: Update the coordinate-column loading loop in
from_columns_with_doc_indices to detect missing ranks without silently
truncating later columns: after the first absent doc_index_storage_column(rank),
verify that no subsequent coordinate columns exist or otherwise validate the
expected contiguous rank count. Return the established corruption error when a
gap is found, while preserving successful loading for contiguous columns.
- Around line 146-162: Add synchronized rustdoc examples and relevant
cross-links for the public APIs at
rust/lance-index/src/scalar/inverted/index.rs:146-162, 949-959, 1296-1321, and
6756-6778, documenting the FTS schemas, fts_schema, bm25_search_documents,
load_params, and coordinate-storage helpers; also document coordinate_rank,
documents_from_batch, granularity-aware load_segments, and load_segment_params
at rust/lance/src/index/scalar/inverted.rs:88-104, 780-790, and 855-862. Keep
every example aligned with the current function signatures and link related
structs and methods.
In `@rust/lance/src/dataset/scanner.rs`:
- Around line 1272-1286: Update the documentation for Scanner::full_text_search
to construct and pass a FullTextSearchQuery rather than using the nonexistent
two-argument form. Document ListElement granularity behavior, including the
generated _doc_index output, and add links to FullTextSearchQuery and
DocumentGranularity.
🪄 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: e9a3b2ac-b1ab-4e53-b59c-fea45bc9f4c5
📒 Files selected for processing (11)
python/python/lance/dataset.pyrust/lance-index/src/scalar/inverted/index.rsrust/lance/src/dataset/mem_wal/memtable/flush.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/fts.rsrust/lance/src/dataset/scanner.rsrust/lance/src/dataset/tests/dataset_index.rsrust/lance/src/index/append.rsrust/lance/src/index/create.rsrust/lance/src/index/scalar.rsrust/lance/src/index/scalar/inverted.rsrust/lance/src/io/exec/fts.rs
🛑 Comments failed to post (2)
rust/lance-index/src/scalar/inverted/index.rs (1)
1298-1318: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -A12 -B4 'open_index_file|NotFound|is_not_found' rust/lance-index rust/lance --glob '*.rs'Repository: lance-format/lance
Length of output: 50375
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the target function and nearby legacy-loading context. sed -n '1280,1335p' rust/lance-index/src/scalar/inverted/index.rs printf '\n---\n' # Find the store API and how open_index_file errors are shaped. rg -n -A8 -B4 'async fn open_index_file|fn open_index_file|open_index_file\(' rust/lance-index rust/lance --glob '*.rs' | head -n 120 printf '\n---\n' # Search for load_legacy_index and metadata error handling in the same module. rg -n -A12 -B6 'load_legacy_index|metadata.*params|tokenizer' rust/lance-index/src/scalar/inverted/index.rsRepository: lance-format/lance
Length of output: 43781
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the IndexStore API and any concrete open_index_file implementations. rg -n -A12 -B6 'trait IndexStore|open_index_file\(|fn open_index_file' rust/lance-index rust/lance --glob '*.rs' | head -n 220 printf '\n---\n' # Look for explicit not-found matching around inverted index loading paths. rg -n -A8 -B4 'load_params\(|load_legacy_index\(|METADATA_FILE|TOKENS_FILE|Error::NotFound|object_store::Error::NotFound' rust/lance-index/src/scalar/inverted/index.rs rust/lance/src/index/scalar.rs --glob '*.rs'Repository: lance-format/lance
Length of output: 47582
Restrict the legacy fallback to missing metadata
Err(_)catches permission, transport, and other storage failures too; onlyNotFoundshould fall back toTOKENS_FILE, and other errors should propagate.- Missing or malformed
"params"inMETADATA_FILEshould raiseError::corrupt_filewith file/key context, not a generic index error.🤖 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 1298 - 1318, The load_params method must only use the TOKENS_FILE legacy path when opening METADATA_FILE returns a NotFound error; propagate all other storage errors. Replace the generic missing-"params" error and serde parsing failures for METADATA_FILE with Error::corrupt_file messages that include the metadata filename and params key context.Source: Coding guidelines
rust/lance/src/io/exec/fts.rs (1)
430-432: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Document the public scorer metric method.
Add Rustdoc explaining the millisecond unit, fallback-only scope, and its relationship to [
FtsIndexMetrics::new], with a synchronized usage example.As per coding guidelines, “Document all public APIs with examples and links 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/fts.rs` around lines 430 - 432, Add Rustdoc to the public FtsIndexMetrics::record_scorer_build method documenting that elapsed is recorded in milliseconds and applies only to the fallback scorer path, linking its metric setup to FtsIndexMetrics::new. Include a synchronized usage example that demonstrates invoking the method with a duration.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/src/dataset/mem_wal/index/fts.rs (1)
1359-1362: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate tail-freeze failures from
flush.
freezenow returnsResult, but this path only logs the error and continues. If tail partition construction fails,flushcan publish an incomplete index while appearing successful, losing documents from the exported index. Return the error fromflush(or otherwise prevent successful completion) and add a failure-path test.🤖 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/dataset/mem_wal/index/fts.rs` around lines 1359 - 1362, Update the tail-freeze handling in flush so an error returned by self.freeze(&st) is propagated and prevents successful completion instead of only being logged. Preserve the existing visible_count guard, and add a failure-path test verifying that a freeze failure causes flush to fail and does not publish an incomplete index.
🤖 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/src/dataset/mem_wal/index/fts.rs`:
- Around line 1359-1362: Update the tail-freeze handling in flush so an error
returned by self.freeze(&st) is propagated and prevents successful completion
instead of only being logged. Preserve the existing visible_count guard, and add
a failure-path test verifying that a freeze failure causes flush to fail and
does not publish an incomplete index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cb9c2dc5-78d4-43e5-877f-f6db44b9cc58
📒 Files selected for processing (1)
rust/lance/src/dataset/mem_wal/index/fts.rs
Collaborator
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
Collaborator
Author
|
@coderabbitai pause |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
✅ Action performedReviews paused. |
BubbleCal
reviewed
Jul 16, 2026
wjones127
self-requested a review
July 17, 2026 16:52
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full-text search currently treats each dataset row as a document. This change adds an explicit
DocumentGranularitywhile preserving row-level behavior as the default.Row: each dataset row is one document.ListElement: each element of the deepestListon the schema path is one document.groups.docs.content.(final_field_id, document_granularity)._doc_index, containing root-to-leaf physical list ordinals.The same document semantics are used by indexed search, flat search, mixed-fragment search, MemWAL, append/merge/optimize, and the Rust, Python, and Java APIs. Existing metadata without
document_granularitynaturally remains row-granular becauseROW = 0.Python example
Summary by CodeRabbit
New Features
DocumentGranularityoption that can be set for inverted/FTS index configuration and for match/phrase queries._doc_index, preserving document boundaries.Bug Fixes