test(fts): cover raw row-list index coverage - #8181
Conversation
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The row-document fix is correct for modern indexes, but the fallback must also preserve released pre-#7656 element-document indexes. Those indexes remain readable and have no persisted list-document-mode marker, so this patch moves the same coverage-dependent false positives and false negatives onto historical datasets.
Please make fallback tokenization honor both released document models and add a checked-in historical-index fixture with appended unindexed rows.
| for element in iter_str_array(elements.as_ref()).flatten() { | ||
| all_tokens += count_text(element, temp_query_token_counts); | ||
| } | ||
| let doc = materialize_string_list(elements.as_ref()); |
There was a problem hiding this comment.
This unconditionally changes flat BM25 to row-document tokenization, but released pre-#7656 list indexes still use element documents. With the raw tokenizer, a historical indexed row ["a", "b"] has tokens "a" and "b"; after this line, an appended same-shaped row has only "a b". Query "a" therefore matches the indexed row but misses the appended row, while "a b" does the reverse.
The fallback needs a compatibility strategy that honors the index document model. Persist an explicit mode for future writers and define a conservative policy for unmarked historical indexes, covered by a released fixture.
Reproducer run against this head
Add this test in the existing index.rs test module:
#[tokio::test]
async fn flat_bm25_preserves_released_element_document_fallback() {
let mut docs =
GenericListBuilder::<i32, _>::new(GenericStringBuilder::<i32>::new());
docs.values().append_value("a");
docs.values().append_value("b");
docs.append(true);
let docs = Arc::new(docs.finish()) as ArrayRef;
let schema = Arc::new(Schema::new(vec![
ROW_ID_FIELD.clone(),
Field::new("text", docs.data_type().clone(), true),
]));
let batch = RecordBatch::try_new(
schema.clone(),
vec![Arc::new(UInt64Array::from(vec![2_u64])) as ArrayRef, docs],
)
.unwrap();
let input: SendableRecordBatchStream = Box::pin(RecordBatchStreamAdapter::new(
schema,
stream::iter(vec![Ok(batch)]),
));
let tokenizer = InvertedIndexParams::default()
.base_tokenizer("raw".to_owned())
.max_token_length(None)
.lower_case(false)
.stem(false)
.remove_stop_words(false)
.ascii_folding(false)
.build()
.unwrap();
let output = flat_bm25_search_stream_with_metrics(
input, "text".to_owned(), "a".to_owned(),
tokenizer, None, 100, None,
)
.await
.unwrap();
let batches: Vec<_> = output.try_collect().await.unwrap();
let scored = arrow::compute::concat_batches(&FTS_SCHEMA, &batches).unwrap();
assert_eq!(
scored[ROW_ID].as_primitive::<UInt64Type>().values(),
&[2]
);
}Run:
CARGO_TARGET_DIR=/home/agent/tmp/pr8181-impl-target \
cargo test -p lance-index \
flat_bm25_preserves_released_element_document_fallback -- --nocapture
Observed: the assertion fails with left: ScalarBuffer([]), right: [2].
There was a problem hiding this comment.
Addressed in 263ce55. Persisted one list-document mode per index, preserved released element-document fallback semantics, and added a checked-in v8 fixture with appended unindexed rows.
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The compatibility fallback still permits query results to depend on whether an identical list row is indexed. Both is not a persisted document model; it creates extra documents and changes matching and BM25 statistics on only the flat side.
Please resolve one model per index from durable format/history or positive layout evidence, and require a rebuild (or explicit operator choice) when an unmarked index is genuinely ambiguous. Indexed and appended fragments must use the same document contract.
| .get(LIST_DOCUMENT_MODE_KEY) | ||
| .map(|value| ListDocumentMode::from_str(value)) | ||
| .transpose()? | ||
| .unwrap_or(ListDocumentMode::Both); |
There was a problem hiding this comment.
Defaulting an unmarked index to Both does not preserve its historical element-document model; it adds a joined-row document only for appended data. The checked-in v8 fixture has an indexed ["a", "b"] row and appends the same value, yet raw query "a b" returns only appended row 2. That keeps the coverage-dependent semantics reported by #8180 and also counts both flat representations in BM25 corpus statistics.
Resolve a single model where evidence permits it (for example, repeated stored row IDs prove Elements, and format ancestry can identify newer Row indexes). If metadata remains ambiguous, reject hybrid fallback/update with an actionable rebuild path rather than synthesizing matches.
Reproducer
At head 263ce55f4d951a63c592ea13d0d60ea2a3897331, in a detached worktree, change the final assertion in test_released_fts_list_index_uses_conservative_flat_fallback:
- vec![2]
+ Vec::<u64>::new()Run:
CARGO_TARGET_DIR=/home/agent/tmp/pr8181-head2-target cargo test -p lance test_released_fts_list_index_uses_conservative_flat_fallback --lib -- --nocapture
Observed:
assertion `left == right` failed
left: [2]
right: []
There was a problem hiding this comment.
Addressed in e0176a5. Replaced the hybrid fallback with durable single-mode inference and rebuild-required handling when historical evidence is genuinely ambiguous.
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The historical document-model mismatch is now handled conservatively, but this revision introduces two regressions for released indexes: opening any unmarked partitioned FTS index performs a full row-ID scan, and append-only optimization no longer preserves the reference segment's storage format.
Please defer document-model inference until a list operation needs it, and carry the reference index's resolved build parameters into delta creation. This keeps ordinary indexed reads cheap and every append segment merge-compatible.
| ) | ||
| })?; | ||
| has_row_document_layout |= documents.has_row_document_layout(); | ||
| row_id_columns.push(documents.row_ids_column().await?); |
There was a problem hiding this comment.
This eagerly reads every document row ID whenever an unmarked partitioned FTS index is opened, including scalar text/JSON indexes where list-document mode is irrelevant. Every released index lacks the new marker; row_ids_column() reads 0..num_docs for each partition, all arrays are retained together, and duplicate detection builds another corpus-wide set. A 100M-document index therefore reads at least about 800 MB of row IDs merely to open.
Defer inference until a list fallback or mutation actually needs the model, skip it entirely for scalar/indexed-only use, and scan partitions incrementally rather than retaining every column.
Reproducer
At head e0176a584e9c4d888ea2fc1336ea8d85510c4db5, modify test_modern_index_without_deleted_col_has_empty_bitmap to wrap doc_file_path(0) with the existing CountingStore, then assert after InvertedIndex::load:
assert_eq!(
counter.rows_read(),
0,
"opening an unmarked scalar index must not scan document row ids"
);Run:
CARGO_TARGET_DIR=/home/agent/tmp/pr8181-e0176-head-verify-target cargo test -p lance-index test_modern_index_without_deleted_col_has_empty_bitmap --lib --locked -- --nocapture
Observed:
assertion `left == right` failed: opening an unmarked scalar index must not scan document row ids
left: 1
right: 0
There was a problem hiding this comment.
Addressed in 62bf072. Deferred unmarked-mode inference until list fallback or mutation needs it, skipped indexed-only and scalar opens, and scans row IDs incrementally by partition.
| })?; | ||
| let reference_list_document_mode = | ||
| reference_inverted_index.list_document_mode(); | ||
| let reference_params = reference_inverted_index.params().clone(); |
There was a problem hiding this comment.
Cloning the persisted params here loses format_version, which is build-only and skip_serializing. In the append-only branch with no selected old segment, a V1 reference therefore trains its delta with the default V2 format (or an environment-selected format). The committed segments are no longer merge-compatible, and merge_segments rejects their differing formats.
Carry the reference index's resolved format into these typed params, preserving the behavior of the previous derive_index_params() path.
Reproducer
Run against head e0176a584e9c4d888ea2fc1336ea8d85510c4db5:
CARGO_TARGET_DIR=/home/agent/tmp/pr8181-e0176-head-verify-target cargo test -p lance test_fts_v1_remains_queryable_after_append_optimize --lib --locked -- --nocapture
Observed:
assertion failed: dataset.load_indices().await.unwrap().iter().all(|index| index.index_version == 1)
The existing V1 preservation test fails immediately after OptimizeOptions::append().
There was a problem hiding this comment.
Addressed in 62bf072. Delta creation now derives the reference index parameters so append optimization preserves released V1 storage format and merge compatibility.
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The lazy inference fixes the prior load and append-format regressions, but remap can still change a released element-document index's durable semantics based on whether mode inference ran before compaction.
Resolve and persist the historical document mode before rewriting remapped document metadata, and cover both unresolved and already-resolved remap orderings.
| ) -> Result<CreatedIndex> { | ||
| let files = self | ||
| .to_builder() | ||
| .to_builder(self.known_list_document_mode()) |
There was a problem hiding this comment.
This uses Ambiguous whenever the lazy cell has not been initialized, so remap loses the recoverable Elements mode of an unmarked released index. Remap also rewrites each document file with total_tokens row-layout metadata while omitting an ambiguous top-level mode. On reload, retained duplicate row IDs conflict with that new row evidence and resolve as Ambiguous; if remap filtered the duplicates away, the index can resolve as Row. Later list fallback or append therefore depends on whether an earlier operation happened to initialize the cell before compaction.
Resolve the historical mode before rewriting remapped metadata and persist it, or preserve equivalent durable evidence across remap.
Reproducer
Add this test to the existing index.rs test module:
#[tokio::test]
async fn test_remap_preserves_lazy_unmarked_element_document_mode() {
let src_dir = TempObjDir::default();
let dest_dir = TempObjDir::default();
let src_store = Arc::new(LanceIndexStore::new(
ObjectStore::local().into(),
src_dir.clone(),
Arc::new(LanceCache::no_cache()),
));
let dest_store = Arc::new(LanceIndexStore::new(
ObjectStore::local().into(),
dest_dir.clone(),
Arc::new(LanceCache::no_cache()),
));
let format_version = InvertedListFormatVersion::V1;
let mut partition = InnerBuilder::new_with_format_version(
0, false, TokenSetFormat::default(), format_version,
);
partition.tokens.add("test".to_owned());
let mut posting_list = PostingListBuilder::new_with_posting_tail_codec(
false, format_version.posting_tail_codec(),
);
posting_list.add(0, PositionRecorder::Count(1));
posting_list.add(1, PositionRecorder::Count(1));
partition.posting_lists.push(posting_list);
partition.docs.append(100, 1);
partition.docs.append(100, 1);
write_test_partition_with_optional_impacts(
&src_store, 0, partition, TokenSetFormat::default(), false,
)
.await;
write_test_metadata(
&src_store,
vec![0],
InvertedIndexParams::default().format_version(format_version),
)
.await;
let index = InvertedIndex::load(src_store, None, &LanceCache::no_cache())
.await
.unwrap();
assert!(index.list_document_mode.get().is_none());
index
.remap(
&RowAddrRemap::direct(HashMap::from([(100, Some(101))])),
dest_store.as_ref(),
)
.await
.unwrap();
let remapped = InvertedIndex::load(dest_store, None, &LanceCache::no_cache())
.await
.unwrap();
assert_eq!(
remapped.list_document_mode().await.unwrap(),
ListDocumentMode::Elements
);
}Run against this head:
cargo test -p lance-index test_remap_preserves_lazy_unmarked_element_document_mode --lib -- --nocapture
Observed:
assertion `left == right` failed
left: Ambiguous
right: Elements
FAILED. 0 passed; 1 failed; 932 filtered out
There was a problem hiding this comment.
Addressed in c20bd41. Remap now resolves and persists the historical mode before rewriting documents; regression cases cover unresolved and pre-resolved element indexes, plus ambiguous-mode preservation.
There was a problem hiding this comment.
The remap fix now resolves and persists historical modes before rewriting, so the previous access-order compatibility failure is closed. The remaining concern is a one-time extra inference pass when remapping unmarked scalar indexes; carrying the column's listness into remap, or reusing the rewrite's row-ID read, would avoid that migration cost for very large indexes.
| mapping: &RowAddrRemap, | ||
| dest_store: &dyn IndexStore, | ||
| ) -> Result<CreatedIndex> { | ||
| let list_document_mode = self.list_document_mode().await?; |
There was a problem hiding this comment.
Non-blocking: this resolves the document mode for every unmarked inverted index, including scalar string indexes where list semantics are irrelevant. Inference reads each partition's row-ID column and retains a corpus-wide RoaringTreemap; remap then reloads the partitions with a no-cache reader, so the first remap after upgrade adds O(N) row-ID I/O and potentially O(N) compressed-set memory before the full rewrite. The persisted destination marker makes this a one-time migration cost. Passing column listness into remap, or folding resolution into the rewrite's row-ID pass, would preserve compatibility without the extra scalar-index pass.
There was a problem hiding this comment.
Addressed in 29da87b. Deferred mode inference now consumes the document set already loaded for remap, eliminating the separate row-ID pass; regression coverage asserts remap performs only the rewrite's single document-file read.
There was a problem hiding this comment.
The remap path now reuses the document materialization and preserves historical modes deterministically, so the duplicate storage-I/O concern is resolved. The remaining non-blocking risk is CPU and set memory on very large unmarked scalar indexes: inference still walks all row IDs before remap walks them again. Passing column listness into remap would skip that irrelevant work for scalar columns.
| if let Some(mut inference) = list_document_mode_inference.take() { | ||
| let docs = std::mem::take(&mut builder.docs); | ||
| let (inference, docs) = spawn_cpu(move || { | ||
| inference.observe_row_ids(docs.iter().map(|(row_id, _)| *row_id)); |
There was a problem hiding this comment.
Non-blocking: this reuses builder.docs, so it removes the second storage read, but it remains a separate O(N) CPU traversal immediately before DocSet::remap traverses the same rows. The inference also retains O(unique row IDs) RoaringTreemap state across partitions. Sequential IDs should compress well and persisted destination metadata makes the cost one-time, but sparse IDs remain unbenchmarked. Threading column listness into remap would let scalar indexes skip inference entirely.
There was a problem hiding this comment.
Addressed in 0b2b56c. Dataset remap now passes the indexed column type, so unmarked scalar indexes persist Row without list-mode inference; typed list remap still infers and persists historical modes.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The dataset-aware remap path closes the remaining scalar-index migration cost: scalar columns skip irrelevant historical list-mode inference, while list columns and type-agnostic callers still resolve and persist compatible modes. This preserves released list semantics without adding corpus-wide inference work to scalar remaps.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The focused regression covers the original mismatch under the explicit document-granularity contract: raw row documents now behave consistently across indexed and appended fragments, while list-element documents remain an opt-in mode. Relying on the persisted granularity avoids reintroducing the superseded historical inference layer.
Summary
Root cause and resolution
The reported mismatch came from indexed and flat FTS paths assigning different document boundaries to
List<Utf8>values. PR #7788 now defines that contract explicitly:DocumentGranularity::Rowis the default and joins a list into one document, whileDocumentGranularity::ListElementis opt-in and carries element coordinates. Indexed, flat, and mixed-fragment paths share those semantics.That upstream change supersedes the compatibility-inference approach previously developed here. The remaining focused regression reproduces #8180 with raw-tokenized list rows split across indexed and appended fragments and verifies that Row queries behave identically regardless of index coverage.
Validation
cargo test -p lance --test integration_tests --features slow_tests test_row_document_raw_list_is_consistent_across_index_coveragecargo test -p lance --test integration_tests --features slow_tests test_element_document_fts_flat_indexed_and_mixedcargo test -p lance-index flat_bm25_search_treats_string_lists_as_row_documents --libcargo test -p lance test_fts_list_index_uses_row_level_documents --libcargo clippy --all --tests --benches -- -D warningscargo fmt --all -- --checkgit diff --checkFixes #8180