Skip to content

test(fts): cover raw row-list index coverage - #8181

Merged
Xuanwo merged 9 commits into
mainfrom
gatekeeper/fix-8180-1
Aug 5, 2026
Merged

test(fts): cover raw row-list index coverage#8181
Xuanwo merged 9 commits into
mainfrom
gatekeeper/fix-8180-1

Conversation

@lance-gatekeeper

@lance-gatekeeper lance-gatekeeper Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • rely on the explicit FTS document-granularity semantics merged in feat(index): add FTS document granularity #7788
  • remove the superseded historical list-document inference layer from this repair
  • add a raw-tokenizer regression covering default Row behavior across indexed and appended fragments

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::Row is the default and joins a list into one document, while DocumentGranularity::ListElement is 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_coverage
  • cargo test -p lance --test integration_tests --features slow_tests test_element_document_fts_flat_indexed_and_mixed
  • cargo test -p lance-index flat_bm25_search_treats_string_lists_as_row_documents --lib
  • cargo test -p lance test_fts_list_index_uses_row_level_documents --lib
  • cargo clippy --all --tests --benches -- -D warnings
  • cargo fmt --all -- --check
  • git diff --check

Fixes #8180

@github-actions github-actions Bot added A-index Vector index, linalg, tokenizer bug Something isn't working labels Aug 3, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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].

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: []

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in e0176a5. Replaced the hybrid fallback with durable single-mode inference and rebuild-required handling when historical evidence is genuinely ambiguous.

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread rust/lance/src/index/append.rs Outdated
})?;
let reference_list_document_mode =
reference_inverted_index.list_document_mode();
let reference_params = reference_inverted_index.params().clone();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 62bf072. Delta creation now derives the reference index parameters so append optimization preserves released V1 storage format and merge compatibility.

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Gate recommendation: approve with a non-blocking risk.

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?;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Gate recommendation: approve with a non-blocking risk.

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));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Xuanwo Xuanwo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#7788 is now merged, please rethink this bug based on our new semantic. Is it still valid?

@lance-gatekeeper lance-gatekeeper Bot changed the title fix(fts): align flat list tokenization test(fts): cover raw row-list index coverage Aug 4, 2026
@lance-gatekeeper

Copy link
Copy Markdown
Contributor Author

Addressed in 90d2c5e. Integrated the explicit document-granularity semantics from #7788, removed the superseded historical inference layer, and retained a focused raw-tokenizer regression covering Row behavior across indexed and appended fragments.

@github-actions github-actions Bot added the chore label Aug 4, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Xuanwo
Xuanwo merged commit 54f7b65 into main Aug 5, 2026
43 checks passed
@Xuanwo
Xuanwo deleted the gatekeeper/fix-8180-1 branch August 5, 2026 09:10
@Xuanwo Xuanwo added the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-index Vector index, linalg, tokenizer bug Something isn't working chore K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inverted index and flat full-text search tokenize List<Utf8> differently

1 participant