Skip to content

perf: avoid re-open shard indices and small reads#6026

Merged
wjones127 merged 6 commits intomainfrom
yang/optimize-merge
Feb 27, 2026
Merged

perf: avoid re-open shard indices and small reads#6026
wjones127 merged 6 commits intomainfrom
yang/optimize-merge

Conversation

@BubbleCal
Copy link
Copy Markdown
Contributor

  1. reuse the readers and avoid reopening them in each iteration
  2. read multiple partitions in one read to avoid small IOs

Signed-off-by: BubbleCal <bubble-cal@outlook.com>
@github-actions
Copy link
Copy Markdown
Contributor

Code Review

Summary: Performance optimization to avoid re-opening shard readers and batch partition reads using a 32-partition window.

Feedback

P1 - Test Coverage Gap: The existing test_merge_ivf_pq_success test uses only 2 partitions, which doesn't exercise the new windowing logic (PARTITION_WINDOW_SIZE=32). Consider adding a test with >32 partitions to verify:

  • Correct behavior at window boundaries
  • Batch slicing logic when records span partition boundaries
  • The remaining counter tracking across multiple batches

Minor observation: Holding all Arc<V2Reader> instances for the merge duration increases memory footprint proportionally to shard count. This is a reasonable trade-off for the I/O performance gains.

The implementation logic appears correct with good error handling for row count mismatches.

@BubbleCal BubbleCal requested a review from yanghua February 26, 2026 16:17
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 26, 2026

Codecov Report

❌ Patch coverage is 80.64516% with 42 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...lance-index/src/vector/distributed/index_merger.rs 80.64% 28 Missing and 14 partials ⚠️

📢 Thoughts on this report? Let us know!

@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Feb 26, 2026

just out of curiosity how did you choose 512 as the partition window size? how big should we expect that to be in memory?
also, the bot's comment about test coverage is probably right :)

@BubbleCal
Copy link
Copy Markdown
Contributor Author

just out of curiosity how did you choose 512 as the partition window size? how big should we expect that to be in memory? also, the bot's comment about test coverage is probably right :)

i assume we have 4K rows per partition roughly (the default index param)
so 512 partitions is about 200MB (say d=768, so 768 * 4 / 32 * 4096 * 512 = 192MB)

good catch, just added a test for testing that window size

@wjones127 wjones127 merged commit 0753ed8 into main Feb 27, 2026
29 checks passed
@wjones127 wjones127 deleted the yang/optimize-merge branch February 27, 2026 21:42
wjones127 pushed a commit to wjones127/lance that referenced this pull request Mar 4, 2026
1. reuse the readers and avoid reopening them in each iteration
2. read multiple partitions in one read to avoid small IOs

---------

Signed-off-by: BubbleCal <bubble-cal@outlook.com>
Co-authored-by: Lei Xu <lei@lancedb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants