feat: support multi-segment indices in hamming clustering#7758
Merged
Conversation
hamming_clustering_for_ivf_partition and get_ivf_partition_info previously used only the first physical segment of a logical IVF_FLAT index, silently excluding rows covered by delta segments. They now cover all segments, validating that every segment shares the same global IVF centroids and failing otherwise. New _segments variants (Rust) and an index_segments kwarg (Python) restrict the computation to selected segments, mirroring prewarm_index. Also fixes get_ivf_partition_info reporting size 0 on v3 index files by reading sizes from partition storage instead of the IVF model.
Consolidate the three copies of index_segments coercion (hamming clustering wrappers, prewarm_index, ScannerBuilder.with_index_segments) into lance.util._normalize_index_segment_ids, and reject a bare str/UUID argument with a clear TypeError instead of iterating it element-wise.
github-actions
Bot
added
A-python
Contributor
📝 WalkthroughWalkthroughChangesThe change adds shared index-segment ID normalization, extends IVF_FLAT hamming clustering and partition-info APIs with optional segment selection, and implements multi-segment processing and validation in Rust with accompanying tests. Multi-segment IVF_FLAT support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PythonAPI
participant DatasetBinding
participant HammingEngine
participant IVFSegments
PythonAPI->>DatasetBinding: pass normalized index_segments
DatasetBinding->>HammingEngine: select full or explicit segment clustering
HammingEngine->>IVFSegments: open and validate IVF_FLAT segments
IVFSegments-->>HammingEngine: partition row IDs and hash vectors
HammingEngine-->>DatasetBinding: return clustering or partition info
DatasetBinding-->>PythonAPI: return result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/src/index/vector/hamming.rs`:
- Around line 227-228: Update the accumulation buffer initialization near
all_row_ids and all_hashes to use Vec::with_capacity(), estimating the combined
capacity by summing partition_size(partition_id) across the opened segments.
Reserve capacity for both vectors before concatenating, while preserving their
existing element types and accumulation behavior.
🪄 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: c8647382-5ba9-4935-a7fb-1dc9d09fd26f
📒 Files selected for processing (7)
python/python/lance/dataset.pypython/python/lance/lance/__init__.pyipython/python/lance/util.pypython/python/lance/vector.pypython/python/tests/test_vector.pypython/src/dataset.rsrust/lance/src/index/vector/hamming.rs
Comment on lines
+227
to
+228
| let mut all_row_ids: Vec<u64> = Vec::new(); | ||
| let mut all_hashes: Vec<u64> = Vec::new(); |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Preallocate the accumulation buffers. The combined size is estimable by summing partition_size(partition_id) across the opened segments, so reserving capacity avoids repeated reallocations while concatenating.
♻️ Suggested change
- let mut all_row_ids: Vec<u64> = Vec::new();
- let mut all_hashes: Vec<u64> = Vec::new();
+ let estimated_len: usize = segments
+ .iter()
+ .map(|segment| segment.ivf_flat_bin().partition_size(partition_id))
+ .sum();
+ let mut all_row_ids: Vec<u64> = Vec::with_capacity(estimated_len);
+ let mut all_hashes: Vec<u64> = Vec::with_capacity(estimated_len);As per coding guidelines: "Use Vec::with_capacity() when size is known or estimable, and prefer over-estimating capacity to multiple reallocations."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let mut all_row_ids: Vec<u64> = Vec::new(); | |
| let mut all_hashes: Vec<u64> = Vec::new(); | |
| let estimated_len: usize = segments | |
| .iter() | |
| .map(|segment| segment.ivf_flat_bin().partition_size(partition_id)) | |
| .sum(); | |
| let mut all_row_ids: Vec<u64> = Vec::with_capacity(estimated_len); | |
| let mut all_hashes: Vec<u64> = Vec::with_capacity(estimated_len); |
🤖 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/src/index/vector/hamming.rs` around lines 227 - 228, Update the
accumulation buffer initialization near all_row_ids and all_hashes to use
Vec::with_capacity(), estimating the combined capacity by summing
partition_size(partition_id) across the opened segments. Reserve capacity for
both vectors before concatenating, while preserving their existing element types
and accumulation behavior.
Source: Coding guidelines
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
hamming_clustering_for_ivf_partitionandget_ivf_partition_inforesolved the target index withfind(name)— the first physical segment only. On a logical IVF_FLAT index made of multiple segments (delta segments from append-modeoptimize_indices, or distributed builds committed viacommit_existing_index_segments), this silently processed only the oldest segment: appended rows were excluded and cross-segment duplicate pairs were never compared, with no error raised.Both functions now cover all segments of the logical index. Delta segments of one IVF index share centroids, so partition
pdenotes the same centroid region in every segment; the correct clustering unit is the union of partitionp's rows across all segments in a single pairwise pass (representative = min row id, so results are order-independent).Changes
lance-index::vector::hamming): all-segments behavior for both functions. Every selected segment is validated to share byte-identical global IVF centroids; a mismatch (or missing centroids) raises a descriptive error naming the diverging segment UUIDs and partition counts, advising a retrain. Newhamming_clustering_for_ivf_partition_segments/get_ivf_partition_info_segmentsaccept explicit segment UUIDs, following theprewarm_index_segmentsprecedent (feat: support segment selection in pylance prewarm #7677).index_segmentsargument onhamming_clustering_for_ivf_partitionandget_ivf_partition_info(None= all segments, the default). Segment-id normalization used in three places (these wrappers,prewarm_index,ScannerBuilder.with_index_segments) is consolidated intolance.util._normalize_index_segment_ids.Notes
get_ivf_partition_inforeturnedsize: 0for every partition on v3 index files because the loaded IVF model carries no partition lengths. Sizes now come from partition storage viaVectorIndex::partition_size.get_ivf_partition_infonow validates the indexed column isFixedSizeList<UInt8, 8>, a check it previously skipped. This is consistent with the clustering function, which always required 8-byte hashes.Summary by CodeRabbit
New Features
Bug Fixes