Skip to content

perf(fts): pre-load doc lengths in parallel on cold deferred search path - #8119

Merged
Xuanwo merged 1 commit into
lance-format:mainfrom
westonpace:perf/fts-regression-07-26
Aug 2, 2026
Merged

perf(fts): pre-load doc lengths in parallel on cold deferred search path#8119
Xuanwo merged 1 commit into
lance-format:mainfrom
westonpace:perf/fts-regression-07-26

Conversation

@westonpace

Copy link
Copy Markdown
Member

New-format indexes persist total_tokens in schema metadata, so aggregate_corpus_stats() resolves O(1) without loading doc lengths as a side effect. The scoring phase then had to load lengths sequentially per partition, adding one extra disk round-trip on the cold query path.

Fix: after aggregate_corpus_stats(), pre-load lengths in parallel for partitions that contain at least one query token. Partitions with no matching terms are skipped to preserve the existing no-load optimization for no-hit queries.

New-format indexes persist total_tokens in schema metadata, so
aggregate_corpus_stats() resolves O(1) without loading doc lengths as a
side effect. The scoring phase then had to load lengths sequentially per
partition, adding one extra disk round-trip on the cold query path.

Fix: after aggregate_corpus_stats(), pre-load lengths in parallel for
partitions that contain at least one query token. Partitions with no
matching terms are skipped to preserve the existing no-load optimization
for no-hit queries.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot added A-index Vector index, linalg, tokenizer performance labels Jul 31, 2026
@westonpace
westonpace requested a review from Xuanwo July 31, 2026 15:46

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The cold-path goal is reasonable, but the claimed serial-I/O root cause is not supported by this pipeline: partition loads already run through bounded buffer_unordered streams, and this PR adds neither a benchmark nor an I/O-concurrency test. Please characterize the target workload and demonstrate a before/after cold latency or read-overlap improvement before changing this hot path.

Comment on lines +1355 to +1357
let has_match = (0..request.tokens.len())
.any(|i| part.tokens.get(request.tokens.get_token(i)).is_some());
has_match.then_some(async move { docs.lengths().await.map(|_| ()) })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This predicate treats “any token exists” as sufficient to eagerly read and permanently cache the whole _num_tokens column. An AND/phrase partition can contain one token but still be ineligible, and an empty visibility mask is checked only later; zero-result searches therefore gain O(rows) I/O/memory and can now fail on an irrelevant length read. Keep length loading behind required-position and visibility pruning, then parallelize only surviving reads.

Reproducer

Append this to test_no_hit_partition_does_not_load_document_columns after its existing assertions:

let tokens = Arc::new(Tokens::new(
    vec!["t0".to_owned(), "missing-token".to_owned()],
    DocType::Text,
));
let params = Arc::new(FtsSearchParams::new().with_limit(Some(10)));
let (row_ids, scores) = index
    .bm25_search(
        tokens, params, Operator::And, Arc::new(NoFilter),
        Arc::new(NoOpMetricsCollector), None,
    )
    .await
    .unwrap();
assert!(row_ids.is_empty());
assert!(scores.is_empty());
assert!(
    !documents.lengths_loaded(),
    "an AND query missing a required term should not load document lengths",
);

Run: cargo test -p lance-index test_no_hit_partition_does_not_load_document_columns

Observed on 132ffca279f8785828f0922e72c43473ee6e707a: the result is empty, but the final assertion fails because lengths_loaded() is true.

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance-index/src/scalar/inverted/index.rs 90.00% 0 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

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

Thank you!

@Xuanwo
Xuanwo merged commit 499045f into lance-format:main Aug 2, 2026
40 of 41 checks passed
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 performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants