Conversation
📝 WalkthroughWalkthroughThis change introduces a new Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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: 1
🧹 Nitpick comments (1)
lib/collection/src/collection/mmr/lazy_matrix.rs (1)
13-17: Valid performance concern about memory overhead.The use of
Option<ScoreType>indeed adds memory overhead. For large matrices, consider using a sparse representation (e.g., HashMap) or a separate BitVec to track computed entries if memory becomes a bottleneck.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/collection/src/collection/mmr/lazy_matrix.rs(1 hunks)lib/collection/src/collection/mmr/maximum_marginal_relevance.rs(8 hunks)lib/collection/src/collection/mmr/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
lib/collection/src/collection/mmr/mod.rs (1)
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.
lib/collection/src/collection/mmr/maximum_marginal_relevance.rs (4)
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.
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.
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.
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.
🧬 Code Graph Analysis (2)
lib/collection/src/collection/mmr/mod.rs (1)
lib/collection/src/collection/mmr/maximum_marginal_relevance.rs (1)
mmr_from_points_with_vector(40-117)
lib/collection/src/collection/mmr/maximum_marginal_relevance.rs (1)
lib/collection/src/collection/mmr/lazy_matrix.rs (2)
new(23-47)vectors(35-45)
🔇 Additional comments (7)
lib/collection/src/collection/mmr/mod.rs (1)
1-4: LGTM! Clean module organization.The module structure follows standard Rust patterns with appropriate submodule declarations and public API re-export.
lib/collection/src/collection/mmr/lazy_matrix.rs (2)
49-56: LGTM! Efficient lazy evaluation with caching.The implementation correctly implements lazy evaluation with caching. Note that the mutable self requirement makes this unsuitable for concurrent access, but it's fine for the current single-threaded usage within the blocking task.
58-60: LGTM! Simple delegation to scorer.The implementation correctly delegates to the appropriate scorer. The cast to
PointOffsetTypeis safe in practice given the expected size constraints.lib/collection/src/collection/mmr/maximum_marginal_relevance.rs (4)
92-116: LGTM! Consistent error handling.The changes properly update the error handling to use
CollectionResultandCollectionErrorconsistently throughout the async function.
179-198: LGTM! Clean refactoring to use LazyMatrix.The function now properly delegates similarity matrix creation to the
LazyMatriximplementation, maintaining good separation of concerns.
212-219: LGTM! Correct implementation of lazy similarity computation.The changes properly implement the lazy evaluation pattern:
- Function visibility reduced to private as it's an internal implementation detail
- Takes ownership of
LazyMatrixas required for mutable access- Uses
get_similarityto compute similarities on-demandThis achieves the PR's goal of computing only mn/2 similarities instead of mm/2.
Also applies to: 259-261
17-17: Error type consistency across lib/collection confirmed
- No remaining references to
SegmentResultinlib/collection/.- All async APIs in
lib/collection/consistently returnCollectionResult<…>.No further changes needed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* compute similarities lazily * Update lib/collection/src/collection/mmr/lazy_matrix.rs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Another optimization for MMR.
MMR is usually used as a reranker with more candidate points than selected points.
Without this PR, MMR computes the entire similarity matrix by calculating
m*m/2similiarites, where m=# of candidates.However, what we actually need to compute is
m*n/2similarities, where m=# of candidates, and n=limit.For cases where there are many candidates and small limit, this improves performance more dramatically, for example:
dev(similarities, request timing)limit=10,candidates_limit=1000limit=10,candidates_limit=500limit=10,candidates_limit=100Notice that PR is a bit slower on fewer similarities (10k on dev is faster than 5k on PR). But I believe this is fine considering the amortization for larger number of candidates.