fix: skip empty string docs in FTS stats#7699
Conversation
📝 WalkthroughWalkthroughThe inverted index and in-memory FTS paths now exclude documents that produce zero tokens from BM25 statistics, visibility counts, scoring, tail publication, freezing, and flushed indexes. Tests cover mixed and entirely empty corpora. ChangesZero-token FTS handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant FtsMemIndex
participant Tokenizer
participant TailIndex
participant Partition
FtsMemIndex->>Tokenizer: tokenize batch
Tokenizer-->>FtsMemIndex: return token counts
FtsMemIndex->>TailIndex: append non-zero-token rows
TailIndex->>Partition: freeze non-zero-token rows
Partition-->>FtsMemIndex: provide searchable frozen index
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
A-index
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance-index/src/scalar/inverted/builder.rs (1)
1433-1441: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
token_num > 0guard downstream is now always true.With this unconditional early return whenever
token_num == 0, execution past this point guaranteestoken_num > 0. The} else if token_num > 0 {at Line 1483 is therefore a silent guard against an impossible condition and should be simplified to a plainelse. As per coding guidelines, "Do not silently guard against impossible conditions; usedebug_assert!, return an explicit error, or remove the check."♻️ Proposed simplification (Line 1483)
- } else if token_num > 0 { + } else {🤖 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/builder.rs` around lines 1433 - 1441, In the control flow following the token_num == 0 early return, simplify the downstream `else if token_num > 0` branch to a plain `else`, since reaching it guarantees a positive token count. Preserve the existing branch body and behavior.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.
Outside diff comments:
In `@rust/lance-index/src/scalar/inverted/builder.rs`:
- Around line 1433-1441: In the control flow following the token_num == 0 early
return, simplify the downstream `else if token_num > 0` branch to a plain
`else`, since reaching it guarantees a positive token count. Preserve the
existing branch body and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1a248101-40ed-4aab-8dcf-7ee0edffef09
📒 Files selected for processing (1)
rust/lance-index/src/scalar/inverted/builder.rs
|
Hi @BubbleCal and @Xuanwo, this PR addresses the empty-document follow-up identified during the review of #7656. It makes plain string documents that produce zero tokens follow the same behavior as list-string documents, and adds regression coverage for mixed empty/non-empty input, all-empty indexes, and tail partition handling. Since you’re familiar with the original change and discussion, could you please take a look when you have time? I’d especially appreciate a check on whether the zero-token and empty-index edge cases are covered appropriately. Thanks! |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| ); | ||
|
|
||
| if skip_empty_document && token_num == 0 { | ||
| if token_num == 0 { |
There was a problem hiding this comment.
Could we apply this zero-token policy consistently across the other FTS paths? The flat/unindexed Utf8 path still appends every row and adds counted_input.num_rows() to num_docs, while MemWAL keeps zero-length documents and flushes them into the DocSet. A whitespace-only, stop-word-only, or overlength document is therefore excluded after a normal index build but counted while it is unindexed or flushed through MemWAL, so avgdl/IDF and potentially top-k ordering can change as the same data moves between these paths. Please update those paths as part of this change and add cross-path coverage.
There was a problem hiding this comment.
Thanks for pointing this out. I understood the consistency issue at a high level, but I wasn’t sure about the best way to implement the policy across the flat/unindexed and MemWAL paths.
This follow-up commit was implemented mostly with assistance from GPT-5.6-sol. It now excludes zero-token documents from the flat counted input, MemWAL corpus statistics, frozen DocSets, and the flush builder. The added coverage uses whitespace-only, stop-word-only, overlength, null, and normal documents, and checks that num_docs, avgdl, and the IDF/query weight match the normal-build baseline through the flat path and the MemWAL tail-to-frozen/flush transitions. It also checks that the MemWAL score remains unchanged across freeze.
Thanks again for the detailed review—it clarified the invariant that needs to hold across the full FTS lifecycle.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-index/src/scalar/inverted/index.rs`:
- Around line 9469-9487: Replace the manual Schema and RecordBatch::try_new
fixture setup with arrow_array::record_batch!(), preserving the ROW_ID_FIELD
values and nullable text values in the test data.
🪄 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: 72d74616-a25c-4352-b70e-d8cdd6003152
📒 Files selected for processing (3)
rust/lance-index/src/scalar/inverted/builder.rsrust/lance-index/src/scalar/inverted/index.rsrust/lance/src/dataset/mem_wal/index/fts.rs
| let schema = Arc::new(Schema::new(vec![ | ||
| ROW_ID_FIELD.clone(), | ||
| Field::new("text", DataType::Utf8, true), | ||
| ])); | ||
| let batch = RecordBatch::try_new( | ||
| schema, | ||
| vec![ | ||
| Arc::new(UInt64Array::from(vec![0_u64, 1, 2, 3, 4, 5])) as ArrayRef, | ||
| Arc::new(StringArray::from(vec![ | ||
| Some(""), | ||
| Some(" "), | ||
| Some("the"), | ||
| Some("overlength"), | ||
| None, | ||
| Some("hello"), | ||
| ])) as ArrayRef, | ||
| ], | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target area and nearby test context.
sed -n '9440,9525p' rust/lance-index/src/scalar/inverted/index.rs
# Find existing uses of record_batch!() in Rust tests for repo-local style.
rg -n --glob '*.rs' 'record_batch!\(' rust | head -n 80
# Check the Arrow crate version in the workspace lockfile / manifests.
rg -n --glob 'Cargo.lock' 'name = "(arrow-array|arrow-ar|arrow)"|version = ' Cargo.lock
rg -n --glob 'Cargo.toml' 'arrow-array|arrow' Cargo.toml rust/**/Cargo.tomlRepository: lance-format/lance
Length of output: 39859
Use record_batch!() for this test fixture.
Replace the manual Schema / RecordBatch::try_new setup with arrow_array::record_batch!() and keep the nullable string values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance-index/src/scalar/inverted/index.rs` around lines 9469 - 9487,
Replace the manual Schema and RecordBatch::try_new fixture setup with
arrow_array::record_batch!(), preserving the ROW_ID_FIELD values and nullable
text values in the test data.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance-index/src/scalar/inverted/builder.rs (1)
3593-3621: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winMissing
block_sizefield breaks compilation.
IndexWorkerConfigrequires ablock_size: usizefield (see the struct definition and every other literal in this file, e.g. lines 3284, 3308, 3910). This new test's literal omits it, so the file will fail to compile.🐛 Proposed fix
IndexWorkerConfig { with_position: false, format_version: InvertedListFormatVersion::V1, fragment_mask: None, token_set_format: TokenSetFormat::default(), worker_memory_limit_bytes: u64::MAX, + block_size: InvertedIndexParams::default().block_size, },🤖 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/builder.rs` around lines 3593 - 3621, Update the IndexWorkerConfig literal in test_all_empty_string_documents_do_not_create_tail_partition to include the required block_size field, matching the value or established pattern used by other IndexWorkerConfig literals in this file.
🧹 Nitpick comments (1)
rust/lance-index/src/scalar/inverted/index.rs (1)
10955-10974: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
record_batch!()hereReplace the manual
Schema/Arc/RecordBatch::try_newsetup withrecord_batch!()to keep this test fixture concise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-index/src/scalar/inverted/index.rs` around lines 10955 - 10974, Simplify the batch fixture in flat_bm25_skips_zero_token_documents_from_corpus_stats by replacing the manual Schema, Arc, and RecordBatch::try_new construction with the existing record_batch!() helper, while preserving the same row IDs and text values.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.
Outside diff comments:
In `@rust/lance-index/src/scalar/inverted/builder.rs`:
- Around line 3593-3621: Update the IndexWorkerConfig literal in
test_all_empty_string_documents_do_not_create_tail_partition to include the
required block_size field, matching the value or established pattern used by
other IndexWorkerConfig literals in this file.
---
Nitpick comments:
In `@rust/lance-index/src/scalar/inverted/index.rs`:
- Around line 10955-10974: Simplify the batch fixture in
flat_bm25_skips_zero_token_documents_from_corpus_stats by replacing the manual
Schema, Arc, and RecordBatch::try_new construction with the existing
record_batch!() helper, while preserving the same row IDs and text values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: b8c8f9d9-5dbb-4743-af54-44dccd70d1c0
📒 Files selected for processing (3)
rust/lance-index/src/scalar/inverted/builder.rsrust/lance-index/src/scalar/inverted/index.rsrust/lance/src/dataset/mem_wal/index/fts.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/lance-index/src/scalar/inverted/index.rs (1)
6181-6185: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPreallocate before materializing shared token counts.
to_vec()has no spare slot, so the followingpushcan reallocate and copy the entire buffer again.Proposed fix
Self::Shared(values) => { - let mut owned = values.to_vec(); + let mut owned = Vec::with_capacity(values.len() + 1); + owned.extend_from_slice(values.as_ref()); owned.push(value); *self = Self::Owned(owned); }As per coding guidelines, “Use
Vec::with_capacity()when size is known or estimable.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-index/src/scalar/inverted/index.rs` around lines 6181 - 6185, Update the Self::Shared branch to allocate the owned vector with capacity for values.len() + 1 before copying the shared values, then append the new value without triggering an avoidable reallocation; preserve the existing transition to Self::Owned.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.
Nitpick comments:
In `@rust/lance-index/src/scalar/inverted/index.rs`:
- Around line 6181-6185: Update the Self::Shared branch to allocate the owned
vector with capacity for values.len() + 1 before copying the shared values, then
append the new value without triggering an avoidable reallocation; preserve the
existing transition to Self::Owned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: f03ddb5b-6ffa-42a3-ab85-2b64f94a1030
📒 Files selected for processing (1)
rust/lance-index/src/scalar/inverted/index.rs
2 participants
Summary
Fixes #7660.
This PR makes FTS indexing skip plain string documents that produce zero tokens, matching the behavior recently added for list-string documents. Empty strings and whitespace-only strings no longer get appended to the FTS
DocSet, so they no longer inflate BM25 corpus statistics likenum_docsand average document length.Why
I followed the suggested fix from the issue and audited
IndexWorker::process_document.The root cause was that the empty-document behavior already existed, but only for list-string inputs.
process_batchpassedskip_empty_document = trueforList<Utf8>/LargeList<Utf8>, while plainUtf8/LargeUtf8passedfalse. As a result, a plain string document such as""or" "could be stored in the document metadata withnum_tokens = 0.Since such a document has no posting-list entries, keeping it only affects corpus-level statistics used by BM25 scoring. It does not add searchable content.
Implementation
I removed the special-case flag and made
process_documentreturn early whenever tokenization yieldstoken_num == 0, regardless of whether the source is a plain string or a list-string document.This keeps the change localized to the FTS builder and mirrors the issue's suggested direction: zero-token string documents now follow the same skip behavior as empty list-string documents.
Tests
Added regression coverage for:
Validation:
cargo fmt --allcargo test -p lance-index empty_string_documentsgit diff --check