Skip to content

feat: cleanup partial idx files when merging distributed vector index#5729

Merged
BubbleCal merged 2 commits intolance-format:mainfrom
yanghua:feat-cleanup-partial-idx-files
Jan 16, 2026
Merged

feat: cleanup partial idx files when merging distributed vector index#5729
BubbleCal merged 2 commits intolance-format:mainfrom
yanghua:feat-cleanup-partial-idx-files

Conversation

@yanghua
Copy link
Copy Markdown
Collaborator

@yanghua yanghua commented Jan 16, 2026

No description provided.

@github-actions github-actions Bot added the enhancement New feature or request label Jan 16, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Code Review - Summary: This PR adds cleanup of temporary partial_* directories after distributed vector index merge operations. P1 Issues - 1. Missing Test Coverage: The new cleanup_partial_vector_dirs function lacks test coverage. This is a file deletion operation that affects the index directory structure. Consider adding a test that creates a mock index directory with partial_* subdirectories, calls finalize_distributed_merge, and verifies the partial directories are removed while other files remain. 2. Potential Race Condition: The cleanup happens after v2_writer.finish().await but before the function returns. If a concurrent process is still reading from partial_* directories (e.g., during centroid extraction at lines 1917-1957), deletion could cause read failures. Consider whether the centroids extraction should happen before cleanup or if there is coordination needed. Minor Observations - The function docstring says it always returns Ok but the signature returns Result. The outer caller ignores errors anyway via if let Err, so this is cosmetic but could be clarified. Overall the approach is reasonable - cleanup failures are logged but do not block index finalization, which is the right tradeoff.

@BubbleCal BubbleCal self-requested a review January 16, 2026 11:07
@BubbleCal BubbleCal merged commit 9dfe8f3 into lance-format:main Jan 16, 2026
26 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants