[map index] use roaring bitmap in mutable map index #6926
Conversation
📝 WalkthroughWalkthroughThis set of changes standardizes the iterator types across various map index implementations by shifting from iterators over references to iterators over owned Estimated code review effort3 (~45 minutes) Possibly related PRs
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/segment/src/index/field_index/map_index/mutable_map_index.rs (1)
443-444: Potential overflow when casting from u64 to usize.RoaringBitmap's
len()returnsu64, but casting tousizecould overflow on 32-bit systems whereusizeis 32 bits. While Qdrant likely targets 64-bit systems, consider usingtry_into()for safer conversion or documenting the 64-bit requirement.- self.map.get(value).map(|p| p.len() as usize) + self.map.get(value).and_then(|p| p.len().try_into().ok())- self.map.iter().map(|(k, v)| (k.borrow(), v.len() as usize)) + self.map.iter().filter_map(|(k, v)| { + v.len().try_into().ok().map(|len| (k.borrow(), len)) + })Also applies to: 447-448
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/segment/src/index/field_index/map_index/immutable_map_index.rs(3 hunks)lib/segment/src/index/field_index/map_index/mmap_map_index.rs(2 hunks)lib/segment/src/index/field_index/map_index/mod.rs(8 hunks)lib/segment/src/index/field_index/map_index/mutable_map_index.rs(5 hunks)
🧠 Learnings (5)
📓 Common learnings
Learnt from: generall
PR: qdrant/qdrant#6766
File: lib/segment/src/index/field_index/full_text_index/inverted_index/mutable_inverted_index.rs:274-282
Timestamp: 2025-06-28T19:25:53.448Z
Learning: In the full text inverted index implementations (MutableInvertedIndex, ImmutableInvertedIndex, MmapInvertedIndex), there's an important distinction between TokenId and PointOffsetType: TokenId represents a token/word identifier used to index into postings lists, while PointOffsetType represents a point/document identifier. The iter_point_ids method should take a TokenId and return an iterator over PointOffsetType values (points that contain that token), not the other way around.
lib/segment/src/index/field_index/map_index/mmap_map_index.rs (7)
Learnt from: generall
PR: #6766
File: lib/segment/src/index/field_index/full_text_index/inverted_index/mutable_inverted_index.rs:274-282
Timestamp: 2025-06-28T19:25:53.448Z
Learning: In the full text inverted index implementations (MutableInvertedIndex, ImmutableInvertedIndex, MmapInvertedIndex), there's an important distinction between TokenId and PointOffsetType: TokenId represents a token/word identifier used to index into postings lists, while PointOffsetType represents a point/document identifier. The iter_point_ids method should take a TokenId and return an iterator over PointOffsetType values (points that contain that token), not the other way around.
Learnt from: generall
PR: #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.
Learnt from: coszio
PR: #6565
File: lib/posting_list/src/iterator.rs:0-0
Timestamp: 2025-05-26T14:50:13.183Z
Learning: The advance_until_greater_or_equal method in PostingIterator in lib/posting_list/src/iterator.rs is intentionally designed with different semantics than next() and can output the same value multiple times. This is intended behavior for a search method that finds the first element with ID >= target_id.
Learnt from: coszio
PR: #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 as its first parameter, which means using checked_sub(1) without unwrapping is intentional and correct.
Learnt from: generall
PR: #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.
Learnt from: coszio
PR: #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, using zerocopy's FromBytes and IntoBytes traits is preferred over size_of::() because it guarantees memory layout equals byte representation, making serialization safe and correct.
Learnt from: JojiiOfficial
PR: #5951
File: lib/segment/src/index/field_index/map_index/mmap_map_index.rs:225-242
Timestamp: 2025-03-12T09:33:16.728Z
Learning: In Rust, std::mem::size_of_val(slice) correctly calculates the total size of a slice (equivalent to slice.len() * std::mem::size_of::<T>()). Using manual calculation with slice.len() * std::mem::size_of::<T>() should be avoided as it triggers the Clippy lint "manual_slice_size_calculation".
lib/segment/src/index/field_index/map_index/mod.rs (5)
Learnt from: generall
PR: #6766
File: lib/segment/src/index/field_index/full_text_index/inverted_index/mutable_inverted_index.rs:274-282
Timestamp: 2025-06-28T19:25:53.448Z
Learning: In the full text inverted index implementations (MutableInvertedIndex, ImmutableInvertedIndex, MmapInvertedIndex), there's an important distinction between TokenId and PointOffsetType: TokenId represents a token/word identifier used to index into postings lists, while PointOffsetType represents a point/document identifier. The iter_point_ids method should take a TokenId and return an iterator over PointOffsetType values (points that contain that token), not the other way around.
Learnt from: generall
PR: #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.
Learnt from: coszio
PR: #6565
File: lib/posting_list/src/iterator.rs:0-0
Timestamp: 2025-05-26T14:50:13.183Z
Learning: The advance_until_greater_or_equal method in PostingIterator in lib/posting_list/src/iterator.rs is intentionally designed with different semantics than next() and can output the same value multiple times. This is intended behavior for a search method that finds the first element with ID >= target_id.
Learnt from: JojiiOfficial
PR: #5951
File: lib/api/src/grpc/qdrant.rs:6493-6496
Timestamp: 2025-03-12T09:25:16.340Z
Learning: The payload_index_io_read field in HardwareUsage was introduced in PR #5951 and is safe to modify as it was not used in production before this PR.
Learnt from: IvanPleshkov
PR: #6663
File: lib/quantization/src/encoded_vectors_binary.rs:270-276
Timestamp: 2025-06-18T09:23:14.977Z
Learning: Pattern matching on &Option<T> with Some(x) is valid and idiomatic in Rust. The bound variable x will have type &T and can be used to access fields through automatic dereferencing. No need to use .as_ref() in such cases.
lib/segment/src/index/field_index/map_index/immutable_map_index.rs (6)
Learnt from: generall
PR: #6766
File: lib/segment/src/index/field_index/full_text_index/inverted_index/mutable_inverted_index.rs:274-282
Timestamp: 2025-06-28T19:25:53.448Z
Learning: In the full text inverted index implementations (MutableInvertedIndex, ImmutableInvertedIndex, MmapInvertedIndex), there's an important distinction between TokenId and PointOffsetType: TokenId represents a token/word identifier used to index into postings lists, while PointOffsetType represents a point/document identifier. The iter_point_ids method should take a TokenId and return an iterator over PointOffsetType values (points that contain that token), not the other way around.
Learnt from: generall
PR: #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.
Learnt from: coszio
PR: #6565
File: lib/posting_list/src/iterator.rs:0-0
Timestamp: 2025-05-26T14:50:13.183Z
Learning: The advance_until_greater_or_equal method in PostingIterator in lib/posting_list/src/iterator.rs is intentionally designed with different semantics than next() and can output the same value multiple times. This is intended behavior for a search method that finds the first element with ID >= target_id.
Learnt from: timvisee
PR: #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.
Learnt from: timvisee
PR: #6150
File: lib/segment/src/id_tracker/mutable_id_tracker.rs:62-71
Timestamp: 2025-03-12T16:23:44.715Z
Learning: In the MutableIdTracker implementation in Qdrant, the error case where versions file exists but mappings file is missing is considered a system-level error (potential corruption) rather than a user error, and is handled via logging and debug assertions without surfacing user-facing errors.
Learnt from: coszio
PR: #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 as its first parameter, which means using checked_sub(1) without unwrapping is intentional and correct.
lib/segment/src/index/field_index/map_index/mutable_map_index.rs (8)
Learnt from: generall
PR: #6766
File: lib/segment/src/index/field_index/full_text_index/inverted_index/mutable_inverted_index.rs:274-282
Timestamp: 2025-06-28T19:25:53.448Z
Learning: In the full text inverted index implementations (MutableInvertedIndex, ImmutableInvertedIndex, MmapInvertedIndex), there's an important distinction between TokenId and PointOffsetType: TokenId represents a token/word identifier used to index into postings lists, while PointOffsetType represents a point/document identifier. The iter_point_ids method should take a TokenId and return an iterator over PointOffsetType values (points that contain that token), not the other way around.
Learnt from: generall
PR: #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.
Learnt from: coszio
PR: #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.
Learnt from: generall
PR: #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.
Learnt from: timvisee
PR: #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.
Learnt from: timvisee
PR: #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.
Learnt from: coszio
PR: #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, using zerocopy's FromBytes and IntoBytes traits is preferred over size_of::() because it guarantees memory layout equals byte representation, making serialization safe and correct.
Learnt from: coszio
PR: #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 as its first parameter, which means using checked_sub(1) without unwrapping is intentional and correct.
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: generall
PR: qdrant/qdrant#6766
File: lib/segment/src/index/field_index/full_text_index/inverted_index/mutable_inverted_index.rs:274-282
Timestamp: 2025-06-28T19:25:53.448Z
Learning: In the full text inverted index implementations (MutableInvertedIndex, ImmutableInvertedIndex, MmapInvertedIndex), there's an important distinction between TokenId and PointOffsetType: TokenId represents a token/word identifier used to index into postings lists, while PointOffsetType represents a point/document identifier. The iter_point_ids method should take a TokenId and return an iterator over PointOffsetType values (points that contain that token), not the other way around.
lib/segment/src/index/field_index/map_index/mmap_map_index.rs (7)
Learnt from: generall
PR: #6766
File: lib/segment/src/index/field_index/full_text_index/inverted_index/mutable_inverted_index.rs:274-282
Timestamp: 2025-06-28T19:25:53.448Z
Learning: In the full text inverted index implementations (MutableInvertedIndex, ImmutableInvertedIndex, MmapInvertedIndex), there's an important distinction between TokenId and PointOffsetType: TokenId represents a token/word identifier used to index into postings lists, while PointOffsetType represents a point/document identifier. The iter_point_ids method should take a TokenId and return an iterator over PointOffsetType values (points that contain that token), not the other way around.
Learnt from: generall
PR: #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.
Learnt from: coszio
PR: #6565
File: lib/posting_list/src/iterator.rs:0-0
Timestamp: 2025-05-26T14:50:13.183Z
Learning: The advance_until_greater_or_equal method in PostingIterator in lib/posting_list/src/iterator.rs is intentionally designed with different semantics than next() and can output the same value multiple times. This is intended behavior for a search method that finds the first element with ID >= target_id.
Learnt from: coszio
PR: #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 as its first parameter, which means using checked_sub(1) without unwrapping is intentional and correct.
Learnt from: generall
PR: #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.
Learnt from: coszio
PR: #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, using zerocopy's FromBytes and IntoBytes traits is preferred over size_of::() because it guarantees memory layout equals byte representation, making serialization safe and correct.
Learnt from: JojiiOfficial
PR: #5951
File: lib/segment/src/index/field_index/map_index/mmap_map_index.rs:225-242
Timestamp: 2025-03-12T09:33:16.728Z
Learning: In Rust, std::mem::size_of_val(slice) correctly calculates the total size of a slice (equivalent to slice.len() * std::mem::size_of::<T>()). Using manual calculation with slice.len() * std::mem::size_of::<T>() should be avoided as it triggers the Clippy lint "manual_slice_size_calculation".
lib/segment/src/index/field_index/map_index/mod.rs (5)
Learnt from: generall
PR: #6766
File: lib/segment/src/index/field_index/full_text_index/inverted_index/mutable_inverted_index.rs:274-282
Timestamp: 2025-06-28T19:25:53.448Z
Learning: In the full text inverted index implementations (MutableInvertedIndex, ImmutableInvertedIndex, MmapInvertedIndex), there's an important distinction between TokenId and PointOffsetType: TokenId represents a token/word identifier used to index into postings lists, while PointOffsetType represents a point/document identifier. The iter_point_ids method should take a TokenId and return an iterator over PointOffsetType values (points that contain that token), not the other way around.
Learnt from: generall
PR: #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.
Learnt from: coszio
PR: #6565
File: lib/posting_list/src/iterator.rs:0-0
Timestamp: 2025-05-26T14:50:13.183Z
Learning: The advance_until_greater_or_equal method in PostingIterator in lib/posting_list/src/iterator.rs is intentionally designed with different semantics than next() and can output the same value multiple times. This is intended behavior for a search method that finds the first element with ID >= target_id.
Learnt from: JojiiOfficial
PR: #5951
File: lib/api/src/grpc/qdrant.rs:6493-6496
Timestamp: 2025-03-12T09:25:16.340Z
Learning: The payload_index_io_read field in HardwareUsage was introduced in PR #5951 and is safe to modify as it was not used in production before this PR.
Learnt from: IvanPleshkov
PR: #6663
File: lib/quantization/src/encoded_vectors_binary.rs:270-276
Timestamp: 2025-06-18T09:23:14.977Z
Learning: Pattern matching on &Option<T> with Some(x) is valid and idiomatic in Rust. The bound variable x will have type &T and can be used to access fields through automatic dereferencing. No need to use .as_ref() in such cases.
lib/segment/src/index/field_index/map_index/immutable_map_index.rs (6)
Learnt from: generall
PR: #6766
File: lib/segment/src/index/field_index/full_text_index/inverted_index/mutable_inverted_index.rs:274-282
Timestamp: 2025-06-28T19:25:53.448Z
Learning: In the full text inverted index implementations (MutableInvertedIndex, ImmutableInvertedIndex, MmapInvertedIndex), there's an important distinction between TokenId and PointOffsetType: TokenId represents a token/word identifier used to index into postings lists, while PointOffsetType represents a point/document identifier. The iter_point_ids method should take a TokenId and return an iterator over PointOffsetType values (points that contain that token), not the other way around.
Learnt from: generall
PR: #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.
Learnt from: coszio
PR: #6565
File: lib/posting_list/src/iterator.rs:0-0
Timestamp: 2025-05-26T14:50:13.183Z
Learning: The advance_until_greater_or_equal method in PostingIterator in lib/posting_list/src/iterator.rs is intentionally designed with different semantics than next() and can output the same value multiple times. This is intended behavior for a search method that finds the first element with ID >= target_id.
Learnt from: timvisee
PR: #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.
Learnt from: timvisee
PR: #6150
File: lib/segment/src/id_tracker/mutable_id_tracker.rs:62-71
Timestamp: 2025-03-12T16:23:44.715Z
Learning: In the MutableIdTracker implementation in Qdrant, the error case where versions file exists but mappings file is missing is considered a system-level error (potential corruption) rather than a user error, and is handled via logging and debug assertions without surfacing user-facing errors.
Learnt from: coszio
PR: #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 as its first parameter, which means using checked_sub(1) without unwrapping is intentional and correct.
lib/segment/src/index/field_index/map_index/mutable_map_index.rs (8)
Learnt from: generall
PR: #6766
File: lib/segment/src/index/field_index/full_text_index/inverted_index/mutable_inverted_index.rs:274-282
Timestamp: 2025-06-28T19:25:53.448Z
Learning: In the full text inverted index implementations (MutableInvertedIndex, ImmutableInvertedIndex, MmapInvertedIndex), there's an important distinction between TokenId and PointOffsetType: TokenId represents a token/word identifier used to index into postings lists, while PointOffsetType represents a point/document identifier. The iter_point_ids method should take a TokenId and return an iterator over PointOffsetType values (points that contain that token), not the other way around.
Learnt from: generall
PR: #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.
Learnt from: coszio
PR: #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.
Learnt from: generall
PR: #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.
Learnt from: timvisee
PR: #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.
Learnt from: timvisee
PR: #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.
Learnt from: coszio
PR: #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, using zerocopy's FromBytes and IntoBytes traits is preferred over size_of::() because it guarantees memory layout equals byte representation, making serialization safe and correct.
Learnt from: coszio
PR: #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 as its first parameter, which means using checked_sub(1) without unwrapping is intentional and correct.
⏰ 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: e2e-tests
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-consistency
- GitHub Check: test-consensus-compose
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: integration-tests-consensus
- GitHub Check: integration-tests
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: storage-compat-test
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: lint
🔇 Additional comments (8)
lib/segment/src/index/field_index/map_index/mmap_map_index.rs (1)
325-344: LGTM! Iterator standardization implemented correctly.The change to return
IdIter<'_>yielding ownedPointOffsetTypevalues is consistent with the PR's goal of standardizing iterator types across map index implementations. The addition of.copied()after filtering is necessary and correct for the type conversion.lib/segment/src/index/field_index/map_index/mod.rs (2)
237-243: LGTM! Consistent iterator type change across implementations.The
get_iteratormethod signature change to returnIdIter<'_>is correctly implemented across all map index variants (Mutable, Immutable, Mmap).
493-494: Correctly removed.copied()calls after iterator standardization.All
.copied()calls have been appropriately removed sinceget_iteratornow returns an iterator yielding owned values directly. This simplifies the code and reduces unnecessary operations.Also applies to: 750-751, 759-760, 903-904, 920-921, 943-944, 1096-1097, 1111-1112
lib/segment/src/index/field_index/map_index/immutable_map_index.rs (2)
21-21: LGTM! Iterator type standardization in immutable index.The import change and method signature updates correctly implement the standardized iterator type that yields owned values.
Also applies to: 513-521
522-542: Correct implementation of owned value iterator.The
get_iteratormethod properly handles the conversion from references to owned values with the dereference operation*idxon line 537. The empty iterator fallback also correctly returns the expected type.lib/segment/src/index/field_index/map_index/mutable_map_index.rs (3)
2-2: Successfully migrated from BTreeSet to RoaringBitmap.The import changes and type migration align with the PR objectives to improve performance for scenarios with many values per point. RoaringBitmap provides better memory efficiency and performance for dense sets of integers.
Also applies to: 12-12, 18-18, 42-42
298-299: Correct API usage for RoaringBitmap remove operation.The change from
vals.remove(&idx)tovals.remove(idx)is correct - RoaringBitmap's remove method takes the value directly, not a reference.
453-454: Clean iterator implementation with RoaringBitmap.The iterator construction is correctly implemented, leveraging RoaringBitmap's built-in iterator which yields owned
u32values (PointOffsetType).Also applies to: 456-461
|
Could you please also include benchmark on "normal" workloads, even if they are not improving |
|
@generall I made a mistake when benchmarking. When compared to perf build and all feature flags on, the difference is not as dramatic. I have updated the description accordingly. Still increases upsert performance. |
Similar to #6685, swaps the container of ids per value in mutable map index from
BTreeSet<u32>toRoaringBitmap.This is not too noticeable on "normal" workloads, but when there is many values per point, it does provide a bit of advantage on insertion.
For example, with the following command (100k points):
This is the performance improvement (edit: reran with release build of bfb and perf builds of qdrant)
And, with just 1 keyword per point