feat(index): support element-document FTS targets#7788
Open
Xuanwo wants to merge 5 commits into
Open
Conversation
📝 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 Document-granularity contracts
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
![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
Comment on lines
+16
to
+34
| /** The unit treated as one full-text-search document. */ | ||
| public enum DocumentGranularity { | ||
| /** All text selected from one dataset row belongs to one document. */ | ||
| ROW("row"), | ||
|
|
||
| /** Each element of the deepest list on the field path is one document. */ | ||
| LIST_ELEMENT("list_element"); | ||
|
|
||
| private final String rustString; | ||
|
|
||
| DocumentGranularity(String rustString) { | ||
| this.rustString = rustString; | ||
| } | ||
|
|
||
| /** Return the stable value understood by the Rust API and serialized index parameters. */ | ||
| public String toRustString() { | ||
| return rustString; | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a builder usage example and API link.
Show InvertedIndexParams.builder().documentGranularity(...) and link to the builder method.
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 `@java/src/main/java/org/lance/DocumentGranularity.java` around lines 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.
Source: Coding guidelines
Comment on lines
+55
to
+57
| // The posting-list payload format. This is separate from index_version, | ||
| // which is an element-document compatibility marker for version 4. | ||
| optional uint32 posting_format_version = 14; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document how an absent posting_format_version is resolved.
Specify whether absence means inference from block_size, a fixed legacy version, or another fallback. This compatibility rule is part of the persistent schema contract.
As per coding guidelines, “Document the semantic meaning of both present and absent states for optional fields.”
🤖 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 `@protos/index_old.proto` around lines 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.
Source: Coding guidelines
Comment on lines
+3208
to
+3213
| if lance_field is None and logical_index_type not in ["INVERTED", "FTS"]: | ||
| raise KeyError(f"{column} not found in schema") | ||
| parameters = dict(index_type.parameters) | ||
| if logical_index_type in ["INVERTED", "FTS"]: | ||
| parameters["document_granularity"] = kwargs["document_granularity"] | ||
| config = json.dumps(parameters) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not require document_granularity to exist in kwargs.
create_index_uncommitted also invokes this helper, but its kwargs may not contain this key. An inverted IndexConfig therefore raises KeyError instead of using its configured value or the row default.
Proposed fix
parameters = dict(index_type.parameters)
if logical_index_type in ["INVERTED", "FTS"]:
- parameters["document_granularity"] = kwargs["document_granularity"]
+ if "document_granularity" in kwargs:
+ parameters["document_granularity"] = kwargs[
+ "document_granularity"
+ ]
+ else:
+ parameters.setdefault(
+ "document_granularity", DocumentGranularity.ROW.value
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if lance_field is None and logical_index_type not in ["INVERTED", "FTS"]: | |
| raise KeyError(f"{column} not found in schema") | |
| parameters = dict(index_type.parameters) | |
| if logical_index_type in ["INVERTED", "FTS"]: | |
| parameters["document_granularity"] = kwargs["document_granularity"] | |
| config = json.dumps(parameters) | |
| if lance_field is None and logical_index_type not in ["INVERTED", "FTS"]: | |
| raise KeyError(f"{column} not found in schema") | |
| parameters = dict(index_type.parameters) | |
| if logical_index_type in ["INVERTED", "FTS"]: | |
| if "document_granularity" in kwargs: | |
| parameters["document_granularity"] = kwargs[ | |
| "document_granularity" | |
| ] | |
| else: | |
| parameters.setdefault( | |
| "document_granularity", DocumentGranularity.ROW.value | |
| ) | |
| config = json.dumps(parameters) |
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 3212-3212: use jsonify instead of json.dumps for JSON output
Context: json.dumps(parameters)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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/dataset.py` around lines 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.
Comment on lines
153
to
+164
| let details = pbold::InvertedIndexDetails::try_from(¶ms)?; | ||
| let mut inverted_index = | ||
| InvertedIndexBuilder::new_with_fragment_mask(params, fragment_mask) | ||
| .with_progress(progress); | ||
| let files = inverted_index.update(data, index_store, None).await?; | ||
| Ok(CreatedIndex { | ||
| index_details: prost_types::Any::from_msg(&details).unwrap(), | ||
| index_version: format_version.index_version(), | ||
| index_version: if is_element_document { | ||
| INVERTED_INDEX_VERSION_ELEMENT_DOCUMENT | ||
| } else { | ||
| format_version.index_version() | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate the index-details encoding error instead of panicking.
Line 159 uses .unwrap() in library code, allowing an encoding failure to terminate the process rather than return the existing Result.
Proposed fix
- index_details: prost_types::Any::from_msg(&details).unwrap(),
+ index_details: prost_types::Any::from_msg(&details).map_err(|error| {
+ Error::internal(format!(
+ "failed to encode InvertedIndexDetails: {error}"
+ ))
+ })?,As per coding guidelines, “Never use .unwrap(), .expect(), panic!(), or assert!() in library code for fallible operations; use ? with Result and proper error types.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let details = pbold::InvertedIndexDetails::try_from(¶ms)?; | |
| let mut inverted_index = | |
| InvertedIndexBuilder::new_with_fragment_mask(params, fragment_mask) | |
| .with_progress(progress); | |
| let files = inverted_index.update(data, index_store, None).await?; | |
| Ok(CreatedIndex { | |
| index_details: prost_types::Any::from_msg(&details).unwrap(), | |
| index_version: format_version.index_version(), | |
| index_version: if is_element_document { | |
| INVERTED_INDEX_VERSION_ELEMENT_DOCUMENT | |
| } else { | |
| format_version.index_version() | |
| }, | |
| let details = pbold::InvertedIndexDetails::try_from(¶ms)?; | |
| let mut inverted_index = | |
| InvertedIndexBuilder::new_with_fragment_mask(params, fragment_mask) | |
| .with_progress(progress); | |
| let files = inverted_index.update(data, index_store, None).await?; | |
| Ok(CreatedIndex { | |
| index_details: prost_types::Any::from_msg(&details).map_err(|error| { | |
| Error::internal(format!( | |
| "failed to encode InvertedIndexDetails: {error}" | |
| )) | |
| })?, | |
| index_version: if is_element_document { | |
| INVERTED_INDEX_VERSION_ELEMENT_DOCUMENT | |
| } else { | |
| format_version.index_version() | |
| }, |
🤖 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.rs` around lines 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.
Source: Coding guidelines
Comment on lines
+401
to
+433
| async fn resolve_document_keys(&self, doc_ids: &[u32]) -> Result<Vec<(u64, Vec<u32>)>> { | ||
| if self.coordinate_rank == 0 { | ||
| return Ok(self | ||
| .resolve_row_ids(doc_ids) | ||
| .await? | ||
| .into_iter() | ||
| .map(|row_id| (row_id, Vec::new())) | ||
| .collect()); | ||
| } | ||
| if let Some(full) = self.full.get() | ||
| && full.has_row_ids() | ||
| { | ||
| return Ok(doc_ids | ||
| .iter() | ||
| .map(|&doc_id| (full.row_id(doc_id), full.doc_index(doc_id))) | ||
| .collect()); | ||
| } | ||
|
|
||
| let ranges = doc_ids | ||
| .iter() | ||
| .map(|&doc_id| doc_id as usize..doc_id as usize + 1) | ||
| .collect::<Vec<_>>(); | ||
| let coordinate_names = (0..self.coordinate_rank) | ||
| .map(doc_index_storage_column) | ||
| .collect::<Vec<_>>(); | ||
| let mut projection = Vec::with_capacity(1 + coordinate_names.len()); | ||
| projection.push(ROW_ID); | ||
| projection.extend(coordinate_names.iter().map(String::as_str)); | ||
| let batch = self | ||
| .reader() | ||
| .await? | ||
| .read_ranges(&ranges, Some(&projection)) | ||
| .await?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply fragment-reuse remapping before resolving document keys.
Unlike docs_for_wand, this direct API does not guard frag_reuse_index; its targeted read therefore returns raw stored row IDs. Force ensure_loaded() when a remapper exists before returning row/element coordinates.
Proposed fix
async fn resolve_document_keys(&self, doc_ids: &[u32]) -> Result<Vec<(u64, Vec<u32>)>> {
+ if self.frag_reuse_index.is_some() {
+ let full = self.ensure_loaded().await?;
+ return Ok(doc_ids
+ .iter()
+ .map(|&doc_id| (full.row_id(doc_id), full.doc_index(doc_id)))
+ .collect());
+ }
+
if self.coordinate_rank == 0 {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async fn resolve_document_keys(&self, doc_ids: &[u32]) -> Result<Vec<(u64, Vec<u32>)>> { | |
| if self.coordinate_rank == 0 { | |
| return Ok(self | |
| .resolve_row_ids(doc_ids) | |
| .await? | |
| .into_iter() | |
| .map(|row_id| (row_id, Vec::new())) | |
| .collect()); | |
| } | |
| if let Some(full) = self.full.get() | |
| && full.has_row_ids() | |
| { | |
| return Ok(doc_ids | |
| .iter() | |
| .map(|&doc_id| (full.row_id(doc_id), full.doc_index(doc_id))) | |
| .collect()); | |
| } | |
| let ranges = doc_ids | |
| .iter() | |
| .map(|&doc_id| doc_id as usize..doc_id as usize + 1) | |
| .collect::<Vec<_>>(); | |
| let coordinate_names = (0..self.coordinate_rank) | |
| .map(doc_index_storage_column) | |
| .collect::<Vec<_>>(); | |
| let mut projection = Vec::with_capacity(1 + coordinate_names.len()); | |
| projection.push(ROW_ID); | |
| projection.extend(coordinate_names.iter().map(String::as_str)); | |
| let batch = self | |
| .reader() | |
| .await? | |
| .read_ranges(&ranges, Some(&projection)) | |
| .await?; | |
| async fn resolve_document_keys(&self, doc_ids: &[u32]) -> Result<Vec<(u64, Vec<u32>)>> { | |
| if self.frag_reuse_index.is_some() { | |
| let full = self.ensure_loaded().await?; | |
| return Ok(doc_ids | |
| .iter() | |
| .map(|&doc_id| (full.row_id(doc_id), full.doc_index(doc_id))) | |
| .collect()); | |
| } | |
| if self.coordinate_rank == 0 { | |
| return Ok(self | |
| .resolve_row_ids(doc_ids) | |
| .await? | |
| .into_iter() | |
| .map(|row_id| (row_id, Vec::new())) | |
| .collect()); | |
| } | |
| if let Some(full) = self.full.get() | |
| && full.has_row_ids() | |
| { | |
| return Ok(doc_ids | |
| .iter() | |
| .map(|&doc_id| (full.row_id(doc_id), full.doc_index(doc_id))) | |
| .collect()); | |
| } | |
| let ranges = doc_ids | |
| .iter() | |
| .map(|&doc_id| doc_id as usize..doc_id as usize + 1) | |
| .collect::<Vec<_>>(); | |
| let coordinate_names = (0..self.coordinate_rank) | |
| .map(doc_index_storage_column) | |
| .collect::<Vec<_>>(); | |
| let mut projection = Vec::with_capacity(1 + coordinate_names.len()); | |
| projection.push(ROW_ID); | |
| projection.extend(coordinate_names.iter().map(String::as_str)); | |
| let batch = self | |
| .reader() | |
| .await? | |
| .read_ranges(&ranges, Some(&projection)) | |
| .await?; |
🤖 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/lazy_docset.rs` around lines 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.
Comment on lines
+372
to
+378
| let (child_index, child) = | ||
| find_child_case_insensitive(field, name).ok_or_else(|| { | ||
| Error::index(format!( | ||
| "FTS field '{}' does not contain child '{}'", | ||
| field.name, name | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Classify invalid FTS paths as caller input errors.
Missing roots or child fields originate from the requested path, but these branches return Error::index. Use Error::invalid_input so callers receive the documented error category.
As per coding guidelines, use Error::invalid_input for caller data issues.
Also applies to: 418-432
🤖 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/scalar/inverted.rs` around lines 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.
Source: Coding guidelines
Comment on lines
+879
to
+896
| #[test] | ||
| fn resolve_element_document_field() { | ||
| let schema = fts_test_schema(); | ||
| let tags = schema.field("tags").unwrap(); | ||
| let resolved = | ||
| resolve_fts_field(&schema, "tags", DocumentGranularity::ListElement).unwrap(); | ||
| assert_eq!(resolved.final_field_id, tags.id); | ||
| assert_eq!(resolved.root_column, "tags"); | ||
| assert_eq!(resolved.canonical_path, "tags"); | ||
| assert_eq!(resolved.coordinate_rank(), 1); | ||
|
|
||
| let err = resolve_fts_field(&schema, "text", DocumentGranularity::ListElement).unwrap_err(); | ||
| assert!(err.to_string().contains("has no List layer"), "{err}"); | ||
|
|
||
| let err = | ||
| resolve_fts_field(&schema, "tags[*]", DocumentGranularity::ListElement).unwrap_err(); | ||
| assert!(err.to_string().contains("does not exist"), "{err}"); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert both the error variant and message.
These tests only inspect Display text. Assert the invalid-input variant too, and parameterize the two input cases with rstest.
As per coding guidelines, assert both the error variant and message, and use rstest for Rust tests differing only by inputs.
🤖 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/scalar/inverted.rs` around lines 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.
Source: Coding guidelines
Comment on lines
+114
to
+126
| fn with_new_children( | ||
| self: Arc<Self>, | ||
| mut children: Vec<Arc<dyn ExecutionPlan>>, | ||
| ) -> DataFusionResult<Arc<dyn ExecutionPlan>> { | ||
| if children.len() != 1 { | ||
| return Err(DataFusionError::Internal( | ||
| "FtsDocumentExec expects one child".to_string(), | ||
| )); | ||
| } | ||
| Ok(Arc::new(Self::new( | ||
| children.pop().unwrap(), | ||
| self.resolved.clone(), | ||
| ))) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the relevant range with line numbers.
git ls-files rust/lance/src/io/exec/fts.rs
wc -l rust/lance/src/io/exec/fts.rs
sed -n '100,140p' rust/lance/src/io/exec/fts.rsRepository: lance-format/lance
Length of output: 1284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect surrounding code for constructor and call patterns.
sed -n '1,220p' rust/lance/src/io/exec/fts.rsRepository: lance-format/lance
Length of output: 8056
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("rust/lance/src/io/exec/fts.rs")
text = path.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 100 <= i <= 140:
print(f"{i:4}: {line}")
PYRepository: lance-format/lance
Length of output: 1465
Include the child count in the error. Matching the single child directly would remove the unnecessary unwrap() and make the failure message say how many children were passed.
🤖 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 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.
Source: Coding guidelines
Comment on lines
+267
to
+303
| fn batch_document_keys(batch: &RecordBatch) -> Result<Vec<DocumentKey>> { | ||
| let row_ids = batch[ROW_ID].as_primitive::<UInt64Type>(); | ||
| let doc_indices = batch | ||
| .column_by_name(DOC_INDEX_COL) | ||
| .map(|column| column.as_list::<i32>()); | ||
| (0..batch.num_rows()) | ||
| .map(|row| { | ||
| let doc_index = if let Some(doc_indices) = doc_indices { | ||
| if doc_indices.is_null(row) { | ||
| return Err(Error::internal( | ||
| "element-document FTS produced a null document coordinate".to_string(), | ||
| )); | ||
| } | ||
| doc_indices | ||
| .value(row) | ||
| .as_primitive::<arrow::datatypes::UInt32Type>() | ||
| .values() | ||
| .to_vec() | ||
| } else { | ||
| Vec::new() | ||
| }; | ||
| Ok(DocumentKey { | ||
| row_id: row_ids.value(row), | ||
| doc_index, | ||
| }) | ||
| }) | ||
| .collect() | ||
| } | ||
|
|
||
| fn batch_scored_document_keys(batch: &RecordBatch) -> Result<Vec<(DocumentKey, f32)>> { | ||
| let keys = batch_document_keys(batch)?; | ||
| let scores = batch[SCORE_COL].as_primitive::<Float32Type>(); | ||
| Ok(keys | ||
| .into_iter() | ||
| .enumerate() | ||
| .map(|(index, key)| (key, scores.value(index))) | ||
| .collect()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file section with line numbers.
sed -n '220,360p' rust/lance/src/io/exec/fts.rs
# Find callers and related batch construction / column constants.
rg -n "batch_document_keys|batch_scored_document_keys|ROW_ID|SCORE_COL|DOC_INDEX_COL" rust/lance/src/io/exec/fts.rs rust/lance/src -g '!**/target/**'Repository: lance-format/lance
Length of output: 43091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file to locate functions and nearby context.
ast-grep outline rust/lance/src/io/exec/fts.rs --view expanded
# Inspect just the relevant function definitions if the outline is too coarse.
rg -n -A20 -B20 "fn batch_document_keys|fn batch_scored_document_keys|ROW_ID|SCORE_COL|DOC_INDEX_COL" rust/lance/src/io/exec/fts.rsRepository: lance-format/lance
Length of output: 27840
Use fallible lookup for required FTS columns. batch[ROW_ID] and batch[SCORE_COL] can panic if an upstream batch drifts from the expected schema; fetch them with column_by_name() and return a contextual execution error 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/io/exec/fts.rs` around lines 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.
Source: Coding guidelines
Comment on lines
1195
to
1199
| _ => { | ||
| return Err(DataFusionError::Execution(format!( | ||
| "Column {} is not a string", | ||
| "FTS document column {} is not a string; nested List inputs must be expanded before filtering", | ||
| column, | ||
| ))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include the actual Arrow type in this validation error.
Proposed fix
return Err(DataFusionError::Execution(format!(
- "FTS document column {} is not a string; nested List inputs must be expanded before filtering",
+ "FTS document column {} has type {}; expected Utf8 or LargeUtf8; nested List inputs must be expanded before filtering",
column,
+ text_column.data_type(),
)));As per coding guidelines, error messages must include relevant variable names, values, and types.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _ => { | |
| return Err(DataFusionError::Execution(format!( | |
| "Column {} is not a string", | |
| "FTS document column {} is not a string; nested List inputs must be expanded before filtering", | |
| column, | |
| ))); | |
| _ => { | |
| return Err(DataFusionError::Execution(format!( | |
| "FTS document column {} has type {}; expected Utf8 or LargeUtf8; nested List inputs must be expanded before filtering", | |
| column, | |
| text_column.data_type(), | |
| ))); |
🤖 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 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.
Source: Coding guidelines
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
1 participant
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
[*]FTS field-path targets and persist a stableFtsTargetidentity without changing row-document behavior_doc_index: List<UInt32>in element-document resultsSummary by CodeRabbit
New Features
_doc_index.Bug Fixes