Implement GraphLinksFormat::CompressedWithVectors#6982
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new GraphLinksFormat::CompressedWithVectors format that extends the existing compressed links format to include embedded quantized vectors alongside link data. The new format enables storing both graph connectivity and quantized vector data in a single file (links_comp_vec.bin), which will support future optimizations in HNSW search operations.
- Adds the new
CompressedWithVectorsformat with serialization and deserialization support - Introduces
GraphLayersWithVectors::for_each_link_with_vector()trait method for iterating over links with their associated quantized vectors - Updates test infrastructure to support quantization and validates the new format across all existing tests
Reviewed Changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lib/segment/src/vector_storage/quantized/quantized_vectors.rs |
Adds get_quantized_vector() method to provide access to quantized vector bytes |
lib/segment/src/index/hnsw_index/graph_links/header.rs |
Defines new header structure for compressed format with vectors |
lib/segment/src/index/hnsw_index/graph_links/serializer.rs |
Implements serialization logic for the new format with embedded vectors |
lib/segment/src/index/hnsw_index/graph_links/view.rs |
Implements deserialization and access methods for the new format |
lib/segment/src/index/hnsw_index/graph_links.rs |
Adds format parameter types and utility methods |
lib/segment/src/index/hnsw_index/graph_layers.rs |
Adds trait for vector-aware link iteration |
lib/segment/src/fixtures/index_fixtures.rs |
Updates test utilities to support quantization |
lib/common/common/src/bitpacking_links.rs |
Modifies pack_links to use in/out parameter for link reordering |
Comments suppressed due to low confidence (1)
lib/segment/src/index/hnsw_index/graph_links.rs:83
- The panic!() provides no context about what went wrong. Consider providing a descriptive error message explaining that CompressedWithVectors requires quantized vectors.
None => panic!(),
|
|
||
| fn get_quantized_vector(&self, _i: u32) -> &[u8] { | ||
| // Possible to implement, but we don't use it | ||
| unreachable!() |
There was a problem hiding this comment.
Using unreachable!() for unimplemented functionality makes it difficult to debug when this code path is accidentally reached. Consider using unimplemented!() or a proper error return instead.
| unreachable!() | |
| unimplemented!("get_quantized_vector is not implemented for QuantizedMultiVectorStorage") |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a new graph-links format variant Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (13)📓 Common learnings📚 Learning: 2025-08-06T09:56:59.311ZApplied to files:
📚 Learning: 2025-05-19T09:25:54.285ZApplied to files:
📚 Learning: 2025-05-08T16:39:07.893ZApplied to files:
📚 Learning: 2025-08-11T00:37:34.065ZApplied to files:
📚 Learning: 2025-08-10T18:30:02.965ZApplied to files:
📚 Learning: 2025-03-29T17:43:28.174ZApplied to files:
📚 Learning: 2025-05-07T09:13:47.781ZApplied to files:
📚 Learning: 2025-06-06T07:52:38.478ZApplied to files:
📚 Learning: 2025-07-11T11:35:21.549ZApplied to files:
📚 Learning: 2025-05-08T10:34:42.223ZApplied to files:
📚 Learning: 2025-06-02T18:10:47.203ZApplied to files:
📚 Learning: 2025-05-19T14:40:20.068ZApplied to files:
🧬 Code Graph Analysis (2)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (4)
lib/common/common/src/bitpacking_links.rs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
🔇 Additional comments (7)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
lib/segment/src/index/hnsw_index/graph_links/view.rs (2)
180-184: Consider adding explicit test-only handlingThe comment indicates this code path is "not intended to be used outside of tests", but there's no compile-time enforcement. Consider either:
- Adding
#[cfg(test)]attribute if truly test-only- Adding a debug assertion or log warning
- Clarifying why this path exists if it's not for production use
205-206: Consider returning Result instead of panickingThe
unimplemented!()calls will cause panics at runtime. Consider returning aResultto handle unsupported formats more gracefully, even though the panic is documented.- CompressionInfo::Uncompressed { .. } => unimplemented!(), - CompressionInfo::Compressed { .. } => unimplemented!(), + CompressionInfo::Uncompressed { .. } => { + panic!("links_with_vectors not supported for Uncompressed format") + } + CompressionInfo::Compressed { .. } => { + panic!("links_with_vectors not supported for Compressed format") + }lib/segment/src/index/hnsw_index/graph_layers.rs (1)
355-366: Consider tracking the conversion limitationThe documentation clearly states that conversion to "compressed with vectors" format is not supported. Since this is a known limitation, consider adding a TODO comment or creating an issue to track this potential future enhancement.
Would you like me to create an issue to track the implementation of conversion to the CompressedWithVectors format?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
Cargo.toml(1 hunks)lib/common/common/benches/bitpacking.rs(1 hunks)lib/common/common/benches/bitpacking_tango.rs(1 hunks)lib/common/common/src/bitpacking_links.rs(4 hunks)lib/quantization/src/encoded_vectors.rs(1 hunks)lib/quantization/src/encoded_vectors_binary.rs(1 hunks)lib/quantization/src/encoded_vectors_pq.rs(1 hunks)lib/quantization/src/encoded_vectors_u8.rs(2 hunks)lib/segment/Cargo.toml(1 hunks)lib/segment/benches/fixture.rs(3 hunks)lib/segment/benches/hnsw_build_asymptotic.rs(3 hunks)lib/segment/benches/hnsw_build_graph.rs(1 hunks)lib/segment/src/fixtures/index_fixtures.rs(3 hunks)lib/segment/src/index/hnsw_index/gpu/gpu_insert_context.rs(1 hunks)lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/gpu_quantization.rs(2 hunks)lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rs(2 hunks)lib/segment/src/index/hnsw_index/gpu/mod.rs(3 hunks)lib/segment/src/index/hnsw_index/graph_layers.rs(18 hunks)lib/segment/src/index/hnsw_index/graph_layers_builder.rs(21 hunks)lib/segment/src/index/hnsw_index/graph_links.rs(5 hunks)lib/segment/src/index/hnsw_index/graph_links/header.rs(1 hunks)lib/segment/src/index/hnsw_index/graph_links/serializer.rs(8 hunks)lib/segment/src/index/hnsw_index/graph_links/view.rs(8 hunks)lib/segment/src/index/hnsw_index/hnsw.rs(1 hunks)lib/segment/src/index/hnsw_index/links_container.rs(2 hunks)lib/segment/src/index/hnsw_index/tests/mod.rs(2 hunks)lib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs(3 hunks)lib/segment/src/vector_storage/quantized/quantized_multivector_storage.rs(1 hunks)lib/segment/src/vector_storage/quantized/quantized_vectors.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (22)
📚 Learning: in the subgraph_connectivity method in lib/segment/src/index/hnsw_index/graph_layers_builder.rs, the...
Learnt from: generall
PR: qdrant/qdrant#6479
File: lib/segment/src/index/hnsw_index/graph_layers_builder.rs:156-159
Timestamp: 2025-05-30T15:26:54.048Z
Learning: In the subgraph_connectivity method in lib/segment/src/index/hnsw_index/graph_layers_builder.rs, the parameter `q` represents the edge removal/failure probability, not the edge retention probability. The logic `if coin_flip < q { continue; }` correctly simulates edge failures for percolation-based connectivity estimation.
Applied to files:
lib/segment/src/index/hnsw_index/hnsw.rslib/segment/benches/fixture.rslib/segment/benches/hnsw_build_graph.rslib/segment/src/index/hnsw_index/gpu/gpu_insert_context.rslib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/gpu_quantization.rslib/segment/src/index/hnsw_index/gpu/mod.rslib/segment/src/index/hnsw_index/links_container.rslib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rslib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rslib/segment/src/index/hnsw_index/tests/mod.rslib/segment/src/index/hnsw_index/graph_layers.rslib/segment/src/index/hnsw_index/graph_links.rslib/segment/src/index/hnsw_index/graph_layers_builder.rs
📚 Learning: in the subgraph_connectivity method in graph_layers_builder.rs, the previous_visited_points vector a...
Learnt from: generall
PR: qdrant/qdrant#6479
File: lib/segment/src/index/hnsw_index/graph_layers_builder.rs:137-182
Timestamp: 2025-05-30T15:26:14.488Z
Learning: In the subgraph_connectivity method in graph_layers_builder.rs, the previous_visited_points vector and visited BitVec entry point marking are automatically re-initialized at the start of each retry loop iteration, so manual clearing is not needed.
Applied to files:
lib/segment/src/index/hnsw_index/hnsw.rslib/segment/benches/fixture.rslib/segment/src/index/hnsw_index/gpu/mod.rslib/common/common/benches/bitpacking.rslib/common/common/src/bitpacking_links.rslib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rslib/segment/src/index/hnsw_index/tests/mod.rslib/segment/src/index/hnsw_index/graph_layers.rslib/segment/src/index/hnsw_index/graph_links/view.rslib/segment/src/index/hnsw_index/graph_links.rslib/segment/src/index/hnsw_index/graph_layers_builder.rslib/segment/src/index/hnsw_index/graph_links/serializer.rs
📚 Learning: in test code for qdrant's query estimator (lib/segment/src/index/query_estimator.rs), simplified id ...
Learnt from: generall
PR: qdrant/qdrant#6854
File: lib/segment/src/index/query_estimator.rs:320-327
Timestamp: 2025-07-11T11:35:21.549Z
Learning: In test code for Qdrant's query estimator (lib/segment/src/index/query_estimator.rs), simplified ID resolution logic using `id.to_string().parse().unwrap()` is acceptable for testing purposes and doesn't need to match production code's `id_tracker.internal_id()` approach. Test code can use mock implementations that serve the testing goals.
Applied to files:
lib/segment/benches/fixture.rslib/segment/benches/hnsw_build_graph.rslib/segment/src/index/hnsw_index/gpu/gpu_insert_context.rslib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/gpu_quantization.rslib/segment/benches/hnsw_build_asymptotic.rslib/common/common/benches/bitpacking_tango.rslib/segment/src/index/hnsw_index/gpu/mod.rslib/segment/src/vector_storage/quantized/quantized_vectors.rslib/common/common/benches/bitpacking.rslib/segment/src/index/hnsw_index/links_container.rslib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rslib/segment/src/vector_storage/quantized/quantized_multivector_storage.rslib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rslib/segment/src/index/hnsw_index/tests/mod.rslib/segment/src/index/hnsw_index/graph_layers.rslib/segment/src/fixtures/index_fixtures.rslib/segment/src/index/hnsw_index/graph_links.rslib/segment/src/index/hnsw_index/graph_layers_builder.rs
📚 Learning: the rand crate version 0.9.0 and higher changed method names from `gen_*` to `random_*` (e.g., `gen_...
Learnt from: coszio
PR: qdrant/qdrant#6528
File: lib/posting_list/src/tests.rs:44-47
Timestamp: 2025-05-15T22:54:30.292Z
Learning: The rand crate version 0.9.0 and higher changed method names from `gen_*` to `random_*` (e.g., `gen_range()` became `random_range()`). Code using rand 0.9.x should use the `random_*` method names, while code using rand 0.8.x and earlier should use the `gen_*` method names.
Applied to files:
lib/segment/benches/hnsw_build_graph.rslib/common/common/benches/bitpacking.rslib/segment/src/index/hnsw_index/links_container.rslib/quantization/src/encoded_vectors_u8.rslib/segment/src/index/hnsw_index/tests/mod.rslib/segment/src/fixtures/index_fixtures.rs
📚 Learning: the rust rand crate version 0.9+ has introduced a top-level `rng()` function that replaces the now-d...
Learnt from: coszio
PR: qdrant/qdrant#6446
File: lib/gridstore/benches/flush_bench.rs:18-18
Timestamp: 2025-04-29T16:48:34.967Z
Learning: The Rust rand crate version 0.9+ has introduced a top-level `rng()` function that replaces the now-deprecated `thread_rng()` function.
Applied to files:
lib/segment/benches/hnsw_build_graph.rslib/segment/src/index/hnsw_index/tests/mod.rslib/segment/src/fixtures/index_fixtures.rs
📚 Learning: the rand crate version 0.9.0 and newer uses method names with `random_*` prefix (like `random_range`...
Learnt from: coszio
PR: qdrant/qdrant#6528
File: lib/posting_list/src/tests.rs:44-47
Timestamp: 2025-05-15T22:54:30.292Z
Learning: The rand crate version 0.9.0 and newer uses method names with `random_*` prefix (like `random_range`), while versions 0.8.x and older use `gen_*` prefix (like `gen_range`). This naming change was introduced in rand 0.9.0-alpha.1.
Applied to files:
lib/segment/benches/hnsw_build_graph.rslib/common/common/benches/bitpacking.rslib/segment/src/index/hnsw_index/links_container.rslib/segment/src/index/hnsw_index/tests/mod.rslib/segment/src/fixtures/index_fixtures.rs
📚 Learning: the rand crate version 0.9.0 and newer uses method names with `random_*` prefix (like `random_range`...
Learnt from: coszio
PR: qdrant/qdrant#6528
File: lib/posting_list/src/tests.rs:44-47
Timestamp: 2025-05-15T22:54:30.292Z
Learning: The rand crate version 0.9.0 and newer uses method names with `random_*` prefix (like `random_range`), while versions 0.8.x and older use `gen_*` prefix (like `gen_range`). This was part of an API redesign in rand 0.9.0 released in February 2024.
Applied to files:
lib/segment/benches/hnsw_build_graph.rslib/common/common/benches/bitpacking.rslib/segment/src/index/hnsw_index/links_container.rslib/segment/src/index/hnsw_index/tests/mod.rslib/segment/src/fixtures/index_fixtures.rs
📚 Learning: in the qdrant codebase, zerocopy crate is extensively used for safe byte-level operations across gpu...
Learnt from: coszio
PR: qdrant/qdrant#6609
File: lib/gridstore/src/blob.rs:46-59
Timestamp: 2025-06-02T18:10:47.203Z
Learning: In the Qdrant codebase, zerocopy crate is extensively used for safe byte-level operations across GPU operations, HNSW indices, memory-mapped structures, and serialization. When implementing Blob trait for generic Vec<T>, using zerocopy's FromBytes and IntoBytes traits is preferred over size_of::<T>() because it guarantees memory layout equals byte representation, making serialization safe and correct.
Applied to files:
lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/gpu_quantization.rslib/quantization/src/encoded_vectors.rslib/segment/src/index/hnsw_index/gpu/mod.rslib/segment/src/vector_storage/quantized/quantized_vectors.rslib/quantization/src/encoded_vectors_binary.rslib/segment/src/index/hnsw_index/graph_links/header.rslib/quantization/src/encoded_vectors_pq.rslib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rslib/segment/src/vector_storage/quantized/quantized_multivector_storage.rslib/segment/src/index/hnsw_index/graph_links/view.rslib/segment/src/fixtures/index_fixtures.rslib/segment/src/index/hnsw_index/graph_links/serializer.rs
📚 Learning: in lib/quantization/cpp/ simd functions, ivanpleshkov prefers to avoid memcpy in favor of direct poi...
Learnt from: IvanPleshkov
PR: qdrant/qdrant#6728
File: lib/quantization/cpp/sse.c:428-431
Timestamp: 2025-07-02T17:08:10.839Z
Learning: In lib/quantization/cpp/ SIMD functions, IvanPleshkov prefers to avoid memcpy in favor of direct pointer casts for type punning, prioritizing potential compiler optimization over strict aliasing rule compliance in performance-critical quantization code.
Applied to files:
lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/gpu_quantization.rslib/quantization/src/encoded_vectors_u8.rslib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rs
📚 Learning: in the qdrant quantization codebase, hardware counters (hw_counter.cpu_counter()) are used to measur...
Learnt from: IvanPleshkov
PR: qdrant/qdrant#6728
File: lib/quantization/src/encoded_vectors_binary.rs:810-810
Timestamp: 2025-07-02T17:10:13.847Z
Learning: In the Qdrant quantization codebase, hardware counters (hw_counter.cpu_counter()) are used to measure vector data access from storage, not computational cost. With asymmetric binary quantization where query vectors can be longer than storage vectors, the counter should still track vector_data.len() to maintain consistent measurement of storage access patterns, not query processing overhead.
Applied to files:
lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/gpu_quantization.rslib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rslib/segment/src/fixtures/index_fixtures.rs
📚 Learning: in lib/quantization/cpp/sse.c asymmetric binary quantization functions, the argument order in _mm_se...
Learnt from: IvanPleshkov
PR: qdrant/qdrant#6728
File: lib/quantization/cpp/sse.c:122-127
Timestamp: 2025-07-02T16:42:22.247Z
Learning: In lib/quantization/cpp/sse.c asymmetric binary quantization functions, the argument order in _mm_set_epi32 calls is intentionally designed to match query pointer offsets with corresponding bit shift factors. The "numbers" refer to logical indices that correspond to weighting factors (1, 2, 4, 8, etc. representing bit shifts), not SIMD register positions.
Applied to files:
lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/gpu_quantization.rslib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rs
📚 Learning: in the bitpacking crate, the bitpacker::decompress_strictly_sorted function takes an option
Learnt from: coszio
PR: qdrant/qdrant#6528
File: lib/posting_list/src/view.rs:118-118
Timestamp: 2025-05-19T14:40:20.068Z
Learning: In the bitpacking crate, the BitPacker::decompress_strictly_sorted function takes an Option<PointOffsetType> as its first parameter, which means using checked_sub(1) without unwrapping is intentional and correct.
Applied to files:
lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/gpu_quantization.rslib/common/common/benches/bitpacking_tango.rslib/common/common/benches/bitpacking.rslib/segment/src/index/hnsw_index/graph_links/header.rslib/common/common/src/bitpacking_links.rslib/segment/src/index/hnsw_index/graph_links/view.rslib/segment/src/index/hnsw_index/graph_links/serializer.rs
Learnt from: coszio
PR: qdrant/qdrant#6528
File: lib/posting_list/src/view.rs:118-118
Timestamp: 2025-05-19T14:40:20.068Z
Learning: In the bitpacking crate, the BitPacker::decompress_strictly_sorted function takes an Option<PointOffsetType> as its first parameter, which means using checked_sub(1) without unwrapping is intentional and correct.
lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/gpu_quantization.rslib/common/common/benches/bitpacking_tango.rslib/common/common/benches/bitpacking.rslib/segment/src/index/hnsw_index/graph_links/header.rslib/common/common/src/bitpacking_links.rslib/segment/src/index/hnsw_index/graph_links/view.rslib/segment/src/index/hnsw_index/graph_links/serializer.rs📚 Learning: in the posting_list crate's postingchunk struct, avoid adding extra fields like storing computed chu...
Learnt from: coszio
PR: qdrant/qdrant#6565
File: lib/posting_list/src/builder.rs:63-67
Timestamp: 2025-05-26T14:47:23.505Z
Learning: In the posting_list crate's PostingChunk struct, avoid adding extra fields like storing computed chunk_bits to prevent struct bloat and maintain mmap compatibility. The bit calculations from offsets are inexpensive compared to the memory and compatibility benefits of keeping the struct minimal.
Applied to files:
lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/gpu_quantization.rslib/common/common/benches/bitpacking_tango.rslib/common/common/benches/bitpacking.rslib/segment/src/index/hnsw_index/graph_links/header.rslib/common/common/src/bitpacking_links.rs
📚 Learning: in the qdrant codebase, storage backends may have different error handling approaches. specifically,...
Learnt from: timvisee
PR: qdrant/qdrant#6444
File: lib/segment/src/index/field_index/map_index/immutable_map_index.rs:373-375
Timestamp: 2025-05-08T10:34:42.223Z
Learning: In the Qdrant codebase, storage backends may have different error handling approaches. Specifically, the mmap variant of `remove_point` method does not return errors, while the RocksDB variant does propagate errors using the `?` operator. These differences are intentional by design.
Applied to files:
lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/gpu_quantization.rs
📚 Learning: in the asyncrawscorerimpl implementation, the unwrap() call on read_vectors_async results is intenti...
Learnt from: xzfc
PR: qdrant/qdrant#6245
File: lib/segment/src/vector_storage/async_raw_scorer.rs:48-56
Timestamp: 2025-04-22T23:19:51.232Z
Learning: In the AsyncRawScorerImpl implementation, the unwrap() call on read_vectors_async results is intentional, with an explanatory comment noting that this experimental feature is meant to crash rather than silently fall back to synchronous implementation.
Applied to files:
lib/segment/benches/hnsw_build_asymptotic.rslib/segment/src/index/hnsw_index/gpu/mod.rslib/segment/src/index/hnsw_index/links_container.rslib/segment/src/fixtures/index_fixtures.rs
📚 Learning: in asyncrawscorerimpl, the unwrap() call after read_vectors_async is intentional. the io_uring featu...
Learnt from: xzfc
PR: qdrant/qdrant#6245
File: lib/segment/src/vector_storage/async_raw_scorer.rs:48-56
Timestamp: 2025-04-22T23:19:51.232Z
Learning: In AsyncRawScorerImpl, the unwrap() call after read_vectors_async is intentional. The io_uring feature is experimental, and the code is designed to panic rather than silently fall back to a synchronous implementation if it fails, directing users to use the default IO implementation instead.
Applied to files:
lib/segment/benches/hnsw_build_asymptotic.rslib/segment/src/index/hnsw_index/gpu/mod.rslib/segment/src/index/hnsw_index/links_container.rslib/segment/src/fixtures/index_fixtures.rslib/segment/src/index/hnsw_index/graph_layers_builder.rs
📚 Learning: functions gated with `#[cfg(feature = "testing")]` within `#[cfg(test)]` modules are safe from compi...
Learnt from: generall
PR: qdrant/qdrant#6635
File: lib/segment/src/index/field_index/full_text_index/text_index.rs:784-832
Timestamp: 2025-06-14T20:35:10.603Z
Learning: Functions gated with `#[cfg(feature = "testing")]` within `#[cfg(test)]` modules are safe from compilation issues since both the function and its call sites are restricted to test builds. The additional feature gate is often used for optional heavy test utilities.
Applied to files:
lib/segment/src/index/hnsw_index/gpu/mod.rslib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rslib/segment/src/index/hnsw_index/tests/mod.rslib/segment/src/index/hnsw_index/graph_links.rslib/segment/src/index/hnsw_index/graph_layers_builder.rs
📚 Learning: in the qdrant codebase, using `pub(super)` visibility is preferred when fields need to be accessed b...
Learnt from: timvisee
PR: qdrant/qdrant#6503
File: lib/segment/src/index/field_index/geo_index/mmap_geo_index.rs:60-74
Timestamp: 2025-05-12T12:54:27.872Z
Learning: In the Qdrant codebase, using `pub(super)` visibility is preferred when fields need to be accessed by sibling modules (particularly for index type conversions), as it provides the necessary access without bloating the interface with numerous getters.
Applied to files:
lib/quantization/src/encoded_vectors_binary.rslib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rs
📚 Learning: in volatile sparse vector storage implementations, the `total_sparse_size` field should be updated w...
Learnt from: timvisee
PR: qdrant/qdrant#6569
File: lib/segment/src/vector_storage/sparse/volatile_sparse_vector_storage.rs:56-76
Timestamp: 2025-05-21T12:52:15.229Z
Learning: In volatile sparse vector storage implementations, the `total_sparse_size` field should be updated when vectors are inserted, updated, or deleted to maintain an accurate count of the total number of non-zero elements across all vectors.
Applied to files:
lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rs
📚 Learning: the `populate()` method in `quantizedmmapstorage` correctly returns void instead of a `result` becau...
Learnt from: generall
PR: qdrant/qdrant#6323
File: lib/segment/src/vector_storage/quantized/quantized_mmap_storage.rs:12-16
Timestamp: 2025-04-07T23:31:22.515Z
Learning: The `populate()` method in `QuantizedMmapStorage` correctly returns void instead of a `Result` because it directly implements the `Madviseable` trait which defines `populate(&self)` without a return type. Adding an unnecessary `Ok(())` return would trigger Clippy warnings about unnecessary Result wrapping.
Applied to files:
lib/segment/src/vector_storage/quantized/quantized_multivector_storage.rslib/segment/src/index/hnsw_index/graph_layers.rs
📚 Learning: the `populate()` method in `quantizedmmapstorage` does not return a `result` because the underlying ...
Learnt from: generall
PR: qdrant/qdrant#6323
File: lib/segment/src/vector_storage/quantized/quantized_mmap_storage.rs:12-16
Timestamp: 2025-04-07T23:31:22.515Z
Learning: The `populate()` method in `QuantizedMmapStorage` does not return a `Result` because the underlying `mmap.populate()` method doesn't return a Result either. Adding an unnecessary `Ok(())` would cause Clippy to complain.
Applied to files:
lib/segment/src/vector_storage/quantized/quantized_multivector_storage.rslib/segment/src/index/hnsw_index/graph_layers.rs
📚 Learning: in the qdrant codebase, `debug_assert!` is preferred over runtime checks that would panic in release...
Learnt from: generall
PR: qdrant/qdrant#6274
File: lib/segment/src/vector_storage/sparse/mmap_sparse_vector_storage.rs:233-233
Timestamp: 2025-03-29T17:43:28.174Z
Learning: In the qdrant codebase, `debug_assert!` is preferred over runtime checks that would panic in release builds to ensure production stability, even when validating conditions like vector sorting.
Applied to files:
lib/segment/src/index/hnsw_index/tests/mod.rslib/segment/src/fixtures/index_fixtures.rs
🧬 Code Graph Analysis (6)
lib/common/common/benches/bitpacking_tango.rs (1)
lib/common/common/src/bitpacking_links.rs (1)
std(269-272)
lib/common/common/benches/bitpacking.rs (1)
lib/common/common/src/bitpacking_links.rs (1)
std(269-272)
lib/segment/src/index/hnsw_index/tests/mod.rs (6)
lib/segment/src/index/hnsw_index/graph_layers.rs (2)
num_vectors(600-602)distance(611-611)lib/segment/src/fixtures/index_fixtures.rs (2)
distance(37-37)new(26-69)lib/segment/src/index/hnsw_index/graph_layers_builder.rs (2)
ef_construct(254-256)new(304-319)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(47-65)lib/segment/src/vector_storage/quantized/quantized_query_scorer.rs (1)
new(29-55)lib/segment/src/vector_storage/quantized/quantized_scorer_builder.rs (1)
new(36-54)
lib/segment/src/index/hnsw_index/graph_layers.rs (6)
lib/segment/src/index/hnsw_index/graph_links.rs (4)
for_each_link(184-191)format(166-174)links(194-196)load_from_file(150-160)lib/segment/src/index/hnsw_index/graph_layers_builder.rs (4)
for_each_link(55-66)get_visited_list_from_pool(51-53)hnsw_m(249-251)new(304-319)lib/segment/src/index/hnsw_index/graph_links/view.rs (1)
links(154-185)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(47-65)lib/segment/src/index/hnsw_index/graph_layers_healer.rs (1)
new(27-56)lib/segment/src/index/hnsw_index/tests/mod.rs (2)
create_graph_layer_builder_fixture(14-43)create_graph_layer_fixture(46-68)
lib/segment/src/fixtures/index_fixtures.rs (5)
lib/segment/src/vector_storage/dense/volatile_dense_vector_storage.rs (2)
new_volatile_dense_vector_storage(35-37)new(50-58)lib/segment/src/vector_storage/quantized/quantized_scorer_builder.rs (1)
new(36-54)lib/segment/src/vector_storage/quantized/quantized_vectors.rs (1)
create(316-420)lib/segment/src/index/hnsw_index/point_scorer.rs (1)
new_internal(131-164)lib/segment/src/vector_storage/quantized/quantized_query_scorer.rs (1)
new_internal(59-74)
lib/segment/src/index/hnsw_index/graph_links/serializer.rs (5)
lib/segment/src/fixtures/index_fixtures.rs (1)
quantized_vectors(75-77)lib/segment/src/index/hnsw_index/mod.rs (1)
new(30-32)lib/segment/src/index/hnsw_index/graph_links.rs (2)
format(166-174)links(194-196)lib/segment/src/index/hnsw_index/graph_links/view.rs (2)
sorted_count(257-263)links(154-185)lib/common/common/src/bitpacking_links.rs (1)
pack_links(23-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: storage-compat-test
- GitHub Check: lint
- GitHub Check: test-consensus-compose
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-consistency
- GitHub Check: integration-tests
- GitHub Check: e2e-tests
- GitHub Check: integration-tests-consensus
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: rust-tests (ubuntu-latest)
🔇 Additional comments (66)
Cargo.toml (1)
224-224: LGTM!Adding the
integer-encodingdependency to support the new graph links format is appropriate and necessary for the varint encoding functionality.lib/segment/Cargo.toml (1)
47-47: LGTM!Properly references the workspace dependency for integer encoding functionality needed in the segment library.
lib/quantization/src/encoded_vectors.rs (1)
50-51: LGTM!The new
get_quantized_vectormethod provides necessary access to raw quantized vector bytes for the new graph links format. The method signature and documentation are clear and appropriate.lib/common/common/benches/bitpacking.rs (1)
79-82: LGTM!The changes to pass a mutable reference to
pack_linksand use.collect_vec()are consistent with the refactoring for the new link packing functionality. The benchmark logic remains correct.lib/segment/src/index/hnsw_index/hnsw.rs (1)
613-617: LGTM!The change to parameterize the graph links format with quantized vectors reference enables the new
CompressedWithVectorsformat while maintaining backward compatibility. This properly integrates the quantized vector data into the graph serialization process.lib/segment/benches/fixture.rs (3)
12-12: LGTM - Import updated for new parameterized format.The import change from
GraphLinksFormattoGraphLinksFormatParamaligns with the introduction of the newCompressedWithVectorsformat that supports optional quantized vectors.
43-49: LGTM - Constructor updated with quantization flag.The addition of the
falseboolean argument toTestRawScorerProducer::newcorrectly disables quantization in benchmark fixtures, which is appropriate for performance testing contexts.
82-82: LGTM - Format parameter updated consistently.The usage of
GraphLinksFormatParam::Plaininstead ofGraphLinksFormat::Plainis consistent with the new parameterized format handling introduced in this PR.lib/segment/src/index/hnsw_index/gpu/mod.rs (3)
108-108: LGTM - Import updated for new parameterized format.Consistent with other files, the import is updated from
GraphLinksFormattoGraphLinksFormatParamto support the new format with optional quantized vectors.
130-130: LGTM - Test fixture constructor updated.The addition of the
falsequantization flag toTestRawScorerProducer::newis consistent with the updated constructor signature and appropriate for GPU test scenarios.
206-209: LGTM - Graph conversion methods updated consistently.Both calls to
into_graph_layers_ramcorrectly useGraphLinksFormatParam::Plaininstead of the oldGraphLinksFormat::Plain, maintaining consistency with the new parameterized format API.lib/segment/benches/hnsw_build_graph.rs (1)
21-22: LGTM - Benchmark fixture updated with quantization flag.The addition of the
falsequantization flag to theTestRawScorerProducer::newcall is consistent with the updated constructor signature and appropriate for benchmark scenarios where quantization should be disabled.lib/segment/src/vector_storage/quantized/quantized_multivector_storage.rs (1)
296-299: LGTM - Trait method implementation with clear limitations.The implementation of
get_quantized_vectorwithunreachable!()is appropriate here since multivector quantized storage has a different structure where individual vectors can't be easily accessed as single byte slices. The comment clearly explains this limitation.lib/segment/src/index/hnsw_index/links_container.rs (2)
276-276: LGTM - Test fixture updated with quantization flag.The addition of the
falsequantization flag toTestRawScorerProducer::newin the test is consistent with the updated constructor signature and appropriate for test scenarios.
403-403: LGTM - Test fixture updated consistently.The second test function also correctly adds the
falsequantization flag to theTestRawScorerProducer::newcall, maintaining consistency across all test cases.lib/segment/src/index/hnsw_index/gpu/gpu_insert_context.rs (1)
521-527: LGTM! Consistent with quantization support addition.The addition of the
falseparameter toTestRawScorerProducer::newcorrectly disables quantization in the test context, which is appropriate for GPU insertion context tests. This change aligns with the broader refactoring to support optional quantized vectors.lib/segment/benches/hnsw_build_asymptotic.rs (3)
88-89: LGTM! Consistent quantization parameter addition.The addition of
falseas the quantization parameter toTestRawScorerProducer::newis appropriate for benchmarks, ensuring consistent and predictable performance measurements without quantization overhead.
106-107: LGTM! Consistent quantization parameter addition.Same as above - the quantization parameter is correctly set to
falsefor benchmarking purposes.
124-125: LGTM! Consistent quantization parameter addition.The quantization parameter is correctly set to
falsefor the third benchmark instance.lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/gpu_quantization.rs (2)
6-6: LGTM! Trait import refactoring.The change from
EncodedVectorstoEncodedVectors as _indicates this is now being imported as a trait for its methods rather than as a concrete type, which is consistent with the API refactoring mentioned in the AI summary.
246-247: LGTM! Method signature update handled correctly.The change from
get_quantized_vectortoget_quantized_vector_offset_and_codewith tuple destructuring correctly adapts to the new API that returns both offset and code. The code correctly extracts only the offset part that's needed for the buffer upload.lib/common/common/benches/bitpacking_tango.rs (1)
93-98: LGTM! Correctly adapts to updatedpack_linkssignature.The changes from
.collect()to.collect_vec()and passing a mutable reference to the iterator result correctly adapt to the updatedpack_linksfunction signature that now accepts a mutable slice instead of owning aVec<u32>. This maintains the benchmark's functionality while aligning with the API changes.lib/quantization/src/encoded_vectors_binary.rs (1)
868-871: LGTM! Proper trait method implementation for standardized access.Moving
get_quantized_vectorto theEncodedVectorstrait implementation standardizes access to quantized vector bytes across all encoded vector types. The implementation correctly delegates to the underlying storage with proper size calculation.lib/quantization/src/encoded_vectors_pq.rs (1)
556-559: LGTM! Consistent trait implementation matching the standardization pattern.The implementation correctly uses
vector_division.len()as the quantized vector size, which is appropriate for PQ encoding where each chunk is represented by a single centroid index byte. This maintains consistency with the other encoded vector types.lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rs (2)
16-16: LGTM! Standard unused import suppression pattern.The renamed import
EncodedVectors as _is a standard Rust idiom to keep the trait in scope for its methods while suppressing unused import warnings.
271-278: API adaptation looks correct.The changes properly adapt to the updated quantized vector API where
get_quantized_vector_offset_and_codereturns a tuple(offset, code)and the code correctly accesses the vector data via.1.lib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs (3)
57-57: Good test coverage addition.Adding test coverage for the new
CompressedWithVectorsformat ensures the new functionality works correctly and maintains behavioral consistency with existing formats.
68-76: LGTM! Improved readability with multi-line formatting.The reformatted function call improves readability by placing each parameter on its own line.
90-91: Correct parameterization for the new format.The change to use
format.with_param_for_tests(vector_holder.quantized_vectors())properly provides the quantized vectors needed by theCompressedWithVectorsformat variant.lib/segment/src/index/hnsw_index/graph_links/header.rs (2)
33-47: Well-designed header structure extension.The new
HeaderCompressedWithVectorsstruct properly extends the compressed format header with the necessaryvector_size_bytesfield while maintaining consistency with existing patterns including alignment, derives, and padding.
50-50: Appropriate version constant for format identification.The new version constant properly increments from the existing compressed format version while maintaining the established naming convention and bit pattern.
lib/quantization/src/encoded_vectors_u8.rs (2)
265-270: Improved API clarity with explicit method naming.Renaming to
get_quantized_vector_offset_and_codemakes the API more self-documenting by clearly indicating that the method returns both offset and vector code data.
468-471: Good API design with complementary access methods.The new
get_quantized_vectortrait method provides clean access to raw vector bytes, complementing the detailedget_quantized_vector_offset_and_codemethod. This supports both use cases effectively.lib/common/common/src/bitpacking_links.rs (3)
21-25: Good API improvement for reusability.Changing from consuming
Vec<u32>to mutable slice&mut [u32]makes the function more flexible and allows callers to reuse the input data. The updated documentation clearly explains the in/out parameter behavior.
62-65: Correct delta-encoding restoration.The code properly undoes the delta-encoding by cumulatively adding deltas back to restore original values in the sorted portion, maintaining the in/out parameter contract.
239-255: Comprehensive test coverage for API changes.The test updates properly validate both the packing correctness and the in/out parameter behavior by testing against both the original data and the modified input slice.
lib/segment/src/index/hnsw_index/tests/mod.rs (2)
14-43: LGTM! Test fixture properly extended with quantization support.The changes correctly propagate the
use_quantizationparameter toTestRawScorerProducer::newand maintain backward compatibility.
45-68: LGTM! Graph layer fixture correctly uses the new format parameter API.The function properly converts the graph layers builder using the new
GraphLinksFormatParamwith optional quantized vectors.lib/segment/src/index/hnsw_index/graph_links/serializer.rs (3)
47-65: LGTM! Clean abstraction for quantized vector access.The constructor properly transforms the
QuantizedVectorsreference into a function pointer, providing a flexible interface for the inner implementation.
128-147: LGTM! Robust serialization with proper validation.The implementation correctly validates quantized vector sizes and returns an appropriate error on mismatch, ensuring data consistency.
260-277: LGTM! Header serialization properly handles the new format.The implementation correctly writes the
HeaderCompressedWithVectorswith the vector size information.lib/segment/src/fixtures/index_fixtures.rs (2)
75-101: LGTM! Scorer methods properly updated for quantization support.The methods correctly pass the optional quantized vectors to the
FilteredScorerconstructors.
43-62: Temporary directory drop is safe with always_ram=trueThe code paths under
always_ram = Some(true)build and use purely in-memory storage, and all filesystem writes (save_toandatomic_save_json) occur before the temp directory is dropped at the end of the statement. After creation,storage_implnever reads from disk, so it’s safe to drop the TempDir immediately.lib/segment/src/index/hnsw_index/graph_links.rs (3)
58-121: LGTM! Well-designed format parameter abstraction.The dual approach with
with_param_for_tests(panics on misconfiguration) andwith_param(graceful downgrade) provides good safety for both testing and production scenarios.
198-207: LGTM! Clean API for accessing links with vectors.The method provides a clear interface for iterating over link-vector pairs.
285-322: LGTM! Comprehensive test helpers for the new format.The test utilities properly validate that quantized vectors are correctly associated with their corresponding links.
lib/segment/src/index/hnsw_index/graph_layers_builder.rs (3)
55-66: LGTM! Method rename improves clarity.The rename from
links_maptofor_each_linkbetter describes the method's behavior.
185-246: LGTM! Graph layers conversion properly handles the new format.The methods correctly propagate the
GraphLinksFormatParamand handle potential serialization errors.
704-908: LGTM! Comprehensive test coverage for the new format.The tests properly exercise the new
CompressedWithVectorsformat variant and maintain backward compatibility with existing formats.lib/segment/src/index/hnsw_index/graph_links/view.rs (8)
1-1: LGTM!The addition of
Zipto the imports is appropriate for the newLinksWithVectorsIteratortype.
7-7: LGTM!The
integer_encodingcrate import is correctly used for varint encoding/decoding in the new compressed format.
16-18: LGTM!The new header imports are properly structured for the
CompressedWithVectorsformat support.
36-40: LGTM!The
LinksWithVectorsIteratortype is well-defined with clear documentation explaining its purpose.
54-60: LGTM!The
CompressedWithVectorsvariant extends the existingCompressedvariant appropriately with the additionalvector_size_bytesfield.
71-71: LGTM!The load method correctly handles the new format variant.
122-152: LGTM!The
load_compressed_with_vectorsmethod correctly implements the loading logic for the new format, following the same pattern asload_compressedwith appropriate adjustments for the additional vector data.
256-263: LGTM!The test-only
sorted_countmethod correctly handles all compression formats.lib/segment/src/index/hnsw_index/graph_layers.rs (9)
19-19: LGTM!The import of
GraphLinksFormatParamis necessary for the new format support.
31-31: LGTM!The constant follows the existing naming convention and clearly indicates the file's purpose.
53-53: Good naming improvement!The rename from
links_maptofor_each_linkbetter describes the method's behavior and follows Rust naming conventions.Also applies to: 80-80, 146-146, 183-183, 214-219
200-207: LGTM!The new trait is well-documented and provides the necessary interface for iterating over links with their associated vectors.
226-235: LGTM!The trait implementation correctly delegates to the underlying
links_with_vectorsmethod.
307-309: LGTM!The path resolution for the new format is correctly implemented.
342-346: LGTM!The loading priority correctly favors the most advanced format first (CompressedWithVectors → Compressed → Plain).
397-407: LGTM!The test-only
compress_rammethod is correctly updated to useGraphLinksFormatParam.
454-454: Excellent test coverage!The tests are comprehensively updated to cover the new
CompressedWithVectorsformat across all test scenarios. The parameterized tests ensure the new format behaves consistently with existing formats.Also applies to: 463-469, 476-482, 520-520, 533-547, 567-567, 576-585
b2b9ec9 to
34bcd98
Compare
| let quantized_vectors = use_quantization.then(|| { | ||
| QuantizedVectors::create( | ||
| &storage, | ||
| &ScalarQuantizationConfig { |
There was a problem hiding this comment.
nit: new hnsw layout is mostly about binary quantization
There was a problem hiding this comment.
Some of the tests are too fragile to changes in accuracy. So, I prefer to use less aggressive quantization for them rather than tuning test parameters making them too slow¹ or too complicated².
¹ if using the same parameters for both quantized and non-quantized variants of the same test
² if choosing parameters based on whether the quantization is used in a test variant
| GraphLinksFormatParam::Plain => GraphLinksFormatParam::Plain, | ||
| GraphLinksFormatParam::Compressed => GraphLinksFormatParam::Compressed, | ||
| GraphLinksFormatParam::CompressedWithVectors(qv) => { | ||
| get_quantized_vector = |i| qv.get_quantized_vector(i); |
There was a problem hiding this comment.
I would try to make it universal, and not just quantized
| pub(super) fn new_inner<'a>( | ||
| mut edges: Vec<Vec<Vec<PointOffsetType>>>, | ||
| format: GraphLinksFormat, | ||
| format_param: GraphLinksFormatParam<&dyn Fn(PointOffsetType) -> &'a [u8]>, |
There was a problem hiding this comment.
since we actually want to only have function inside enum and nothing else, let's try to avoid &dyn Fn as a generic param
| links.write_varint(raw_links.len() as u64).unwrap(); | ||
|
|
||
| pack_links(&mut links, &mut raw_links, bits_per_unsorted, sorted_count); | ||
| for i in raw_links { |
There was a problem hiding this comment.
We likely need an alignment here, in case we will store non-byte version of vectors here
| bits_per_unsorted: u8, | ||
| }, | ||
| CompressedWithVectors { | ||
| compressed_links: &'a [u8], |
There was a problem hiding this comment.
-> compressed_links_and_vectors
…r_offset_and_code} Reason: it was inconsistent with other implementations of get_quantized_vector().
e0b4321 to
2478cd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
lib/segment/src/index/hnsw_index/graph_links/serializer.rs (2)
136-144: Unwraps on Vec writes are acceptable; add a brief doc noteCalling write_varint/write_zeros on a Vec won’t fail in practice. Keeping unwrap is fine, but a single inline comment or a one-liner doc note at the method level helps prevent regressions.
254-258: serialize_to_writer unwrap on Vec target is fineUnwrapping here is acceptable because writing to an in-memory Vec doesn’t fail. A short “Vec write can’t fail” note is sufficient.
🧹 Nitpick comments (14)
lib/segment/src/vector_storage/quantized/quantized_vectors.rs (2)
163-183: Good addition; consider documenting multivector unsupported semanticsThe layout accessor is correct and aligns with storage-specific layout() impls. For multivector, you return an error; that’s fine.
Minor: document the exact semantics (bytes per vector, alignment) and that multivector is unsupported (returns error), to make the contract explicit to callers.
/// Get layout for a single quantized vector. /// /// I.e. the size of a single vector in bytes, and the required alignment. +/// +/// Note: +/// - Only single-vector storages are supported. For multivector storages this +/// returns an error. pub fn get_quantized_vector_layout(&self) -> OperationResult<Layout> {
185-202: Panics on multivector: document and add a debug assertionPanicking for multivector here is acceptable if this is a hot path, but please make the contract explicit and add a debug assertion to aid test/dev builds.
- pub fn get_quantized_vector(&self, id: PointOffsetType) -> &[u8] { + /// Returns raw quantized bytes for `id`. + /// + /// Panics if called on a multivector storage. Use `is_multivector()` or + /// `get_quantized_vector_layout()` to gate calls appropriately. + pub fn get_quantized_vector(&self, id: PointOffsetType) -> &[u8] { + debug_assert!( + !self.is_multivector(), + "get_quantized_vector() is unsupported for multivector storage" + ); match &self.storage_impl { QuantizedVectorStorage::ScalarRam(storage) => storage.get_quantized_vector(id), QuantizedVectorStorage::ScalarMmap(storage) => storage.get_quantized_vector(id), QuantizedVectorStorage::PQRam(storage) => storage.get_quantized_vector(id), QuantizedVectorStorage::PQMmap(storage) => storage.get_quantized_vector(id), QuantizedVectorStorage::BinaryRam(storage) => storage.get_quantized_vector(id), QuantizedVectorStorage::BinaryMmap(storage) => storage.get_quantized_vector(id), QuantizedVectorStorage::ScalarRamMulti(_) | QuantizedVectorStorage::ScalarMmapMulti(_) | QuantizedVectorStorage::PQRamMulti(_) | QuantizedVectorStorage::PQMmapMulti(_) | QuantizedVectorStorage::BinaryRamMulti(_) | QuantizedVectorStorage::BinaryMmapMulti(_) => { panic!("Cannot get quantized vector from multivector storage"); } } }lib/segment/src/index/hnsw_index/tests/mod.rs (1)
45-68: Minor test ergonomics: consider derivinguse_quantizationfromformatSince quantized vectors are only used by vector-aware formats, consider deriving
use_quantizationasformat.is_with_vectors()here to avoid parameter mismatches in tests. If you prefer explicit control, current approach is fine.-pub(crate) fn create_graph_layer_fixture<R: Rng + ?Sized>( +pub(crate) fn create_graph_layer_fixture<R: Rng + ?Sized>( num_vectors: usize, m: usize, dim: usize, format: GraphLinksFormat, use_heuristic: bool, - use_quantization: bool, distance: Distance, rng: &mut R, ) -> (TestRawScorerProducer, GraphLayers) { - let (vector_holder, graph_layers_builder) = create_graph_layer_builder_fixture( + let use_quantization = format.is_with_vectors(); + let (vector_holder, graph_layers_builder) = create_graph_layer_builder_fixture( num_vectors, m, dim, use_heuristic, use_quantization, distance, rng, ); let graph_layers = graph_layers_builder .into_graph_layers_ram(format.with_param(vector_holder.quantized_vectors())); (vector_holder, graph_layers) }lib/segment/src/fixtures/index_fixtures.rs (1)
26-33: Optional quantization in fixture: sensible; add a brief note on TempDir lifetimeThe RAM-only scalar quantization path is appropriate for tests. Since
create()writes to the provided temp path during construction, droppingTempDirimmediately is safe here, but consider a short comment clarifying thatsave_tohappens before the directory is dropped (to avoid future regressions if behavior changes).- let quantized_vectors = use_quantization.then(|| { + // Note: QuantizedVectors::create will write files under this temporary path + // during construction; since we force an in-RAM storage (always_ram = true), + // no subsequent filesystem access is needed and it's safe to drop the TempDir + // right after creation. + let quantized_vectors = use_quantization.then(|| { QuantizedVectors::create( &storage, &ScalarQuantizationConfig {Also applies to: 43-63
lib/segment/src/index/hnsw_index/graph_links/header.rs (1)
33-49: HeaderCompressedWithVectors: naming and layout nits
- Naming: total_links_bytes is misleading here, as the neighbors region now contains varint + padding + vectors + compressed links. To align with HeaderCompressed and avoid confusion, prefer total_neighbors_bytes.
- Layout: vector_alignment as u8 is fine in practice, but if alignments >255 ever appear, this would overflow. If that’s impossible by construction, add a brief doc note stating the invariant.
- Padding: zero_padding [u8; 4] is okay for 8-byte alignment, but consider a static assertion on struct size/alignment for on-disk stability.
Suggested renames:
- pub(super) total_links_bytes: LittleU64, + pub(super) total_neighbors_bytes: LittleU64,And update the serializer accordingly (see serializer.rs comment).
Would you like me to prepare a follow-up patch that updates the field name across header and serializer (and view if applicable) and adds a compile-time size/alignment assertion?
lib/segment/src/index/hnsw_index/graph_links/serializer.rs (3)
156-165: Improve error context for vector size mismatchInclude expected and actual sizes to speed up debugging.
- if vector.len() != vector_layout.size() { - return Err(OperationError::service_error("Vector size mismatch")); - } + let expected = vector_layout.size(); + let actual = vector.len(); + if actual != expected { + return Err(OperationError::service_error(format!( + "Vector size mismatch: expected {expected}, got {actual}" + ))); + }
166-171: Binary layout note: Vectors precede compressed linksCurrent on-disk order per node: L (varint), padding, V (vectors), then c (compressed links). Ensure any external docs/comments referencing “L, c, Q” are updated to avoid confusion.
293-316: HeaderCompressedWithVectors: field naming alignment with header.rsIf you adopt total_neighbors_bytes in the header, mirror it here:
- total_links_bytes: LittleU64::new(self.neighbors.len() as u64), + total_neighbors_bytes: LittleU64::new(self.neighbors.len() as u64),Also, the u8 cast for alignment is fine with an expect; if we keep u8, consider a brief note explaining typical alignment ranges (<64).
lib/segment/src/index/hnsw_index/graph_links.rs (2)
100-145: with_param/with_param_for_tests ergonomics
- with_param_for_tests: consider giving panic a message for quicker diagnosis.
- None => panic!(), + None => panic!("CompressedWithVectors selected in tests but no vectors provided"),
- with_param downgrading to Compressed when vectors are absent is sensible for production paths.
401-412: Doc nits: state the actual per-node layout orderIn comments/docs, clarify the per-node order is L (varint), padding, vectors, compressed links (not L, c, vectors).
lib/segment/src/index/hnsw_index/graph_links/view.rs (4)
17-19: Unify header imports for consistencyPrefer importing all header items from the same path (super::header) as done for the compressed/plain variants. This avoids mixed absolute/relative paths in a single file.
For example:
use super::header::{ HEADER_VERSION_COMPRESSED, HeaderCompressed, HeaderPlain, HEADER_VERSION_COMPRESSED_WITH_VECTORS, HeaderCompressedWithVectors, };
159-206: CompressedWithVectors loader looks solid; minor naming and alignment note
- Alignment handling using next_multiple_of for the neighbors region is correct and matches vector_alignment requirements.
- Naming: get_slice::(..., header.total_links_bytes.get()) suggests “only links,” but this region also contains vectors. If feasible, consider renaming the header field to better reflect “compressed_links_and_vectors” (echoing the suggestion from generall).
If renaming the header field is out of scope, consider adding a brief comment here explaining that total_links_bytes includes both the vector and compressed-link payloads for each node block.
241-295: Add debug assertions and fix a small doc typo in links_with_vectors()
Bound safety: when vectors_count is decoded, vectors_start + vectors_bytes_len must not exceed end. Today this would panic on corrupt input; adding debug_asserts keeps release behavior unchanged while catching issues in debug builds (consistent with prior preferences for debug_assert over runtime checks).
Typo: “variant-encoded length” should be “varint-encoded length”.
Apply this diff:
- // 1. The variant-encoded length (`N` in the doc). + // 1. The varint-encoded length (`N` in the doc). let (vectors_count, vectors_count_size) = u64::decode_var(&neighbors[start..end]).unwrap(); // 2. Padding to align vectors (`_` in the doc). let vectors_start = (start + vectors_count_size).next_multiple_of(vector_alignment as usize); // 3. Vectors (`V` in the doc). - let vectors_bytes_len = vectors_count as usize * vector_size_bytes.get(); - let vectors = &neighbors[vectors_start..vectors_start + vectors_bytes_len]; + let vectors_bytes_len = vectors_count as usize * vector_size_bytes.get(); + let vectors_end = vectors_start + vectors_bytes_len; + debug_assert!(vectors_start <= end, "Invalid block: vectors_start beyond end"); + debug_assert!(vectors_end <= end, "Invalid block: vectors exceed end"); + let vectors = &neighbors[vectors_start..vectors_end]; debug_assert!(vectors.as_ptr().addr() % vector_alignment as usize == 0); // 4. Compressed links (`c` in the doc). let links = iterate_packed_links( - &neighbors[vectors_start + vectors_bytes_len..end], + &neighbors[vectors_end..end], bits_per_unsorted, hnsw_m.level_m(level), );This preserves the existing panic-on-corruption behavior in release, while providing stronger debug-time checks.
342-344: Fix typo in error message“Unsufficent” -> “Insufficient”.
Apply this diff:
-fn error_unsufficent_size() -> OperationError { - OperationError::service_error("Unsufficent file size for GraphLinks file") +fn error_insufficient_size() -> OperationError { + OperationError::service_error("Insufficient file size for GraphLinks file") }Note: update the call sites accordingly (2 occurrences at Lines 114 and 130).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
Cargo.toml(1 hunks)lib/common/common/benches/bitpacking.rs(1 hunks)lib/common/common/benches/bitpacking_tango.rs(1 hunks)lib/common/common/src/bitpacking_links.rs(4 hunks)lib/quantization/src/encoded_vectors_binary.rs(2 hunks)lib/quantization/src/encoded_vectors_pq.rs(2 hunks)lib/quantization/src/encoded_vectors_u8.rs(2 hunks)lib/segment/Cargo.toml(1 hunks)lib/segment/benches/fixture.rs(3 hunks)lib/segment/benches/hnsw_build_asymptotic.rs(3 hunks)lib/segment/benches/hnsw_build_graph.rs(1 hunks)lib/segment/src/fixtures/index_fixtures.rs(3 hunks)lib/segment/src/index/hnsw_index/gpu/gpu_insert_context.rs(1 hunks)lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/gpu_quantization.rs(1 hunks)lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rs(1 hunks)lib/segment/src/index/hnsw_index/gpu/mod.rs(3 hunks)lib/segment/src/index/hnsw_index/graph_layers.rs(18 hunks)lib/segment/src/index/hnsw_index/graph_layers_builder.rs(20 hunks)lib/segment/src/index/hnsw_index/graph_links.rs(6 hunks)lib/segment/src/index/hnsw_index/graph_links/header.rs(2 hunks)lib/segment/src/index/hnsw_index/graph_links/serializer.rs(7 hunks)lib/segment/src/index/hnsw_index/graph_links/view.rs(8 hunks)lib/segment/src/index/hnsw_index/hnsw.rs(1 hunks)lib/segment/src/index/hnsw_index/links_container.rs(2 hunks)lib/segment/src/index/hnsw_index/tests/mod.rs(2 hunks)lib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs(3 hunks)lib/segment/src/vector_storage/quantized/quantized_vectors.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- lib/segment/Cargo.toml
- lib/segment/src/index/hnsw_index/hnsw.rs
- lib/quantization/src/encoded_vectors_binary.rs
- lib/common/common/benches/bitpacking_tango.rs
- lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/gpu_quantization.rs
- lib/segment/benches/hnsw_build_graph.rs
- lib/common/common/benches/bitpacking.rs
- lib/segment/src/index/hnsw_index/gpu/gpu_insert_context.rs
- lib/segment/src/index/hnsw_index/gpu/mod.rs
- lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rs
- lib/segment/benches/hnsw_build_asymptotic.rs
- lib/segment/src/index/hnsw_index/links_container.rs
- lib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs
- lib/quantization/src/encoded_vectors_u8.rs
- lib/common/common/src/bitpacking_links.rs
- lib/segment/benches/fixture.rs
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: xzfc
PR: qdrant/qdrant#6982
File: lib/segment/src/index/hnsw_index/graph_links/view.rs:217-220
Timestamp: 2025-08-06T09:56:59.311Z
Learning: In Qdrant's GraphLinksView implementation (lib/segment/src/index/hnsw_index/graph_links/view.rs), methods like links() and links_with_vectors() intentionally use unwrap() and can panic for performance reasons, maintaining consistency across similar methods. Error handling improvements are considered secondary to feature completion and require benchmarking before implementation.
📚 Learning: 2025-08-06T09:56:59.311Z
Learnt from: xzfc
PR: qdrant/qdrant#6982
File: lib/segment/src/index/hnsw_index/graph_links/view.rs:217-220
Timestamp: 2025-08-06T09:56:59.311Z
Learning: In Qdrant's GraphLinksView implementation (lib/segment/src/index/hnsw_index/graph_links/view.rs), methods like links() and links_with_vectors() intentionally use unwrap() and can panic for performance reasons, maintaining consistency across similar methods. Error handling improvements are considered secondary to feature completion and require benchmarking before implementation.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rslib/segment/src/index/hnsw_index/graph_links/serializer.rslib/segment/src/index/hnsw_index/graph_links.rslib/segment/src/index/hnsw_index/graph_layers.rslib/segment/src/index/hnsw_index/graph_layers_builder.rs
📚 Learning: 2025-05-08T16:39:07.893Z
Learnt from: xzfc
PR: qdrant/qdrant#6501
File: lib/segment/src/index/hnsw_index/links_container.rs:158-158
Timestamp: 2025-05-08T16:39:07.893Z
Learning: In the Qdrant codebase, allocation failures are generally not handled (allowed to panic) except in very few specific cases. Most `unwrap()` calls on allocation operations are intentional.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rslib/segment/src/index/hnsw_index/graph_links/serializer.rs
📚 Learning: 2025-08-10T18:30:02.965Z
Learnt from: generall
PR: qdrant/qdrant#7006
File: lib/collection/src/operations/verification/update.rs:158-174
Timestamp: 2025-08-10T18:30:02.965Z
Learning: In Qdrant's strict mode verification code (lib/collection/src/operations/verification/update.rs), exhaustive pattern matching without `..` is intentionally used for structs like PointsBatch and PointsList. This design pattern ensures compilation fails when new fields are added, forcing developers to explicitly consider how new fields should be handled in the indexed_filter_write method. This provides visibility and compile-time safety for struct evolution.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-03-29T17:43:28.174Z
Learnt from: generall
PR: qdrant/qdrant#6274
File: lib/segment/src/vector_storage/sparse/mmap_sparse_vector_storage.rs:233-233
Timestamp: 2025-03-29T17:43:28.174Z
Learning: In the qdrant codebase, `debug_assert!` is preferred over runtime checks that would panic in release builds to ensure production stability, even when validating conditions like vector sorting.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-05-07T09:13:47.781Z
Learnt from: KShivendu
PR: qdrant/qdrant#6352
File: lib/segment/src/id_tracker/immutable_id_tracker.rs:379-393
Timestamp: 2025-05-07T09:13:47.781Z
Learning: In the Qdrant codebase, prefer fail-fast error handling by returning explicit errors rather than silently continuing with potentially corrupted state, especially in components like ID trackers that are fundamental to data integrity.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-06-06T07:52:38.478Z
Learnt from: timvisee
PR: qdrant/qdrant#6644
File: lib/gridstore/src/blob.rs:41-57
Timestamp: 2025-06-06T07:52:38.478Z
Learning: In the Blob trait implementations for gridstore (lib/gridstore/src/blob.rs), the maintainer prefers using assert! for input validation over Result-based error handling, as they don't expect invalid inputs in real scenarios and changing to Result would require updating the entire trait interface.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-07-11T11:35:21.549Z
Learnt from: generall
PR: qdrant/qdrant#6854
File: lib/segment/src/index/query_estimator.rs:320-327
Timestamp: 2025-07-11T11:35:21.549Z
Learning: In test code for Qdrant's query estimator (lib/segment/src/index/query_estimator.rs), simplified ID resolution logic using `id.to_string().parse().unwrap()` is acceptable for testing purposes and doesn't need to match production code's `id_tracker.internal_id()` approach. Test code can use mock implementations that serve the testing goals.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-05-08T10:34:42.223Z
Learnt from: timvisee
PR: qdrant/qdrant#6444
File: lib/segment/src/index/field_index/map_index/immutable_map_index.rs:373-375
Timestamp: 2025-05-08T10:34:42.223Z
Learning: In the Qdrant codebase, storage backends may have different error handling approaches. Specifically, the mmap variant of `remove_point` method does not return errors, while the RocksDB variant does propagate errors using the `?` operator. These differences are intentional by design.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-06-02T18:10:47.203Z
Learnt from: coszio
PR: qdrant/qdrant#6609
File: lib/gridstore/src/blob.rs:46-59
Timestamp: 2025-06-02T18:10:47.203Z
Learning: In the Qdrant codebase, zerocopy crate is extensively used for safe byte-level operations across GPU operations, HNSW indices, memory-mapped structures, and serialization. When implementing Blob trait for generic Vec<T>, using zerocopy's FromBytes and IntoBytes traits is preferred over size_of::<T>() because it guarantees memory layout equals byte representation, making serialization safe and correct.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-05-19T14:40:20.068Z
Learnt from: coszio
PR: qdrant/qdrant#6528
File: lib/posting_list/src/view.rs:118-118
Timestamp: 2025-05-19T14:40:20.068Z
Learning: In the bitpacking crate, the BitPacker::decompress_strictly_sorted function takes an Option<PointOffsetType> as its first parameter, which means using checked_sub(1) without unwrapping is intentional and correct.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-05-19T09:25:54.285Z
Learnt from: timvisee
PR: qdrant/qdrant#6546
File: lib/segment/src/index/field_index/full_text_index/text_index.rs:497-518
Timestamp: 2025-05-19T09:25:54.285Z
Learning: In the Qdrant project, using `.expect()` and `.unwrap()` is considered acceptable in test code since test failures should be loud and obvious, and proper error propagation adds unnecessary complexity to test code.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/serializer.rs
📚 Learning: 2025-05-30T15:26:14.488Z
Learnt from: generall
PR: qdrant/qdrant#6479
File: lib/segment/src/index/hnsw_index/graph_layers_builder.rs:137-182
Timestamp: 2025-05-30T15:26:14.488Z
Learning: In the subgraph_connectivity method in graph_layers_builder.rs, the previous_visited_points vector and visited BitVec entry point marking are automatically re-initialized at the start of each retry loop iteration, so manual clearing is not needed.
Applied to files:
lib/segment/src/index/hnsw_index/graph_layers.rslib/segment/src/index/hnsw_index/graph_layers_builder.rs
🧬 Code Graph Analysis (9)
lib/segment/src/vector_storage/quantized/quantized_vectors.rs (5)
lib/quantization/src/encoded_vectors_binary.rs (2)
storage(399-401)get_quantized_vector(782-785)lib/segment/src/vector_storage/quantized/quantized_multivector_storage.rs (1)
storage(141-143)lib/quantization/src/encoded_vectors_pq.rs (2)
storage(48-50)get_quantized_vector(436-439)lib/collection/src/operations/types.rs (1)
service_error(1060-1065)lib/segment/src/common/operation_error.rs (1)
service_error(84-89)
lib/quantization/src/encoded_vectors_pq.rs (2)
lib/quantization/src/encoded_vectors_binary.rs (1)
layout(787-793)lib/quantization/src/encoded_vectors_u8.rs (1)
layout(273-275)
lib/segment/src/fixtures/index_fixtures.rs (3)
lib/segment/src/vector_storage/quantized/quantized_scorer_builder.rs (1)
new(36-54)lib/segment/src/vector_storage/quantized/quantized_vectors.rs (1)
create(341-445)lib/segment/src/index/hnsw_index/point_scorer.rs (1)
new_internal(131-164)
lib/segment/src/index/hnsw_index/graph_links/view.rs (4)
lib/common/common/src/bitpacking_links.rs (1)
iterate_packed_links(71-102)lib/common/common/src/bitpacking.rs (3)
packed_bits(190-192)new(23-29)new(79-87)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)lib/segment/src/index/hnsw_index/graph_links.rs (3)
format(202-210)links_with_vectors(235-242)links(230-232)
lib/segment/src/index/hnsw_index/graph_links/serializer.rs (4)
lib/common/common/src/bitpacking_links.rs (1)
pack_links(23-66)lib/common/common/src/bitpacking.rs (1)
packed_bits(190-192)lib/segment/src/index/hnsw_index/graph_links.rs (4)
vector_layout(72-72)vector_layout(80-82)vector_layout(91-97)format(202-210)lib/segment/src/index/hnsw_index/graph_links/view.rs (1)
sorted_count(315-321)
lib/segment/src/index/hnsw_index/graph_links.rs (2)
lib/segment/src/index/hnsw_index/graph_links/view.rs (2)
links_with_vectors(247-295)links(208-239)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)
lib/segment/src/index/hnsw_index/graph_layers.rs (6)
lib/segment/src/index/hnsw_index/graph_links.rs (3)
for_each_link(220-227)format(202-210)links(230-232)lib/segment/src/index/hnsw_index/graph_layers_builder.rs (3)
for_each_link(54-65)hnsw_m(245-247)new(300-315)lib/segment/src/index/hnsw_index/graph_links/view.rs (1)
links(208-239)lib/segment/src/index/hnsw_index/links_container.rs (1)
links(27-29)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)lib/segment/src/index/hnsw_index/mod.rs (1)
new(30-32)
lib/segment/src/index/hnsw_index/tests/mod.rs (3)
lib/segment/src/fixtures/index_fixtures.rs (2)
distance(37-37)new(26-69)lib/segment/src/index/hnsw_index/graph_layers_builder.rs (2)
ef_construct(250-252)new(300-315)lib/segment/src/index/hnsw_index/mod.rs (1)
new(30-32)
lib/segment/src/index/hnsw_index/graph_layers_builder.rs (4)
lib/segment/src/index/hnsw_index/graph_layers.rs (5)
for_each_link(53-55)for_each_link(214-219)get_links_path(303-311)distance(611-611)num_vectors(600-602)lib/segment/src/index/hnsw_index/graph_links.rs (5)
for_each_link(220-227)on_disk(212-214)links(230-232)load_from_file(186-196)format(202-210)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)lib/segment/src/fixtures/index_fixtures.rs (2)
new(26-69)distance(37-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test-consistency
- GitHub Check: test-consensus-compose
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: e2e-tests
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: storage-compat-test
- GitHub Check: lint
- GitHub Check: integration-tests
- GitHub Check: integration-tests-consensus
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
🔇 Additional comments (38)
Cargo.toml (1)
224-224: Add integer-encoding: looks goodWorkspace-scoped addition is appropriate; no extra features required. Proceed.
lib/segment/src/index/hnsw_index/tests/mod.rs (2)
14-23: Fixture signature changes LGTMGeneric RNG bound and threading
use_quantizationinto the builder path are sound.
26-28: OK: propagate quantization into test vector holderCreation via TestRawScorerProducer::new with
use_quantizationmatches the new flow.lib/segment/src/fixtures/index_fixtures.rs (3)
75-78: Accessor LGTMExposing an Option<&QuantizedVectors> accessor matches downstream needs.
80-89: Scorer wiring LGTMThreading optional quantization into FilteredScorer::new and unwrapping in tests is fine.
91-101: Internal scorer wiring LGTMPassing quantized vectors into the internal scorer path is correct.
lib/segment/src/index/hnsw_index/graph_links/header.rs (3)
11-16: Rename to total_neighbors_count looks goodConsistent with the surrounding terminology shift to “neighbors.” No functional concerns.
26-31: total_neighbors_bytes rename is consistentMatches the “neighbors” terminology and pairs well with the plain header’s total_neighbors_count.
51-52: New version tag for compressed-with-vectorsDistinct and explicit versioning is good; matches the new header type.
lib/segment/src/index/hnsw_index/graph_links/serializer.rs (4)
92-105: Validate vector layout invariant across all producersYou enforce size % align == 0. That simplifies alignment (constant stride keeps subsequent elements aligned). Confirm all current and planned quantizers satisfy this; otherwise, this will hard-fail serialization.
If there are quantizers that don’t satisfy size % align == 0, we could:
- insert per-vector padding (costly), or
- relax alignment for the neighbors region and rely on reader copies.
If this invariant is guaranteed, add a short doc note to GraphLinksVectors::vector_layout describing it.
177-204: neighbors_padding computation is soundPadding the neighbors region base to the vector alignment ensures per-entry alignment combined with size%align==0. Good approach.
233-250: Capacity pre-computation covers CompressedWithVectorsIncluding neighbors_padding in size avoids reallocations in serialize_to_writer.
337-346: Ordering and padding for neighbors/offsets are correct
- CompressedWithVectors: prefix padding before neighbors, then neighbors, then compressed offsets. Matches reader expectations.
- Compressed/Plain: unchanged.
lib/segment/src/index/hnsw_index/graph_layers_builder.rs (5)
54-65: for_each_link filters by readiness; correct behaviorFiltering edges against ready_list avoids dangling links during construction. Good guard.
187-201: Propagating GraphLinksFormatParam end-to-endThreading format_param and using as_format() for IO paths is clean. RAM vs on-disk behavior is consistent.
221-229: into_graph_layers_ram: lifetime-aware format_paramTaking GraphLinksFormatParam<'_> and unwrapping serializer is fine for test-only paths. Good separation.
241-242: links_layers_to_serializer: passing format_param throughSerializer construction matches the new API; return type correctly wrapped in OperationResult.
711-721: Tests: wiring quantization only when needed
- Using format.is_with_vectors() to toggle quantization ensures tests don’t panic on missing vectors when CompressedWithVectors is requested.
- Passing format.with_param_for_tests(vector_holder.quantized_vectors()) is the right approach in tests.
Also applies to: 761-763, 866-868, 890-897
lib/segment/src/index/hnsw_index/graph_layers.rs (7)
31-32: New file name and path mapping for CompressedWithVectorslinks_comp_vec.bin mapping looks good; get_links_path covers the new variant.
Also applies to: 303-311
53-56: links_map -> for_each_link migration is cleanCall sites updated consistently; search logic remains unchanged.
Also applies to: 79-85, 145-149, 182-186
200-207: GraphLayersWithVectors trait and impl
- Clear separation for vector-aware iteration.
- Panic-by-design note aligns with established performance choices in view.rs.
Also applies to: 226-235
342-346: Load order prioritizes CompressedWithVectorsTrying CompressedWithVectors, then Compressed, then Plain is the right fallback chain.
355-367: convert_to_compressed: sensible guard for already-compressed filesSkipping conversion if either compressed or compressed-with-vectors exists prevents redundant work.
392-408: compress_ram (testing): correct use of GraphLinksFormatParamPlain → Compressed conversion uses the new param type; unwraps in testing-only code are acceptable.
451-455: Tests include CompressedWithVectors across scenariosCoverage extended to the new format without complicating the search logic. Good parity across variants.
Also applies to: 463-483, 520-521
lib/segment/src/index/hnsw_index/graph_links.rs (10)
53-59: New GraphLinksFormat variant: CompressedWithVectorsEnum extension is straightforward and isolates the new functionality nicely.
60-68: GraphLinksFormatParam prevents misuseRequiring vectors for CompressedWithVectors at the type level is a good API. as_format() helper is convenient.
Also applies to: 147-156
69-84: GraphLinksVectors trait: layout API is minimal and sufficientThe trait shape matches serializer/view needs. Good to see the layout provided from the producer side.
85-99: QuantizedVectors impl is thin and efficientBridges to existing accessors; no copies on get_vector.
202-209: format() covers new compression infoCorrect mapping for the new variant.
234-242: links_with_vectors API: zip of (link, &[u8])Simple and composable; aligns with GraphLayersWithVectors usage.
321-327: random_vectors test helperProvides deterministic-size payloads and keeps alignment simple; fine for tests.
328-358: check_links validates both links and vector payloadsGood test abstraction. Using view().sorted_count(level_idx) to normalize comparisons is robust.
360-392: Save/load tests include CompressedWithVectors and on-disk/RAMEnd-to-end coverage for the new format looks solid.
394-448: Construction tests exercise multiple shapesRandomized and structured cases both pass through the new serializer path. Good signal.
lib/segment/src/index/hnsw_index/graph_links/view.rs (3)
37-42: LGTM: iterator type aliasExposing LinksWithVectorsIterator as a Zip over packed links and fixed-size vector chunks is clear and minimal.
74-97: Docs/readability: great detailThe in-variant documentation of CompressedWithVectors is clear and very helpful for future maintainers.
208-238: links(): CompressedWithVectors fallback to links_with_vectorsUsing links_with_vectors(point_id, level).0 to obtain just the links is a pragmatic choice for tests and keeps behavior consistent with existing APIs. unwrap() usage matches the established performance-first approach for GraphLinksView.
| @@ -1,3 +1,4 @@ | |||
| use std::alloc::Layout; | |||
There was a problem hiding this comment.
Build break: missing import for align_of in layout()
align_of::() isn’t in scope, causing a compile error. Import it or hardcode alignment 1.
Apply one of the following diffs.
Option A (import align_of, consistent with other modules):
use std::alloc::Layout;
+use std::mem::align_of;
@@
pub fn layout(&self) -> Layout {
- Layout::from_size_align(self.metadata.vector_division.len(), align_of::<u8>()).unwrap()
+ Layout::from_size_align(self.metadata.vector_division.len(), align_of::<u8>()).unwrap()
}Option B (avoid extra import, use explicit alignment):
pub fn layout(&self) -> Layout {
- Layout::from_size_align(self.metadata.vector_division.len(), align_of::<u8>()).unwrap()
+ Layout::from_size_align(self.metadata.vector_division.len(), 1).unwrap()
}Also applies to: 441-443
🤖 Prompt for AI Agents
In lib/quantization/src/encoded_vectors_pq.rs around lines 1 and also at
441-443, the code calls align_of::<u8>() but never imports align_of, causing a
compile error; add an import for align_of (e.g., bring std::mem::align_of into
scope near the top of the file) and keep the existing align_of::<u8>() usages
as-is so both the initial import and the occurrences at 441-443 compile
correctly.
2478cd7 to
5e18d2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
92-105: Compressed-with-vectors layout and alignment — looks correct
- Per-node encoding [N, padding, V, c] matches the reader and docstring.
- Header includes vector_size_bytes and alignment; neighbors are file-aligned via neighbors_padding.
- Offsets semantics are consistent across formats.
Two small follow-ups:
- Consider a debug assertion that neighbors start is aligned as intended when writing (sanity check).
- Fix a comment typo: “variant-encoded” → “varint-encoded”.
Example minor comment fix:
- // 1. The variant-encoded length (`N` in the doc). + // 1. The varint-encoded length (`N` in the doc).Also applies to: 113-171, 177-205, 293-315, 337-346
lib/segment/src/index/hnsw_index/graph_links/view.rs (1)
342-344: Fix typo in error messageMinor spelling fix for clarity.
-fn error_unsufficent_size() -> OperationError { - OperationError::service_error("Unsufficent file size for GraphLinks file") +fn error_unsufficient_size() -> OperationError { + OperationError::service_error("Insufficient file size for GraphLinks file") }Note: If you prefer to avoid renaming the function now, at least fix the message string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.toml(1 hunks)lib/segment/Cargo.toml(1 hunks)lib/segment/benches/fixture.rs(3 hunks)lib/segment/src/index/hnsw_index/gpu/mod.rs(3 hunks)lib/segment/src/index/hnsw_index/graph_layers.rs(18 hunks)lib/segment/src/index/hnsw_index/graph_layers_builder.rs(20 hunks)lib/segment/src/index/hnsw_index/graph_links.rs(6 hunks)lib/segment/src/index/hnsw_index/graph_links/header.rs(2 hunks)lib/segment/src/index/hnsw_index/graph_links/serializer.rs(7 hunks)lib/segment/src/index/hnsw_index/graph_links/view.rs(8 hunks)lib/segment/src/index/hnsw_index/hnsw.rs(1 hunks)lib/segment/src/index/hnsw_index/tests/mod.rs(2 hunks)lib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/segment/src/index/hnsw_index/hnsw.rs
- Cargo.toml
- lib/segment/benches/fixture.rs
- lib/segment/src/index/hnsw_index/gpu/mod.rs
- lib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: xzfc
PR: qdrant/qdrant#6982
File: lib/segment/src/index/hnsw_index/graph_links/view.rs:217-220
Timestamp: 2025-08-06T09:56:59.311Z
Learning: In Qdrant's GraphLinksView implementation (lib/segment/src/index/hnsw_index/graph_links/view.rs), methods like links() and links_with_vectors() intentionally use unwrap() and can panic for performance reasons, maintaining consistency across similar methods. Error handling improvements are considered secondary to feature completion and require benchmarking before implementation.
📚 Learning: 2025-08-06T09:56:59.311Z
Learnt from: xzfc
PR: qdrant/qdrant#6982
File: lib/segment/src/index/hnsw_index/graph_links/view.rs:217-220
Timestamp: 2025-08-06T09:56:59.311Z
Learning: In Qdrant's GraphLinksView implementation (lib/segment/src/index/hnsw_index/graph_links/view.rs), methods like links() and links_with_vectors() intentionally use unwrap() and can panic for performance reasons, maintaining consistency across similar methods. Error handling improvements are considered secondary to feature completion and require benchmarking before implementation.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links.rslib/segment/src/index/hnsw_index/graph_links/serializer.rslib/segment/src/index/hnsw_index/graph_links/view.rslib/segment/src/index/hnsw_index/graph_layers_builder.rslib/segment/src/index/hnsw_index/graph_layers.rs
📚 Learning: 2025-05-19T09:25:54.285Z
Learnt from: timvisee
PR: qdrant/qdrant#6546
File: lib/segment/src/index/field_index/full_text_index/text_index.rs:497-518
Timestamp: 2025-05-19T09:25:54.285Z
Learning: In the Qdrant project, using `.expect()` and `.unwrap()` is considered acceptable in test code since test failures should be loud and obvious, and proper error propagation adds unnecessary complexity to test code.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/serializer.rs
📚 Learning: 2025-05-08T16:39:07.893Z
Learnt from: xzfc
PR: qdrant/qdrant#6501
File: lib/segment/src/index/hnsw_index/links_container.rs:158-158
Timestamp: 2025-05-08T16:39:07.893Z
Learning: In the Qdrant codebase, allocation failures are generally not handled (allowed to panic) except in very few specific cases. Most `unwrap()` calls on allocation operations are intentional.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/serializer.rslib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-08-11T00:37:34.065Z
Learnt from: IvanPleshkov
PR: qdrant/qdrant#6986
File: lib/segment/src/vector_storage/quantized/quantized_ram_storage.rs:46-55
Timestamp: 2025-08-11T00:37:34.065Z
Learning: In `lib/segment/src/vector_storage/quantized/quantized_ram_storage.rs`, the `ChunkedVectors` used in `QuantizedRamStorage` is intentionally designed to be non-persistent during updates. The `push_vector` method only updates the in-memory vectors without flushing to disk, and this is expected behavior rather than a bug.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-08-10T18:30:02.965Z
Learnt from: generall
PR: qdrant/qdrant#7006
File: lib/collection/src/operations/verification/update.rs:158-174
Timestamp: 2025-08-10T18:30:02.965Z
Learning: In Qdrant's strict mode verification code (lib/collection/src/operations/verification/update.rs), exhaustive pattern matching without `..` is intentionally used for structs like PointsBatch and PointsList. This design pattern ensures compilation fails when new fields are added, forcing developers to explicitly consider how new fields should be handled in the indexed_filter_write method. This provides visibility and compile-time safety for struct evolution.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-03-29T17:43:28.174Z
Learnt from: generall
PR: qdrant/qdrant#6274
File: lib/segment/src/vector_storage/sparse/mmap_sparse_vector_storage.rs:233-233
Timestamp: 2025-03-29T17:43:28.174Z
Learning: In the qdrant codebase, `debug_assert!` is preferred over runtime checks that would panic in release builds to ensure production stability, even when validating conditions like vector sorting.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-05-07T09:13:47.781Z
Learnt from: KShivendu
PR: qdrant/qdrant#6352
File: lib/segment/src/id_tracker/immutable_id_tracker.rs:379-393
Timestamp: 2025-05-07T09:13:47.781Z
Learning: In the Qdrant codebase, prefer fail-fast error handling by returning explicit errors rather than silently continuing with potentially corrupted state, especially in components like ID trackers that are fundamental to data integrity.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-06-06T07:52:38.478Z
Learnt from: timvisee
PR: qdrant/qdrant#6644
File: lib/gridstore/src/blob.rs:41-57
Timestamp: 2025-06-06T07:52:38.478Z
Learning: In the Blob trait implementations for gridstore (lib/gridstore/src/blob.rs), the maintainer prefers using assert! for input validation over Result-based error handling, as they don't expect invalid inputs in real scenarios and changing to Result would require updating the entire trait interface.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-05-08T10:34:42.223Z
Learnt from: timvisee
PR: qdrant/qdrant#6444
File: lib/segment/src/index/field_index/map_index/immutable_map_index.rs:373-375
Timestamp: 2025-05-08T10:34:42.223Z
Learning: In the Qdrant codebase, storage backends may have different error handling approaches. Specifically, the mmap variant of `remove_point` method does not return errors, while the RocksDB variant does propagate errors using the `?` operator. These differences are intentional by design.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-07-11T11:35:21.549Z
Learnt from: generall
PR: qdrant/qdrant#6854
File: lib/segment/src/index/query_estimator.rs:320-327
Timestamp: 2025-07-11T11:35:21.549Z
Learning: In test code for Qdrant's query estimator (lib/segment/src/index/query_estimator.rs), simplified ID resolution logic using `id.to_string().parse().unwrap()` is acceptable for testing purposes and doesn't need to match production code's `id_tracker.internal_id()` approach. Test code can use mock implementations that serve the testing goals.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-06-02T18:10:47.203Z
Learnt from: coszio
PR: qdrant/qdrant#6609
File: lib/gridstore/src/blob.rs:46-59
Timestamp: 2025-06-02T18:10:47.203Z
Learning: In the Qdrant codebase, zerocopy crate is extensively used for safe byte-level operations across GPU operations, HNSW indices, memory-mapped structures, and serialization. When implementing Blob trait for generic Vec<T>, using zerocopy's FromBytes and IntoBytes traits is preferred over size_of::<T>() because it guarantees memory layout equals byte representation, making serialization safe and correct.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-05-19T14:40:20.068Z
Learnt from: coszio
PR: qdrant/qdrant#6528
File: lib/posting_list/src/view.rs:118-118
Timestamp: 2025-05-19T14:40:20.068Z
Learning: In the bitpacking crate, the BitPacker::decompress_strictly_sorted function takes an Option<PointOffsetType> as its first parameter, which means using checked_sub(1) without unwrapping is intentional and correct.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-05-30T15:26:14.488Z
Learnt from: generall
PR: qdrant/qdrant#6479
File: lib/segment/src/index/hnsw_index/graph_layers_builder.rs:137-182
Timestamp: 2025-05-30T15:26:14.488Z
Learning: In the subgraph_connectivity method in graph_layers_builder.rs, the previous_visited_points vector and visited BitVec entry point marking are automatically re-initialized at the start of each retry loop iteration, so manual clearing is not needed.
Applied to files:
lib/segment/src/index/hnsw_index/graph_layers_builder.rslib/segment/src/index/hnsw_index/graph_layers.rs
🧬 Code Graph Analysis (6)
lib/segment/src/index/hnsw_index/graph_links.rs (2)
lib/segment/src/index/hnsw_index/graph_links/view.rs (2)
links_with_vectors(247-295)links(208-239)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)
lib/segment/src/index/hnsw_index/graph_links/serializer.rs (2)
lib/common/common/src/bitpacking_links.rs (1)
pack_links(23-66)lib/segment/src/index/hnsw_index/graph_links.rs (3)
vector_layout(72-72)vector_layout(80-82)vector_layout(91-97)
lib/segment/src/index/hnsw_index/graph_links/view.rs (2)
lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)lib/segment/src/index/hnsw_index/graph_links.rs (3)
format(202-210)links_with_vectors(235-242)links(230-232)
lib/segment/src/index/hnsw_index/graph_layers_builder.rs (5)
lib/segment/src/index/hnsw_index/graph_links.rs (5)
for_each_link(220-227)on_disk(212-214)links(230-232)load_from_file(186-196)format(202-210)lib/segment/src/index/hnsw_index/graph_layers.rs (5)
for_each_link(53-55)for_each_link(214-219)get_links_path(303-311)distance(611-611)num_vectors(600-602)lib/segment/src/index/hnsw_index/graph_links/view.rs (1)
links(208-239)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)lib/segment/src/index/hnsw_index/tests/mod.rs (1)
create_graph_layer_fixture(46-68)
lib/segment/src/index/hnsw_index/tests/mod.rs (3)
lib/segment/src/fixtures/index_fixtures.rs (2)
distance(37-37)new(26-69)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)lib/segment/src/index/hnsw_index/graph_links.rs (1)
format(202-210)
lib/segment/src/index/hnsw_index/graph_layers.rs (4)
lib/segment/src/index/hnsw_index/graph_links.rs (3)
for_each_link(220-227)format(202-210)links(230-232)lib/segment/src/index/hnsw_index/graph_layers_builder.rs (3)
for_each_link(54-65)hnsw_m(245-247)new(300-315)lib/segment/src/index/hnsw_index/graph_links/view.rs (1)
links(208-239)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test-consistency
- GitHub Check: integration-tests-consensus
- GitHub Check: integration-tests
- GitHub Check: e2e-tests
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-consensus-compose
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: storage-compat-test
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: lint
🔇 Additional comments (7)
lib/segment/Cargo.toml (1)
47-47: Add integer-encoding dependency — LGTMRequired for VarInt read/write in graph-links serializer/view. Workspace pinning is appropriate.
lib/segment/src/index/hnsw_index/tests/mod.rs (1)
14-23: Quantization plumbed through fixtures — LGTMPassing use_quantization into TestRawScorerProducer and the builder fixture is correct and keeps tests flexible.
Also applies to: 26-28
lib/segment/src/index/hnsw_index/graph_links/header.rs (1)
11-16: New header variant for vectors and renamed fields — LGTMStruct layout and zerocopy usage align with serializer/view. vector_alignment as u8 is sufficient and validated at load time.
Also applies to: 26-31, 33-52
lib/segment/src/index/hnsw_index/graph_layers_builder.rs (1)
16-17: Adopt GraphLinksFormatParam across build paths — LGTM
- Parameterized formats are correctly propagated to serializer and file paths via as_format().
- Tests correctly use with_param_for_tests and pass quantized vectors conditionally.
- The for_each_link implementation respects ready flags.
No action needed.
Also applies to: 184-201, 220-242, 593-607, 761-768, 866-874, 890-907
lib/segment/src/index/hnsw_index/graph_layers.rs (1)
29-32: Vector-aware graph layers support — LGTM
- New links file name and loading preference are correct.
- GraphLayersWithVectors provides the needed iteration hook for future usage.
- Conversion path remains conservative (no vectorized format in prod yet).
Looks good.
Also applies to: 53-65, 200-207, 303-310, 341-353, 355-363, 392-408
lib/segment/src/index/hnsw_index/graph_links.rs (1)
53-68: Parameterized formats and vector plumbing — LGTM
- Clean separation of format vs. parameters.
- Safe NonZero for vector size avoids chunks_exact edge cases.
- Test helpers robustly validate vector payloads in zipped iteration.
No changes requested.
Also applies to: 69-99, 100-145, 147-157, 234-243, 321-326, 328-358, 360-392, 394-448
lib/segment/src/index/hnsw_index/graph_links/view.rs (1)
159-206: Reader for compressed-with-vectors — LGTM
- Alignment validation and per-node parsing [N, pad, V, c] match serializer.
- Using NonZero for vector sizes avoids zero-chunk pitfalls.
- Unwraps are acceptable in this path per project conventions.
No behavioral changes requested.
Also applies to: 241-295
5e18d2f to
ec5c151
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/segment/src/index/hnsw_index/tests/mod.rs (1)
65-66: Use with_param_for_tests to avoid silent downgrade in testsUsing
with_paramhere can silently downgrade CompressedWithVectors to Compressed if vectors are None. In tests, prefer failing fast to catch misconfigurations.- let graph_layers = graph_layers_builder - .into_graph_layers_ram(format.with_param(vector_holder.quantized_vectors())); + let graph_layers = graph_layers_builder + .into_graph_layers_ram(format.with_param_for_tests(vector_holder.quantized_vectors()));
🧹 Nitpick comments (2)
lib/segment/src/index/hnsw_index/graph_links.rs (2)
106-119: Add descriptive panic messageThe panic at line 116 should include a descriptive message to help developers understand the issue.
- None => panic!(), + None => panic!("CompressedWithVectors format requires vectors to be provided"),
139-144: Simplify boolean method with matches! macroThe
is_with_vectorsmethod can be simplified using thematches!macro.- pub fn is_with_vectors(&self) -> bool { - match self { - GraphLinksFormat::Plain | GraphLinksFormat::Compressed => false, - GraphLinksFormat::CompressedWithVectors => true, - } - } + pub fn is_with_vectors(&self) -> bool { + matches!(self, GraphLinksFormat::CompressedWithVectors) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.toml(1 hunks)lib/segment/Cargo.toml(1 hunks)lib/segment/benches/fixture.rs(3 hunks)lib/segment/src/index/hnsw_index/gpu/mod.rs(3 hunks)lib/segment/src/index/hnsw_index/graph_layers.rs(18 hunks)lib/segment/src/index/hnsw_index/graph_layers_builder.rs(20 hunks)lib/segment/src/index/hnsw_index/graph_links.rs(6 hunks)lib/segment/src/index/hnsw_index/graph_links/header.rs(2 hunks)lib/segment/src/index/hnsw_index/graph_links/serializer.rs(7 hunks)lib/segment/src/index/hnsw_index/graph_links/view.rs(8 hunks)lib/segment/src/index/hnsw_index/hnsw.rs(1 hunks)lib/segment/src/index/hnsw_index/tests/mod.rs(2 hunks)lib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- lib/segment/Cargo.toml
- Cargo.toml
- lib/segment/src/index/hnsw_index/gpu/mod.rs
- lib/segment/src/index/hnsw_index/hnsw.rs
- lib/segment/src/index/hnsw_index/graph_links/serializer.rs
- lib/segment/src/index/hnsw_index/graph_links/view.rs
- lib/segment/benches/fixture.rs
- lib/segment/src/index/hnsw_index/graph_layers.rs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: xzfc
PR: qdrant/qdrant#6982
File: lib/segment/src/index/hnsw_index/graph_links/view.rs:217-220
Timestamp: 2025-08-06T09:56:59.311Z
Learning: In Qdrant's GraphLinksView implementation (lib/segment/src/index/hnsw_index/graph_links/view.rs), methods like links() and links_with_vectors() intentionally use unwrap() and can panic for performance reasons, maintaining consistency across similar methods. Error handling improvements are considered secondary to feature completion and require benchmarking before implementation.
📚 Learning: 2025-08-06T09:56:59.311Z
Learnt from: xzfc
PR: qdrant/qdrant#6982
File: lib/segment/src/index/hnsw_index/graph_links/view.rs:217-220
Timestamp: 2025-08-06T09:56:59.311Z
Learning: In Qdrant's GraphLinksView implementation (lib/segment/src/index/hnsw_index/graph_links/view.rs), methods like links() and links_with_vectors() intentionally use unwrap() and can panic for performance reasons, maintaining consistency across similar methods. Error handling improvements are considered secondary to feature completion and require benchmarking before implementation.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links.rslib/segment/src/index/hnsw_index/graph_layers_builder.rs
📚 Learning: 2025-05-30T15:26:14.488Z
Learnt from: generall
PR: qdrant/qdrant#6479
File: lib/segment/src/index/hnsw_index/graph_layers_builder.rs:137-182
Timestamp: 2025-05-30T15:26:14.488Z
Learning: In the subgraph_connectivity method in graph_layers_builder.rs, the previous_visited_points vector and visited BitVec entry point marking are automatically re-initialized at the start of each retry loop iteration, so manual clearing is not needed.
Applied to files:
lib/segment/src/index/hnsw_index/graph_layers_builder.rs
📚 Learning: 2025-08-11T07:57:01.355Z
Learnt from: IvanPleshkov
PR: qdrant/qdrant#6986
File: lib/segment/src/vector_storage/quantized/quantized_vectors.rs:81-84
Timestamp: 2025-08-11T07:57:01.355Z
Learning: In `lib/segment/src/vector_storage/quantized/quantized_vectors.rs`, the BitsStoreType parameter difference between single-vector and multi-vector Binary quantization is intentional: single-vector storage uses `EncodedVectorsBin<u128, ...>` to enable 128-bit SIMD/popcount optimizations for longer vectors, while multi-vector storage uses `EncodedVectorsBin<u8, ...>` because multivectors are typically shorter and benefit from byte-granular storage.
Applied to files:
lib/segment/src/index/hnsw_index/tests/mod.rs
🧬 Code Graph Analysis (4)
lib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs (2)
lib/segment/src/index/hnsw_index/tests/mod.rs (1)
create_graph_layer_builder_fixture(14-43)lib/segment/src/index/hnsw_index/graph_links.rs (1)
format(202-210)
lib/segment/src/index/hnsw_index/graph_links.rs (2)
lib/segment/src/index/hnsw_index/graph_links/view.rs (2)
links_with_vectors(247-295)links(208-239)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)
lib/segment/src/index/hnsw_index/graph_layers_builder.rs (6)
lib/segment/src/index/hnsw_index/graph_links.rs (3)
for_each_link(220-227)links(230-232)format(202-210)lib/segment/src/index/hnsw_index/graph_layers.rs (5)
for_each_link(53-55)for_each_link(214-219)get_links_path(303-311)distance(611-611)num_vectors(600-602)lib/segment/src/index/hnsw_index/graph_links/view.rs (1)
links(208-239)lib/segment/src/index/hnsw_index/links_container.rs (1)
links(27-29)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)lib/segment/src/fixtures/index_fixtures.rs (2)
new(26-69)distance(37-37)
lib/segment/src/index/hnsw_index/tests/mod.rs (1)
lib/segment/src/fixtures/index_fixtures.rs (2)
distance(37-37)new(26-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: e2e-tests
- GitHub Check: test-consistency
- GitHub Check: test-consensus-compose
- GitHub Check: integration-tests-consensus
- GitHub Check: integration-tests
- GitHub Check: storage-compat-test
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: lint
🔇 Additional comments (14)
lib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs (3)
57-57: Good test coverage for the new format variantAdding the test case for
CompressedWithVectorsensures the new format is properly tested alongside existing formats.
68-76: Correct parameterization of quantization supportThe use of
format.is_with_vectors()to determine the quantization flag is appropriate and maintains consistency between the format type and quantization settings.
90-91: Proper use of test-specific parameter methodUsing
with_param_for_tests()ensures the test will fail fast if vectors are expected but not available, which is the correct behavior for test scenarios.lib/segment/src/index/hnsw_index/graph_links/header.rs (2)
11-11: Field renaming improves clarityThe rename from
total_links_counttototal_neighbors_countbetter reflects that these represent neighbor relationships in the graph.
26-30: Ignore padding concern forHeaderCompressedThe
zero_paddingfield in HeaderCompressed remains at[u8; 5]; the[u8; 4]padding you saw is on HeaderCompressedWithVectors. No change was made toHeaderCompressed, so there’s no backward‐compatibility impact on existing serialized headers.Likely an incorrect or invalid review comment.
lib/segment/src/index/hnsw_index/tests/mod.rs (1)
14-22: Proper threading of quantization support through test fixturesThe addition of the
use_quantizationparameter and its propagation toTestRawScorerProducer::newcorrectly enables quantization testing.lib/segment/src/index/hnsw_index/graph_layers_builder.rs (3)
54-65: Method rename improves API consistencyRenaming from
links_maptofor_each_linkaligns with the naming convention used in other parts of the codebase.
184-218: Proper error propagation in graph layer constructionThe conversion to use
GraphLinksFormatParamand proper error propagation withOperationResultimproves error handling.
761-762: Consistent use of with_param_for_tests in testsAll test conversions to RAM correctly use
with_param_for_testswhich will fail fast if vectors are expected but not available.Also applies to: 866-867, 905-906
lib/segment/src/index/hnsw_index/graph_links.rs (5)
60-67: Well-designed type safety for vector requirementsThe
GraphLinksFormatParamenum ensures compile-time safety by requiring vectors when usingCompressedWithVectorsformat. This prevents runtime errors from missing vector data.
69-73: Clean trait abstraction for vector accessThe
GraphLinksVectorstrait provides a good abstraction for different vector storage implementations.
234-242: Well-implemented vector iteration supportThe
links_with_vectorsmethod cleanly zips links with their corresponding vectors, providing a convenient API for consumers.
321-326: Good addition of test helper for random vectorsThe
random_vectorsfunction provides necessary test infrastructure for the new vector-aware format.
328-358: Thorough test validation logicThe updated
check_linksfunction properly validates both links and their associated vectors when testingCompressedWithVectorsformat.
ec5c151 to
1b8bed8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/segment/src/index/hnsw_index/graph_links/view.rs (1)
2-2: Fix incorrect NonZero importThe
std::nummodule does not provide a genericNonZero<T>type. This import will cause a compilation error.Apply this diff:
-use std::num::NonZero; +use nonzero::NonZero;Also ensure the
nonzerocrate is added toCargo.tomlif not already present.
🧹 Nitpick comments (2)
lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
132-171: Correct CompressedWithVectors serialization with room for optimizationThe serialization correctly implements the documented format with proper alignment and validation. The comment about avoiding the intermediate buffer is valid - splitting
pack_linksinto prepare and write steps could improve performance, but the current implementation is correct.lib/segment/src/index/hnsw_index/graph_layers.rs (1)
355-390: Documented limitation for format conversionThe code correctly documents that conversion to CompressedWithVectors format is not supported. Since this conversion code is not used in production (per the comment), this limitation is acceptable.
If format conversion becomes needed in the future, consider implementing a migration path from Plain/Compressed to CompressedWithVectors by extracting vectors from the appropriate source.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.toml(1 hunks)lib/segment/Cargo.toml(1 hunks)lib/segment/benches/fixture.rs(3 hunks)lib/segment/src/index/hnsw_index/gpu/mod.rs(3 hunks)lib/segment/src/index/hnsw_index/graph_layers.rs(18 hunks)lib/segment/src/index/hnsw_index/graph_layers_builder.rs(20 hunks)lib/segment/src/index/hnsw_index/graph_links.rs(6 hunks)lib/segment/src/index/hnsw_index/graph_links/header.rs(2 hunks)lib/segment/src/index/hnsw_index/graph_links/serializer.rs(7 hunks)lib/segment/src/index/hnsw_index/graph_links/view.rs(8 hunks)lib/segment/src/index/hnsw_index/hnsw.rs(1 hunks)lib/segment/src/index/hnsw_index/tests/mod.rs(2 hunks)lib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- lib/segment/Cargo.toml
- lib/segment/src/index/hnsw_index/hnsw.rs
- Cargo.toml
- lib/segment/src/index/hnsw_index/tests/test_compact_graph_layer.rs
- lib/segment/src/index/hnsw_index/gpu/mod.rs
- lib/segment/benches/fixture.rs
- lib/segment/src/index/hnsw_index/graph_links/header.rs
- lib/segment/src/index/hnsw_index/graph_links.rs
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: xzfc
PR: qdrant/qdrant#6982
File: lib/segment/src/index/hnsw_index/graph_links/view.rs:217-220
Timestamp: 2025-08-06T09:56:59.311Z
Learning: In Qdrant's GraphLinksView implementation (lib/segment/src/index/hnsw_index/graph_links/view.rs), methods like links() and links_with_vectors() intentionally use unwrap() and can panic for performance reasons, maintaining consistency across similar methods. Error handling improvements are considered secondary to feature completion and require benchmarking before implementation.
📚 Learning: 2025-08-06T09:56:59.311Z
Learnt from: xzfc
PR: qdrant/qdrant#6982
File: lib/segment/src/index/hnsw_index/graph_links/view.rs:217-220
Timestamp: 2025-08-06T09:56:59.311Z
Learning: In Qdrant's GraphLinksView implementation (lib/segment/src/index/hnsw_index/graph_links/view.rs), methods like links() and links_with_vectors() intentionally use unwrap() and can panic for performance reasons, maintaining consistency across similar methods. Error handling improvements are considered secondary to feature completion and require benchmarking before implementation.
Applied to files:
lib/segment/src/index/hnsw_index/graph_layers.rslib/segment/src/index/hnsw_index/graph_links/view.rslib/segment/src/index/hnsw_index/graph_links/serializer.rslib/segment/src/index/hnsw_index/graph_layers_builder.rs
📚 Learning: 2025-05-30T15:26:14.488Z
Learnt from: generall
PR: qdrant/qdrant#6479
File: lib/segment/src/index/hnsw_index/graph_layers_builder.rs:137-182
Timestamp: 2025-05-30T15:26:14.488Z
Learning: In the subgraph_connectivity method in graph_layers_builder.rs, the previous_visited_points vector and visited BitVec entry point marking are automatically re-initialized at the start of each retry loop iteration, so manual clearing is not needed.
Applied to files:
lib/segment/src/index/hnsw_index/graph_layers.rslib/segment/src/index/hnsw_index/graph_layers_builder.rs
📚 Learning: 2025-08-11T00:37:34.065Z
Learnt from: IvanPleshkov
PR: qdrant/qdrant#6986
File: lib/segment/src/vector_storage/quantized/quantized_ram_storage.rs:46-55
Timestamp: 2025-08-11T00:37:34.065Z
Learning: In `lib/segment/src/vector_storage/quantized/quantized_ram_storage.rs`, the `ChunkedVectors` used in `QuantizedRamStorage` is intentionally designed to be non-persistent during updates. The `push_vector` method only updates the in-memory vectors without flushing to disk, and this is expected behavior rather than a bug.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-05-08T16:39:07.893Z
Learnt from: xzfc
PR: qdrant/qdrant#6501
File: lib/segment/src/index/hnsw_index/links_container.rs:158-158
Timestamp: 2025-05-08T16:39:07.893Z
Learning: In the Qdrant codebase, allocation failures are generally not handled (allowed to panic) except in very few specific cases. Most `unwrap()` calls on allocation operations are intentional.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rslib/segment/src/index/hnsw_index/graph_links/serializer.rs
📚 Learning: 2025-08-10T18:30:02.965Z
Learnt from: generall
PR: qdrant/qdrant#7006
File: lib/collection/src/operations/verification/update.rs:158-174
Timestamp: 2025-08-10T18:30:02.965Z
Learning: In Qdrant's strict mode verification code (lib/collection/src/operations/verification/update.rs), exhaustive pattern matching without `..` is intentionally used for structs like PointsBatch and PointsList. This design pattern ensures compilation fails when new fields are added, forcing developers to explicitly consider how new fields should be handled in the indexed_filter_write method. This provides visibility and compile-time safety for struct evolution.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-03-29T17:43:28.174Z
Learnt from: generall
PR: qdrant/qdrant#6274
File: lib/segment/src/vector_storage/sparse/mmap_sparse_vector_storage.rs:233-233
Timestamp: 2025-03-29T17:43:28.174Z
Learning: In the qdrant codebase, `debug_assert!` is preferred over runtime checks that would panic in release builds to ensure production stability, even when validating conditions like vector sorting.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-05-07T09:13:47.781Z
Learnt from: KShivendu
PR: qdrant/qdrant#6352
File: lib/segment/src/id_tracker/immutable_id_tracker.rs:379-393
Timestamp: 2025-05-07T09:13:47.781Z
Learning: In the Qdrant codebase, prefer fail-fast error handling by returning explicit errors rather than silently continuing with potentially corrupted state, especially in components like ID trackers that are fundamental to data integrity.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-06-06T07:52:38.478Z
Learnt from: timvisee
PR: qdrant/qdrant#6644
File: lib/gridstore/src/blob.rs:41-57
Timestamp: 2025-06-06T07:52:38.478Z
Learning: In the Blob trait implementations for gridstore (lib/gridstore/src/blob.rs), the maintainer prefers using assert! for input validation over Result-based error handling, as they don't expect invalid inputs in real scenarios and changing to Result would require updating the entire trait interface.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-07-11T11:35:21.549Z
Learnt from: generall
PR: qdrant/qdrant#6854
File: lib/segment/src/index/query_estimator.rs:320-327
Timestamp: 2025-07-11T11:35:21.549Z
Learning: In test code for Qdrant's query estimator (lib/segment/src/index/query_estimator.rs), simplified ID resolution logic using `id.to_string().parse().unwrap()` is acceptable for testing purposes and doesn't need to match production code's `id_tracker.internal_id()` approach. Test code can use mock implementations that serve the testing goals.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-05-08T10:34:42.223Z
Learnt from: timvisee
PR: qdrant/qdrant#6444
File: lib/segment/src/index/field_index/map_index/immutable_map_index.rs:373-375
Timestamp: 2025-05-08T10:34:42.223Z
Learning: In the Qdrant codebase, storage backends may have different error handling approaches. Specifically, the mmap variant of `remove_point` method does not return errors, while the RocksDB variant does propagate errors using the `?` operator. These differences are intentional by design.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-06-02T18:10:47.203Z
Learnt from: coszio
PR: qdrant/qdrant#6609
File: lib/gridstore/src/blob.rs:46-59
Timestamp: 2025-06-02T18:10:47.203Z
Learning: In the Qdrant codebase, zerocopy crate is extensively used for safe byte-level operations across GPU operations, HNSW indices, memory-mapped structures, and serialization. When implementing Blob trait for generic Vec<T>, using zerocopy's FromBytes and IntoBytes traits is preferred over size_of::<T>() because it guarantees memory layout equals byte representation, making serialization safe and correct.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-05-19T14:40:20.068Z
Learnt from: coszio
PR: qdrant/qdrant#6528
File: lib/posting_list/src/view.rs:118-118
Timestamp: 2025-05-19T14:40:20.068Z
Learning: In the bitpacking crate, the BitPacker::decompress_strictly_sorted function takes an Option<PointOffsetType> as its first parameter, which means using checked_sub(1) without unwrapping is intentional and correct.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/view.rs
📚 Learning: 2025-08-11T07:57:01.355Z
Learnt from: IvanPleshkov
PR: qdrant/qdrant#6986
File: lib/segment/src/vector_storage/quantized/quantized_vectors.rs:81-84
Timestamp: 2025-08-11T07:57:01.355Z
Learning: In `lib/segment/src/vector_storage/quantized/quantized_vectors.rs`, the BitsStoreType parameter difference between single-vector and multi-vector Binary quantization is intentional: single-vector storage uses `EncodedVectorsBin<u128, ...>` to enable 128-bit SIMD/popcount optimizations for longer vectors, while multi-vector storage uses `EncodedVectorsBin<u8, ...>` because multivectors are typically shorter and benefit from byte-granular storage.
Applied to files:
lib/segment/src/index/hnsw_index/tests/mod.rs
📚 Learning: 2025-05-19T09:25:54.285Z
Learnt from: timvisee
PR: qdrant/qdrant#6546
File: lib/segment/src/index/field_index/full_text_index/text_index.rs:497-518
Timestamp: 2025-05-19T09:25:54.285Z
Learning: In the Qdrant project, using `.expect()` and `.unwrap()` is considered acceptable in test code since test failures should be loud and obvious, and proper error propagation adds unnecessary complexity to test code.
Applied to files:
lib/segment/src/index/hnsw_index/graph_links/serializer.rs
🧬 Code Graph Analysis (5)
lib/segment/src/index/hnsw_index/graph_layers.rs (4)
lib/segment/src/index/hnsw_index/graph_links.rs (3)
for_each_link(220-227)format(202-210)links(230-232)lib/segment/src/index/hnsw_index/graph_layers_builder.rs (3)
for_each_link(54-65)hnsw_m(245-247)new(300-315)lib/segment/src/index/hnsw_index/graph_links/view.rs (1)
links(208-239)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)
lib/segment/src/index/hnsw_index/graph_links/view.rs (5)
lib/common/common/src/bitpacking_links.rs (1)
iterate_packed_links(71-102)lib/common/common/src/bitpacking.rs (3)
packed_bits(190-192)new(23-29)new(79-87)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)lib/segment/src/index/hnsw_index/mod.rs (1)
new(30-32)lib/segment/src/index/hnsw_index/graph_links.rs (3)
format(202-210)links_with_vectors(235-242)links(230-232)
lib/segment/src/index/hnsw_index/tests/mod.rs (2)
lib/segment/src/fixtures/index_fixtures.rs (2)
distance(37-37)new(26-69)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)
lib/segment/src/index/hnsw_index/graph_links/serializer.rs (3)
lib/common/common/src/bitpacking_links.rs (1)
pack_links(23-66)lib/segment/src/index/hnsw_index/graph_links.rs (4)
vector_layout(72-72)vector_layout(80-82)vector_layout(91-97)format(202-210)lib/segment/src/index/hnsw_index/graph_links/view.rs (1)
sorted_count(315-321)
lib/segment/src/index/hnsw_index/graph_layers_builder.rs (6)
lib/segment/src/index/hnsw_index/graph_layers.rs (4)
for_each_link(53-55)for_each_link(214-219)get_links_path(303-311)distance(611-611)lib/segment/src/index/hnsw_index/graph_links.rs (4)
for_each_link(220-227)links(230-232)normalize_links(283-287)format(202-210)lib/segment/src/index/hnsw_index/graph_links/view.rs (1)
links(208-239)lib/segment/src/index/hnsw_index/graph_links/serializer.rs (1)
new(52-219)lib/segment/src/fixtures/index_fixtures.rs (2)
new(26-69)distance(37-37)lib/segment/tests/integration/multivector_hnsw_test.rs (1)
distance(69-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: e2e-tests
- GitHub Check: test-consensus-compose
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-consistency
- GitHub Check: integration-tests-consensus
- GitHub Check: lint
- GitHub Check: integration-tests
- GitHub Check: storage-compat-test
🔇 Additional comments (12)
lib/segment/src/index/hnsw_index/graph_links/view.rs (4)
37-98: Well-structured vector support implementationThe new
LinksWithVectorsIteratortype alias andCompressedWithVectorsvariant are well-designed with clear documentation explaining the data layout. Good use ofNonZeroto avoid corner cases and proper alignment support.
159-206: Robust implementation of CompressedWithVectors loaderThe loader properly validates alignment constraints, handles data alignment, and includes appropriate error handling for the NonZero conversion. The implementation is consistent with other format loaders.
247-295: Test-oriented vector iteration implementationThe implementation correctly handles the CompressedWithVectors format with proper alignment and data layout. The use of
unwrap()is consistent with the codebase's performance-oriented design pattern as noted in previous reviews.
314-322: LGTM! Clean test helper implementationThe
sorted_countmethod provides a clear way to determine the number of sorted links for testing purposes.lib/segment/src/index/hnsw_index/tests/mod.rs (1)
14-68: Proper quantization support in test fixturesThe addition of
use_quantizationparameter and the use offormat.with_param_for_tests()ensures tests properly handle quantized vectors without silent downgrades. Good integration with the new CompressedWithVectors format.lib/segment/src/index/hnsw_index/graph_links/serializer.rs (2)
52-105: Good error handling and validation for vector layoutThe constructor properly validates that vector size is a multiple of its alignment, which is crucial for correct memory layout. The change to return
OperationResultenables proper error propagation.
293-316: LGTM! Proper header construction for CompressedWithVectorsThe header properly includes all necessary fields for the new format. The
expect()for alignment conversion is reasonable as alignment values are small powers of 2.lib/segment/src/index/hnsw_index/graph_layers_builder.rs (3)
54-65: Consistent trait method namingThe rename from
links_maptofor_each_linkimproves consistency with the trait interface naming conventions.
221-230: Acceptable use of unwrap() in test codeThe use of
unwrap()in this test-only method is consistent with the codebase's testing practices where failures should be loud and obvious.
703-906: Comprehensive test coverage for new formatTests properly include the CompressedWithVectors format and correctly use quantization when the format supports vectors. Good use of
format.with_param_for_tests()to prevent silent downgrades.lib/segment/src/index/hnsw_index/graph_layers.rs (2)
200-235: Clean trait design for vector-aware iterationThe
GraphLayersWithVectorstrait provides a focused interface for iterating over links with their associated vectors. The panic documentation clearly communicates the precondition.
341-353: Smart format prioritization for backward compatibilityLoading checks formats from newest to oldest, enabling gradual migration while maintaining compatibility with existing data.
63f2c4c to
5da977d
Compare
* Simplify graph_links tests
* Rstest-ify hnsw_index::tests::test_save_load
* Rename EncodedVectorsU8::{get_quantized_vector => get_quantized_vector_offset_and_code}
Reason: it was inconsistent with other implementations of
get_quantized_vector().
* Add QuantizedVectors::{get_quantized_vectors, get_quantized_vector_layout}
* Rename `links_map` to `for_each_link`
* pack_links: expose the updated `raw_links` order
* TestRawScorerProducer: support quantization
* Make GraphLinksSerializer::new return Result
* Rename links/compressed_links into `neighbors`
* Implement GraphLinksFormat::CompressedWithVectors
* GraphLayersWithVectors
* bitpacking: add packed_links_size
* Reorder neighbors: N__VVVVVVVcccccccccc -> Ncccccccccc__VVVVVVV
This PR:
GraphLinksFormat::CompressedWithVectors/links_comp_vec.binformat.GraphLinksSerializer) and reading (GraphLinksView).GraphLayersWithVectors::for_each_link_with_vector()trait method.GraphLayersBase::for_each_link()(formerly,GraphLayersBase::links_map()), but iterates over(PointOffsetType, &[u8])pairs.GraphLayersBase::_search_on_level()-like function.TestRawScorerProducerwith quantized vectors.The main commit is
Implement GraphLinksFormat::CompressedWithVectors, everything else is a preparation work and could be reviewed separately.Format
For comparison, the old format (
links_compressed.bin) has a section calledcompressed_linkswhich looks like this:The new format (
links_comp_vec.bin) is based on that, but this section now looks like this:care compressed links (i.e. it representsVec<u32>but compressed).Qare encoded quantized vectors (i.e.Vec<Vec<u8>>).Lis a varint-encoded length. Number of links == number of quantized vectors == this value.It assumes that every quantized vector has the same byte length. Consequently, multivectors are not supported.