Conversation
📝 WalkthroughWalkthroughThis set of changes introduces a new asynchronous implementation of the Maximum Marginal Relevance (MMR) algorithm for vector-based point collections. The core logic is encapsulated in a new module, exposing the Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (19)
💤 Files with no reviewable changes (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (10)
🧰 Additional context used🧠 Learnings (2)📓 Common learningslib/collection/src/collection/mmr.rs (12)⏰ Context from checks skipped due to timeout of 90000ms (13)
🔇 Additional comments (5)
✨ 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 (4)
lib/collection/src/collection/mod.rs (1)
5-5: Publicly exportingmmrmay widen the crate’s surface areaExposing the new
mmrmodule publicly makes its API part of the
collection crate’s semver contract.
If the intent is purely internal use (e.g. only bycollection
sub-modules), consider reducing visibility topub(crate)to avoid
future breaking-change constraints.lib/segment/src/data_types/vectors.rs (1)
38-44: Refactor to eliminate code duplicationThe
as_vector_refmethod duplicates logic from the existingFrom<&'a VectorInternal> for VectorRef<'a>implementation (lines 192-202). Consider refactoring to use the existing conversion:pub fn as_vector_ref(&self) -> VectorRef<'_> { - match self { - VectorInternal::Dense(dense) => VectorRef::Dense(dense.as_slice()), - VectorInternal::Sparse(sparse) => VectorRef::Sparse(sparse), - VectorInternal::MultiDense(multivec) => VectorRef::MultiDense(multivec.into()), - } + VectorRef::from(self) }lib/collection/src/shards/local_shard/query.rs (1)
415-453: Consider tracking the integration of this unused method.The implementation looks correct with proper score threshold filtering. Since this method is marked as dead code, ensure there's a plan to integrate it into the query execution flow.
Would you like me to create an issue to track the integration of this MMR rescoring method into the query execution pipeline?
lib/collection/src/collection/mmr.rs (1)
90-175: Consider validating vector type consistency.The function assumes all vectors have the same type and dimensions based on the first vector (line 108). While this should be guaranteed by the caller, consider adding a debug assertion or validation to ensure all vectors match the expected type.
Add validation after line 108:
match &vectors[0] { VectorInternal::Dense(vector) => { + debug_assert!(vectors.iter().all(|v| matches!(v, VectorInternal::Dense(_)))); new_volatile_dense_vector_storage(vector.len(), distance) } VectorInternal::Sparse(_sparse_vector) => { + debug_assert!(vectors.iter().all(|v| matches!(v, VectorInternal::Sparse(_)))); new_volatile_sparse_vector_storage() } VectorInternal::MultiDense(typed_multi_dense_vector) => { + debug_assert!(vectors.iter().all(|v| matches!(v, VectorInternal::MultiDense(_))));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
lib/collection/src/collection/mmr.rs(1 hunks)lib/collection/src/collection/mod.rs(1 hunks)lib/collection/src/operations/universal_query/shard_query.rs(1 hunks)lib/collection/src/shards/local_shard/query.rs(3 hunks)lib/segment/src/data_types/vectors.rs(2 hunks)lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rs(0 hunks)lib/segment/src/types.rs(1 hunks)lib/segment/src/vector_storage/dense/mod.rs(0 hunks)lib/segment/src/vector_storage/dense/volatile_dense_vector_storage.rs(1 hunks)lib/segment/src/vector_storage/mod.rs(0 hunks)lib/segment/src/vector_storage/multi_dense/mod.rs(0 hunks)lib/segment/src/vector_storage/multi_dense/volatile_multi_dense_vector_storage.rs(2 hunks)lib/segment/src/vector_storage/quantized/quantized_vectors.rs(0 hunks)lib/segment/src/vector_storage/query_scorer/mod.rs(1 hunks)lib/segment/src/vector_storage/query_scorer/sparse_metric_query_scorer.rs(1 hunks)lib/segment/src/vector_storage/raw_scorer.rs(3 hunks)lib/segment/src/vector_storage/sparse/mod.rs(0 hunks)lib/segment/src/vector_storage/vector_storage_base.rs(0 hunks)lib/sparse/src/common/sparse_vector.rs(1 hunks)
💤 Files with no reviewable changes (7)
- lib/segment/src/vector_storage/quantized/quantized_vectors.rs
- lib/segment/src/vector_storage/sparse/mod.rs
- lib/segment/src/vector_storage/multi_dense/mod.rs
- lib/segment/src/vector_storage/mod.rs
- lib/segment/src/vector_storage/dense/mod.rs
- lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rs
- lib/segment/src/vector_storage/vector_storage_base.rs
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
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.
lib/segment/src/vector_storage/query_scorer/mod.rs (2)
Learnt from: generall
PR: qdrant/qdrant#6088
File: lib/segment/src/index/field_index/mod.rs:18-18
Timestamp: 2025-03-03T15:54:45.553Z
Learning: In the Qdrant codebase, modules can be organized as directories with their own `mod.rs` file, rather than single files, following standard Rust module organization patterns.
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.
lib/collection/src/collection/mod.rs (2)
Learnt from: generall
PR: qdrant/qdrant#6088
File: lib/segment/src/index/field_index/mod.rs:18-18
Timestamp: 2025-03-03T15:54:45.553Z
Learning: In the Qdrant codebase, modules can be organized as directories with their own `mod.rs` file, rather than single files, following standard Rust module organization patterns.
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.
lib/sparse/src/common/sparse_vector.rs (1)
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.
lib/segment/src/vector_storage/multi_dense/volatile_multi_dense_vector_storage.rs (3)
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.
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.
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.
lib/segment/src/types.rs (1)
undefined
<retrieved_learning>
Learnt from: coszio
PR: #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.
</retrieved_learning>
lib/segment/src/vector_storage/dense/volatile_dense_vector_storage.rs (4)
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.
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.
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.
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.
lib/segment/src/vector_storage/raw_scorer.rs (5)
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.
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.
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.
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.
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.
lib/segment/src/vector_storage/query_scorer/sparse_metric_query_scorer.rs (1)
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.
🧬 Code Graph Analysis (2)
lib/segment/src/vector_storage/raw_scorer.rs (3)
lib/segment/src/common/operation_error.rs (1)
service_error(84-89)lib/segment/src/vector_storage/query_scorer/sparse_metric_query_scorer.rs (1)
new(16-29)lib/segment/src/data_types/vectors.rs (2)
new(281-287)new(748-751)
lib/collection/src/shards/local_shard/query.rs (1)
lib/collection/src/collection/mmr.rs (1)
mmr_from_points_with_vector(38-84)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: integration-tests-consensus
🔇 Additional comments (17)
lib/segment/src/vector_storage/query_scorer/mod.rs (1)
15-15: No missing module file
Verified thatlib/segment/src/vector_storage/query_scorer/sparse_metric_query_scorer.rsexists, so thepub mod sparse_metric_query_scorer;declaration inmod.rswill compile as expected.lib/segment/src/types.rs (1)
264-264: LGTM! Excellent addition for MMR functionality.Adding
EnumIterto theDistanceenum enables iteration over all distance function variants, which is valuable for the MMR implementation. This allows the system to programmatically handle multiple distance metrics consistently during MMR computations, supporting the flexibility needed for various vector types and distance metric combinations.lib/sparse/src/common/sparse_vector.rs (1)
141-144: LGTM: Good consistency improvementThe
lenmethod implementation is correct and provides consistency withRemappedSparseVector. This follows standard Rust conventions and enables better API consistency across the codebase.lib/segment/src/vector_storage/dense/volatile_dense_vector_storage.rs (1)
39-47: LGTM: Appropriate test-only restrictionRestricting these specialized constructor functions to test builds is appropriate. The main
new_volatile_dense_vector_storagefunction remains available for production use, while byte and half precision variants are now test-only, which aligns with the broader refactoring to support MMR functionality.lib/segment/src/vector_storage/multi_dense/volatile_multi_dense_vector_storage.rs (1)
70-94: LGTM: Consistent with refactoring patternThese changes maintain consistency with the dense vector storage approach by restricting specialized byte and half precision constructors to test builds while keeping the main constructor available. This supports the MMR functionality integration.
lib/collection/src/operations/universal_query/shard_query.rs (1)
69-76: LGTM: Well-designed MMR configuration structThe
MmrInternalstruct is appropriately designed with clear documentation. The field types (VectorNameBuffor vector naming andf32for lambda parameter) are suitable for MMR configuration, and the documentation clearly explains the trade-off between diversity and relevance.lib/segment/src/data_types/vectors.rs (1)
532-542: LGTM: Correctly implemented take methodThe
takemethod is well-implemented. It correctly consumesselfand handles all variants appropriately, usingHashMap::removefor theNamedcase to extract vectors by name. This method supports the MMR functionality by enabling selective vector extraction.lib/segment/src/vector_storage/raw_scorer.rs (3)
18-20: LGTM!The imports are properly organized and follow the existing pattern. They support the new sparse scorer functionality.
Also applies to: 28-29
133-151: Well-structured sparse scorer for volatile storage.The function correctly handles the special case for
Nearestqueries with volatile sparse storage, with appropriate error handling and fallback to the existing implementation for other query types.
97-97: Correct handling of SparseVolatile variant.The unconditional inclusion of volatile sparse storage handling aligns with the broader changes to make volatile storages available in all builds, supporting the MMR functionality.
lib/collection/src/shards/local_shard/query.rs (1)
17-17: LGTM!The imports are correctly added to support the MMR rescoring functionality.
Also applies to: 27-27
lib/segment/src/vector_storage/query_scorer/sparse_metric_query_scorer.rs (3)
9-29: Well-designed sparse scorer struct.The struct properly holds the necessary references and correctly sets the CPU multiplier for counting sparse vector intersections.
31-42: Correct scoring implementation with proper hardware counter tracking.The method correctly tracks CPU operations by counting the minimum number of comparisons needed for sparse vector scoring.
45-91: Consider the error handling strategy for missing vectors.The implementation uses
expectwhich will panic if vectors are not found. While this might be intentional (vectors should exist when scoring), consider whether returning an error would be more appropriate for production code.Please confirm that panicking on missing vectors is the intended behavior, as it differs from error propagation used in other parts of the codebase.
lib/collection/src/collection/mmr.rs (3)
38-84: Excellent implementation of the MMR algorithm.The function correctly handles deduplication, filtering, and edge cases. The early return optimization for fewer than 2 points is appropriate.
177-216: Correct implementation of the MMR formula.The function efficiently applies the MMR formula using a TopK structure and properly updates the scores in the returned points.
218-710: Excellent test coverage!The test suite comprehensively covers all major scenarios including edge cases, different lambda values, vector types, and distance metrics. The helper functions make the tests readable and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
lib/collection/src/collection/mmr.rs (3)
51-56: Consider logging or documenting the silent filtering behavior.The function silently filters out points that don't have the specified vector. This could be surprising to users who might expect all input points to be considered. Consider adding debug logging or updating the documentation to make this behavior more explicit.
.filter_map(|mut p| { let vector = p .vector .take() - // silently ignore points without this named vector .and_then(|v| v.take(&mmr.using))?; + // Points without the required vector are filtered out + log::debug!("Filtering out point {:?} - missing vector '{}'", p.id, mmr.using); Some((vector, p)) })
114-118: Improve error message clarity.The error message could be more informative about the specific configuration issue.
let multivec_config = collection_params .vectors .get_params(&using) .and_then(|vector_params| vector_params.multivector_config) - .ok_or_else(|| CollectionError::service_error(format!("multivectors are present for {using}, but no multivector config is defined")))?; + .ok_or_else(|| CollectionError::service_error(format!( + "Multi-dense vectors found for '{}', but multivector_config is not defined in collection parameters", + using + )))?;
427-427: Fix incorrect comment about score value.The comment mentions score 0.9, but none of the test points have this score. The highest score in the test data is 0.98.
- assert_ne!(scored_points[0].score, 0.9); // MMR should modify the original scores + assert_ne!(scored_points[0].score, 0.98); // MMR should modify the original scores
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/collection/src/collection/mmr.rs(1 hunks)lib/segment/src/data_types/vectors.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/segment/src/data_types/vectors.rs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
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.
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.
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: test-low-resources
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: test-consensus-compose
- GitHub Check: Basic TLS/HTTPS tests
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-consistency
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: integration-tests-consensus
- GitHub Check: storage-compat-test
- GitHub Check: integration-tests
- GitHub Check: lint
|
@generall noticed that there is a mistake in the way mmr is calculated. Drafting while I fix this |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/collection/src/collection/mmr.rs (1)
175-178: Improve debug assertion pattern.The
debug_assert!(false, ...)pattern is unusual. Consider using a more conventional approach.- debug_assert!( - false, - "There should be at least two vectors to calculate similarity matrix" - ); + debug_assert!( + num_vectors >= 2, + "There should be at least two vectors to calculate similarity matrix" + );Alternatively, you could use
unreachable!()if this condition should never be reached in practice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
lib/collection/src/collection/mmr.rs(1 hunks)lib/collection/src/collection/mod.rs(1 hunks)lib/collection/src/operations/universal_query/shard_query.rs(1 hunks)lib/collection/src/shards/local_shard/query.rs(4 hunks)lib/segment/src/data_types/vectors.rs(1 hunks)lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rs(0 hunks)lib/segment/src/types.rs(2 hunks)lib/segment/src/vector_storage/dense/mod.rs(0 hunks)lib/segment/src/vector_storage/dense/volatile_dense_vector_storage.rs(1 hunks)lib/segment/src/vector_storage/mod.rs(0 hunks)lib/segment/src/vector_storage/multi_dense/mod.rs(0 hunks)lib/segment/src/vector_storage/multi_dense/volatile_multi_dense_vector_storage.rs(2 hunks)lib/segment/src/vector_storage/quantized/quantized_vectors.rs(0 hunks)lib/segment/src/vector_storage/query_scorer/mod.rs(1 hunks)lib/segment/src/vector_storage/query_scorer/sparse_metric_query_scorer.rs(1 hunks)lib/segment/src/vector_storage/raw_scorer.rs(3 hunks)lib/segment/src/vector_storage/sparse/mod.rs(0 hunks)lib/segment/src/vector_storage/vector_storage_base.rs(0 hunks)lib/sparse/src/common/sparse_vector.rs(1 hunks)
💤 Files with no reviewable changes (7)
- lib/segment/src/vector_storage/dense/mod.rs
- lib/segment/src/vector_storage/mod.rs
- lib/segment/src/vector_storage/multi_dense/mod.rs
- lib/segment/src/index/hnsw_index/gpu/gpu_vector_storage/mod.rs
- lib/segment/src/vector_storage/sparse/mod.rs
- lib/segment/src/vector_storage/quantized/quantized_vectors.rs
- lib/segment/src/vector_storage/vector_storage_base.rs
✅ Files skipped from review due to trivial changes (1)
- lib/collection/src/collection/mod.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- lib/segment/src/vector_storage/query_scorer/mod.rs
- lib/segment/src/vector_storage/dense/volatile_dense_vector_storage.rs
- lib/sparse/src/common/sparse_vector.rs
- lib/segment/src/vector_storage/multi_dense/volatile_multi_dense_vector_storage.rs
- lib/segment/src/data_types/vectors.rs
- lib/collection/src/operations/universal_query/shard_query.rs
- lib/segment/src/vector_storage/raw_scorer.rs
- lib/collection/src/shards/local_shard/query.rs
- lib/segment/src/vector_storage/query_scorer/sparse_metric_query_scorer.rs
- lib/segment/src/types.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
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.
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.
lib/collection/src/collection/mmr.rs (6)
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.
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.
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.
Learnt from: xzfc
PR: qdrant/qdrant#6245
File: lib/segment/src/index/hnsw_index/point_scorer.rs:116-121
Timestamp: 2025-04-22T23:17:41.734Z
Learning: The method `is_none_or` exists in the Qdrant codebase and compiles correctly, despite not being part of standard Rust's Option type. Code reviews should verify compilation issues before reporting them.
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.
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.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: Basic TLS/HTTPS tests
- GitHub Check: test-low-resources
- GitHub Check: test-consensus-compose
- GitHub Check: integration-tests-consensus
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: rust-tests (windows-latest)
🔇 Additional comments (3)
lib/collection/src/collection/mmr.rs (3)
235-307: Excellent MMR algorithm implementation.The MMR algorithm is correctly implemented with proper handling of edge cases, NaN-safe comparisons using OrderedFloat, and efficient data structure usage. The iterative selection process correctly balances relevance and diversity according to the MMR formula.
309-800: Comprehensive and well-structured test suite.The test coverage is excellent, covering both happy path scenarios and edge cases including:
- Different lambda values affecting MMR behavior
- Insufficient points and empty inputs
- Points missing required vectors
- Duplicate point handling
- All vector types (dense, sparse, multi-dense) across all distance metrics
The test helper functions are clean and the use of parameterized tests with rstest is appropriate.
1-38: Well-structured module with excellent documentation.The module organization, imports, and comprehensive documentation are exemplary. The function signatures and error handling integrate well with the existing codebase patterns.
lib/segment/src/vector_storage/query_scorer/sparse_metric_query_scorer.rs
Outdated
Show resolved
Hide resolved
lib/segment/src/vector_storage/query_scorer/sparse_metric_query_scorer.rs
Outdated
Show resolved
Hide resolved
lib/segment/src/vector_storage/query_scorer/sparse_metric_query_scorer.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/collection/src/collection/mmr.rs (1)
90-95: Fix invalid syntax for nested Result handling.The triple question mark
???on line 95 is invalid Rust syntax. Based on your explanation of triple-nested Results, the correct syntax should be:- .map_err(|_| CollectionError::timeout(timeout.as_secs() as usize, "mmr"))???; + .map_err(|_| CollectionError::timeout(timeout.as_secs() as usize, "mmr"))?)?)?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/collection/src/collection/mmr.rs(1 hunks)lib/segment/src/vector_storage/query_scorer/sparse_metric_query_scorer.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/segment/src/vector_storage/query_scorer/sparse_metric_query_scorer.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
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.
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.
lib/collection/src/collection/mmr.rs (11)
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.
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.
Learnt from: coszio
PR: qdrant/qdrant#6768
File: lib/collection/src/collection/mmr.rs:90-95
Timestamp: 2025-07-01T17:12:27.939Z
Learning: In the Qdrant codebase, the user coszio mistakenly believed that triple question mark `???` syntax was valid Rust, when it is actually invalid syntax. The codebase uses standard Rust error propagation with `?` and `??` operators only.
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.
Learnt from: xzfc
PR: qdrant/qdrant#6245
File: lib/segment/src/index/hnsw_index/point_scorer.rs:116-121
Timestamp: 2025-04-22T23:17:41.734Z
Learning: The method `is_none_or` exists in the Qdrant codebase and compiles correctly, despite not being part of standard Rust's Option type. Code reviews should verify compilation issues before reporting them.
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.
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.
Learnt from: coszio
PR: qdrant/qdrant#6768
File: lib/collection/src/collection/mmr.rs:90-95
Timestamp: 2025-07-01T17:23:31.897Z
Learning: In the Qdrant codebase, user coszio correctly identified that the timeout/spawn_blocking pattern creates triple-nested Results (timeout Result wrapping JoinResult wrapping CollectionResult), but mistakenly believed that `???` syntax was valid for unwrapping them. The correct syntax is `?)?)?` or proper error flattening.
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.
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.
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.
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: lint
- GitHub Check: test-consensus-compose
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: test-low-resources
- GitHub Check: test-consistency
- GitHub Check: integration-tests-consensus
- GitHub Check: Basic TLS/HTTPS tests
- GitHub Check: integration-tests
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: storage-compat-test
🔇 Additional comments (6)
lib/collection/src/collection/mmr.rs (6)
1-21: LGTM! Well-organized imports.The imports are well-structured and include all necessary dependencies for the MMR implementation.
107-142: LGTM! Excellent vector storage abstraction.The function correctly handles all vector types and creates appropriate volatile storage. The error handling for missing multivector config is well done.
144-159: LGTM! Clean relevance computation.The function correctly creates a scorer and computes similarities between the query and all candidate vectors.
200-221: Excellent optimization for symmetric matrix computation.The implementation cleverly computes only the upper triangle of the similarity matrix and mirrors it to the lower triangle, reducing computational complexity by half while maintaining correctness.
235-316: LGTM! Solid MMR algorithm implementation.The MMR algorithm is correctly implemented with proper lambda weighting, efficient data structures (IndexSet), and clear score handling. The explanation of why query similarity is used as the final score is particularly helpful.
318-809: Excellent comprehensive test coverage.The test suite thoroughly covers all vector types, distance metrics, edge cases, and MMR parameter variations. The helper functions are well-designed and the test scenarios are realistic and valuable.
* mmr at local shard * cleaner impl * handle timeout * protect against small input * fmt * fix compilation * add sparse vector raw scorer for volatile storage * mmr tests * clippy * thx coderabbit * mmr needs a query vector * fix mmr computation * don't measure vector_io_read in sparse scorer * use indexset, and explain why use query score * coderabbit + small input * oopsie * don't remove vectors from points
Adds the required internals to compute Maximum Marginal Relevance as a kind of rescoring. Assumes that the candidate points already retrieved the vectors.
This PR:
(I know there is a large diff, but most of it belongs to testing)