Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
agourlay
left a comment
There was a problem hiding this comment.
I find the change quite complex but it does seem to work properly.
Tracking dependencies explicitly is more appealing than a two stage flushing mechanism.
👏
ad69c75 to
5b525f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @lib/common/common/src/sort_utils.rs:
- Around line 27-38: The example comment has inconsistent capitalization for the
third key: change the lowercase `k3` to uppercase `K3` so the example keys
(`keys`, `sorted_keys`, `perm`) consistently use `K0..K3`; update the comment
occurrences inside the loop example (the lines referencing `k3`, `K3`, and the
swap steps) to use `K3`.
- Around line 4-11: In sort_permutation, validate that values and keys have the
same length before proceeding: add an explicit check at the start of the
function (before the existing values.len() <= 1 early return) that compares
values.len() and keys.len() and aborts with a clear panic/assert message (e.g.,
include both lengths and mention values vs keys) so the function will not
proceed to use mismatched indexing (references: function sort_permutation,
parameters values and keys).
In @lib/common/common/src/toposort.rs:
- Around line 44-49: The add_dependency implementation currently overwrites
existing version values; change it to preserve the maximum version instead.
Locate the add_dependency method and replace the inner HashMap insert with an
entry-based upsert that uses std::cmp::max, e.g., get the inner map via
self.dependencies.entry(element).or_default() and then
inner.entry(depends_on).and_modify(|v| *v = std::cmp::max(*v,
value)).or_insert(value) so the stored value is always the maximum SeqNumberType
seen for that dependency pair.
🧹 Nitpick comments (2)
lib/common/common/src/sort_utils.rs (1)
4-8: Consider adding documentation for the public function.The
sort_permutationfunction is public but lacks documentation explaining its purpose, parameters, time complexity (O(n) in-place), and the requirement that keys must be unique.📚 Proposed documentation
+/// Reorders `values` in-place to match the order that would result from sorting `keys`. +/// +/// # Parameters +/// - `values`: Mutable slice of values to reorder +/// - `keys`: Slice of keys corresponding to each value (must be unique and same length as values) +/// - `sort`: Function that takes the keys and returns them in sorted order +/// +/// # Panics +/// Panics if `values.len() != keys.len()` or if keys contain duplicates. +/// +/// # Time complexity +/// O(n) where n is the length of the slices, using an in-place cycle sort algorithm. pub fn sort_permutation<T: Eq + Hash + Copy, V>( values: &mut [V], keys: &[T], sort: impl FnOnce(&[T]) -> Vec<T>, ) {lib/shard/src/segment_holder/flush.rs (1)
130-139: Consider adding production logging for circular dependencies.Circular dependencies in the flush topology would indicate a bug in the dependency tracking logic. Currently, this is only surfaced via
debug_assert!(dev) andlog::warn(staging feature). In production, circular dependencies would be silently handled.Consider adding unconditional error-level logging to aid debugging if this ever occurs:
♻️ Suggested enhancement
debug_assert!( remaining.is_empty(), "circular dependencies detected in flush topology: {remaining:?}", ); - #[cfg(feature = "staging")] if !remaining.is_empty() { - log::warn!( + log::error!( "circular dependencies detected in flush topology for segments: {remaining:?}" ); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/common/common/src/lib.rslib/common/common/src/sort_utils.rslib/common/common/src/toposort.rslib/segment/src/entry/entry_point.rslib/segment/src/segment/entry.rslib/shard/src/proxy_segment/segment_entry.rslib/shard/src/segment_holder/flush.rslib/shard/src/segment_holder/mod.rslib/shard/src/segment_holder/snapshot.rs
💤 Files with no reviewable changes (1)
- lib/segment/src/entry/entry_point.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/*.rs: Prefer explicitSomeType::from(x)over implicitx.into()in Rust
In new code, don't usetransmute_from_u8,transmute_to_u8,transmute_from_u8_to_slice,transmute_from_u8_to_mut_slice,transmute_to_u8_slice- usebytemuckorzerocopycrates instead
Prefer explicit exhaustive matches over catch-all_arm in Rust match expressions, so we don't miss handling new enum variants when they are added later (exceptions: tests and benchmarks, or when sure new variants won't affect logic)
Prefer explicit field ignoring using: _over using..in Rust struct destructuring (exceptions: tests and benchmarks, or when sure new fields won't affect logic)
Files:
lib/common/common/src/sort_utils.rslib/common/common/src/lib.rslib/segment/src/segment/entry.rslib/common/common/src/toposort.rslib/shard/src/proxy_segment/segment_entry.rslib/shard/src/segment_holder/mod.rslib/shard/src/segment_holder/snapshot.rslib/shard/src/segment_holder/flush.rs
🧠 Learnings (18)
📓 Common learnings
Learnt from: timvisee
Repo: qdrant/qdrant PR: 7388
File: lib/shard/src/segment_holder/flush.rs:73-76
Timestamp: 2025-10-13T09:34:22.740Z
Learning: In Qdrant's SegmentHolder flush logic, when `sync=false` and a background flush is already running, returning early is acceptable because: (1) flushes normally only start if there are changes, (2) the lowest version number is acknowledged in the WAL when changes exist, and (3) missed operations will replay on restart. However, an edge case exists when the `force` flag is used: if the first forced flush has no changes but a follow-up non-force flush with changes is ignored, the system might acknowledge a version that is too recent.
📚 Learning: 2025-09-01T11:42:06.964Z
Learnt from: timvisee
Repo: qdrant/qdrant PR: 7157
File: lib/shard/src/segment_holder/mod.rs:808-814
Timestamp: 2025-09-01T11:42:06.964Z
Learning: In Qdrant's segment holder, panicking when no segments exist during flush_all is intentional and preferred over graceful error handling, as having zero segments could permanently corrupt the WAL by acknowledging u64::MAX. The maintainers consider this condition impossible and use the panic as a fail-fast safety mechanism to prevent data corruption.
Applied to files:
lib/segment/src/segment/entry.rslib/shard/src/proxy_segment/segment_entry.rslib/shard/src/segment_holder/mod.rslib/shard/src/segment_holder/snapshot.rslib/shard/src/segment_holder/flush.rs
📚 Learning: 2025-07-11T11:35:21.549Z
Learnt from: generall
Repo: qdrant/qdrant 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.
Applied to files:
lib/segment/src/segment/entry.rslib/shard/src/proxy_segment/segment_entry.rslib/shard/src/segment_holder/snapshot.rs
📚 Learning: 2025-10-12T22:22:11.435Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7388
File: lib/shard/src/segment_holder/flush.rs:167-191
Timestamp: 2025-10-12T22:22:11.435Z
Learning: In Qdrant's SegmentHolder flush logic, when calculating the minimum unsaved version for WAL acknowledgment, the code intentionally returns `segment_persisted_version` (not `segment_persisted_version + 1`) when there are unsaved changes. This conservative approach assumes the last persisted version might not have been fully applied and allows the system to safely retry operations when versions match, ensuring WAL consistency.
Applied to files:
lib/segment/src/segment/entry.rslib/shard/src/segment_holder/mod.rslib/shard/src/segment_holder/snapshot.rslib/shard/src/segment_holder/flush.rs
📚 Learning: 2025-10-13T09:34:22.740Z
Learnt from: timvisee
Repo: qdrant/qdrant PR: 7388
File: lib/shard/src/segment_holder/flush.rs:73-76
Timestamp: 2025-10-13T09:34:22.740Z
Learning: In Qdrant's SegmentHolder flush logic, when `sync=false` and a background flush is already running, returning early is acceptable because: (1) flushes normally only start if there are changes, (2) the lowest version number is acknowledged in the WAL when changes exist, and (3) missed operations will replay on restart. However, an edge case exists when the `force` flag is used: if the first forced flush has no changes but a follow-up non-force flush with changes is ignored, the system might acknowledge a version that is too recent.
Applied to files:
lib/segment/src/segment/entry.rslib/common/common/src/toposort.rslib/shard/src/proxy_segment/segment_entry.rslib/shard/src/segment_holder/mod.rslib/shard/src/segment_holder/snapshot.rslib/shard/src/segment_holder/flush.rs
📚 Learning: 2025-04-20T19:08:45.034Z
Learnt from: generall
Repo: qdrant/qdrant PR: 6403
File: lib/collection/src/collection_manager/holders/proxy_segment.rs:1178-1197
Timestamp: 2025-04-20T19:08:45.034Z
Learning: In the ProxySegment implementation, overwriting previous index changes when inserting a DeleteIfIncompatible operation is intentional. When an incompatible index is detected, the DeleteIfIncompatible operation should take precedence over any previously scheduled operations on that index.
Applied to files:
lib/shard/src/proxy_segment/segment_entry.rs
📚 Learning: 2025-10-13T22:58:03.121Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7400
File: lib/segment/src/id_tracker/simple_id_tracker.rs:234-241
Timestamp: 2025-10-13T22:58:03.121Z
Learning: SimpleIdTracker in lib/segment/src/id_tracker/simple_id_tracker.rs is being deprecated and should not receive fixes related to version tracking or recovery logic, as it has a different version storage structure that is incompatible with newer trackers.
Applied to files:
lib/shard/src/proxy_segment/segment_entry.rslib/shard/src/segment_holder/mod.rslib/shard/src/segment_holder/snapshot.rslib/shard/src/segment_holder/flush.rs
📚 Learning: 2025-08-10T18:30:02.986Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: lib/collection/src/operations/verification/update.rs:158-174
Timestamp: 2025-08-10T18:30:02.986Z
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/shard/src/segment_holder/mod.rs
📚 Learning: 2025-08-11T00:37:34.100Z
Learnt from: IvanPleshkov
Repo: qdrant/qdrant PR: 6986
File: lib/segment/src/vector_storage/quantized/quantized_ram_storage.rs:46-55
Timestamp: 2025-08-11T00:37:34.100Z
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/shard/src/segment_holder/mod.rslib/shard/src/segment_holder/snapshot.rs
📚 Learning: 2025-08-15T11:42:00.297Z
Learnt from: IvanPleshkov
Repo: qdrant/qdrant PR: 7043
File: lib/segment/src/vector_storage/quantized/quantized_mmap_storage.rs:86-90
Timestamp: 2025-08-15T11:42:00.297Z
Learning: In lib/segment/src/vector_storage/quantized/quantized_mmap_storage.rs, overflow protection for encoded_storage_size computation (quantized_vector_size * vectors_count) is implemented in PR #7048, using checked_mul with u64 casting to prevent silent overflow.
Applied to files:
lib/shard/src/segment_holder/mod.rs
📚 Learning: 2025-04-17T10:52:59.516Z
Learnt from: ffuugoo
Repo: qdrant/qdrant PR: 6352
File: lib/segment/src/id_tracker/mutable_id_tracker.rs:289-299
Timestamp: 2025-04-17T10:52:59.516Z
Learning: In Qdrant's MutableIdTracker, FileVersionTracker flushers are designed to capture the pending version at creation time (not at flush time) to ensure they remain in sync with pending_mappings flusher. This design ensures consistency between data and version information during flush operations.
Applied to files:
lib/shard/src/segment_holder/mod.rslib/shard/src/segment_holder/flush.rs
📚 Learning: 2025-08-20T15:03:18.522Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7100
File: lib/segment/src/index/field_index/full_text_index/inverted_index/mutable_inverted_index.rs:82-100
Timestamp: 2025-08-20T15:03:18.522Z
Learning: The merge_postings_iterator function in lib/segment/src/index/field_index/full_text_index/inverted_index/postings_iterator.rs handles deduplication internally, so there's no need to add .dedup() when using it for OR-merge operations in inverted indexes.
Applied to files:
lib/shard/src/segment_holder/snapshot.rs
📚 Learning: 2025-09-01T11:19:26.371Z
Learnt from: IvanPleshkov
Repo: qdrant/qdrant PR: 7193
File: lib/segment/src/vector_storage/quantized/quantized_chunked_mmap_storage.rs:17-30
Timestamp: 2025-09-01T11:19:26.371Z
Learning: In lib/segment/src/vector_storage/quantized/quantized_chunked_mmap_storage.rs, the ChunkedMmapVectors underlying implementation does not validate against zero-sized vectors, so adding such validation in QuantizedChunkedMmapStorage::new is unnecessary and would be redundant.
Applied to files:
lib/shard/src/segment_holder/snapshot.rs
📚 Learning: 2025-03-29T17:43:28.174Z
Learnt from: generall
Repo: qdrant/qdrant PR: 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/shard/src/segment_holder/flush.rs
📚 Learning: 2025-05-07T09:13:47.781Z
Learnt from: KShivendu
Repo: qdrant/qdrant PR: 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/shard/src/segment_holder/flush.rs
📚 Learning: 2025-08-06T09:56:59.311Z
Learnt from: xzfc
Repo: qdrant/qdrant PR: 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/shard/src/segment_holder/flush.rs
📚 Learning: 2025-11-14T10:01:31.926Z
Learnt from: timvisee
Repo: qdrant/qdrant PR: 7526
File: src/wal_pop.rs:7-8
Timestamp: 2025-11-14T10:01:31.926Z
Learning: In the Qdrant codebase, development binaries (like src/wal_pop.rs) don't require explicit bounds checking for command-line arguments. Rust's built-in out-of-bounds panic is acceptable for these development tools.
Applied to files:
lib/shard/src/segment_holder/flush.rs
📚 Learning: 2025-11-02T19:16:42.527Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7474
File: lib/collection/src/shards/transfer/stream_records.rs:46-55
Timestamp: 2025-11-02T19:16:42.527Z
Learning: In the Qdrant repository, do not flag compilation errors such as partial-move errors, type errors, or any syntax issues that would be caught by the Rust compiler during the build process.
Applied to files:
lib/shard/src/segment_holder/flush.rs
🧬 Code graph analysis (2)
lib/shard/src/segment_holder/snapshot.rs (2)
lib/shard/src/segment_holder/mod.rs (1)
segment_ids(295-300)lib/shard/src/segment_holder/tests.rs (1)
segment_ids(486-496)
lib/shard/src/segment_holder/flush.rs (3)
lib/common/common/src/sort_utils.rs (1)
sort_permutation(4-45)lib/shard/src/segment_holder/mod.rs (5)
segments(936-939)segment_ids(295-300)iter(100-104)iter(932-935)is_empty(110-112)lib/common/common/src/toposort.rs (2)
new(35-37)new(95-135)
⏰ 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). (11)
- GitHub Check: Test Python bindings
- GitHub Check: lint
- GitHub Check: e2e-tests
- GitHub Check: integration-tests-consensus
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-consensus-compose
- GitHub Check: integration-tests
- GitHub Check: test-consistency
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests (ubuntu-latest)
🔇 Additional comments (11)
lib/common/common/src/lib.rs (1)
28-28: LGTM!The new module exports are properly declared and maintain alphabetical ordering with existing modules.
Also applies to: 33-33
lib/shard/src/segment_holder/snapshot.rs (1)
56-56: LGTM! Aligns with the PR objective to remove implicit flush ordering.The removal of segment sorting by
flush_orderingis intentional and consistent with the PR's goal of replacing implicit ordering with explicit CoW-based dependencies.lib/segment/src/segment/entry.rs (1)
23-23: LGTM! Import updated to reflect API changes.The removal of
SegmentFlushOrderingfrom the import is consistent with the broader refactoring to remove the flush_ordering trait-based system.lib/shard/src/proxy_segment/segment_entry.rs (1)
17-17: LGTM! Import cleaned up after API removal.Consistent removal of
SegmentFlushOrderingimport following the trait-based flush ordering system removal.lib/shard/src/segment_holder/mod.rs (2)
20-20: LGTM: Flush dependency tracking infrastructure.The
TopoSort<SegmentId, SeqNumberType>structure correctly models the dependency graph where theSeqNumberTypevalue enables pruning stale dependencies after flushing. UsingArc<Mutex<...>>allows safe concurrent access and cloning for background flush threads.Also applies to: 75-79
697-706: Correct dependency direction for CoW safety.When moving a point from
idx(source) toappendable_idx(destination), addingadd_dependency(idx, appendable_idx, op_num)ensuresappendable_idxis flushed beforeidx. This guarantees the new point is persisted before the deletion, preventing data loss on crash.lib/common/common/src/toposort.rs (2)
78-91: LGTM: Graceful cycle handling.The
into_unordered_vecmethod correctly collects both ready nodes and nodes stuck in cycles (non-zero in-degree). This provides a safe fallback when circular dependencies exist, rather than losing elements.
160-253: LGTM: Comprehensive test coverage.Tests cover key scenarios including empty graphs, chains, diamonds, circular dependencies, and missing nodes. The circular dependency test at line 211-240 validates that the iterator stops when cycles are encountered and
into_unordered_vecrecovers the remaining elements.lib/shard/src/segment_holder/flush.rs (3)
23-23: LGTM: Correct separation of lock order and flush order.Lock acquisition order (non-appendable → appendable) is maintained for deadlock prevention, while
sort_permutationreorderssegment_readsfor the actual flush sequence based on CoW dependencies. This separation correctly addresses both concerns.Also applies to: 75-77
89-91: LGTM: Correct dependency pruning after flush.Retaining dependencies where
version > max_applied_versioncorrectly prunes stale entries. Once segments are flushed up tomax_applied_version, all CoW operations up to that version are safely persisted, making their ordering constraints obsolete.Also applies to: 101-103
113-120: LGTM: Clean helper for lock ordering.The method correctly returns segment IDs in the required lock acquisition order (non-appendable first, then appendable).
| let sorted_keys = sort(keys); | ||
|
|
||
| // Build a map from key to its target position in the sorted order | ||
| let key_to_target_idx: HashMap<T, usize> = sorted_keys | ||
| .into_iter() | ||
| .enumerate() | ||
| .map(|(idx, key)| (key, idx)) | ||
| .collect(); | ||
|
|
||
| // Build permutation: perm[i] = where element at index i should go | ||
| let mut perm: Vec<usize> = keys.iter().map(|k| key_to_target_idx[k]).collect(); |
There was a problem hiding this comment.
Clarify or validate the handling of duplicate keys.
If keys contains duplicates, the HashMap on Line 16-20 will only retain the last occurrence, causing earlier duplicates to panic on Line 23 when accessing key_to_target_idx[k]. Either document that keys must be unique, or add validation.
🛡️ Proposed fix to validate unique keys
let key_to_target_idx: HashMap<T, usize> = sorted_keys
.into_iter()
.enumerate()
.map(|(idx, key)| (key, idx))
.collect();
+ assert_eq!(
+ key_to_target_idx.len(),
+ keys.len(),
+ "keys must be unique"
+ );
+
// Build permutation: perm[i] = where element at index i should go
let mut perm: Vec<usize> = keys.iter().map(|k| key_to_target_idx[k]).collect();There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/common/common/src/toposort.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/*.rs: Prefer explicitSomeType::from(x)over implicitx.into()in Rust
In new code, don't usetransmute_from_u8,transmute_to_u8,transmute_from_u8_to_slice,transmute_from_u8_to_mut_slice,transmute_to_u8_slice- usebytemuckorzerocopycrates instead
Prefer explicit exhaustive matches over catch-all_arm in Rust match expressions, so we don't miss handling new enum variants when they are added later (exceptions: tests and benchmarks, or when sure new variants won't affect logic)
Prefer explicit field ignoring using: _over using..in Rust struct destructuring (exceptions: tests and benchmarks, or when sure new fields won't affect logic)
Files:
lib/common/common/src/toposort.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: timvisee
Repo: qdrant/qdrant PR: 7388
File: lib/shard/src/segment_holder/flush.rs:73-76
Timestamp: 2025-10-13T09:34:22.740Z
Learning: In Qdrant's SegmentHolder flush logic, when `sync=false` and a background flush is already running, returning early is acceptable because: (1) flushes normally only start if there are changes, (2) the lowest version number is acknowledged in the WAL when changes exist, and (3) missed operations will replay on restart. However, an edge case exists when the `force` flag is used: if the first forced flush has no changes but a follow-up non-force flush with changes is ignored, the system might acknowledge a version that is too recent.
📚 Learning: 2025-10-13T09:34:22.740Z
Learnt from: timvisee
Repo: qdrant/qdrant PR: 7388
File: lib/shard/src/segment_holder/flush.rs:73-76
Timestamp: 2025-10-13T09:34:22.740Z
Learning: In Qdrant's SegmentHolder flush logic, when `sync=false` and a background flush is already running, returning early is acceptable because: (1) flushes normally only start if there are changes, (2) the lowest version number is acknowledged in the WAL when changes exist, and (3) missed operations will replay on restart. However, an edge case exists when the `force` flag is used: if the first forced flush has no changes but a follow-up non-force flush with changes is ignored, the system might acknowledge a version that is too recent.
Applied to files:
lib/common/common/src/toposort.rs
📚 Learning: 2025-10-13T22:58:03.121Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7400
File: lib/segment/src/id_tracker/simple_id_tracker.rs:234-241
Timestamp: 2025-10-13T22:58:03.121Z
Learning: SimpleIdTracker in lib/segment/src/id_tracker/simple_id_tracker.rs is being deprecated and should not receive fixes related to version tracking or recovery logic, as it has a different version storage structure that is incompatible with newer trackers.
Applied to files:
lib/common/common/src/toposort.rs
📚 Learning: 2025-08-10T18:30:02.986Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: lib/collection/src/operations/verification/update.rs:158-174
Timestamp: 2025-08-10T18:30:02.986Z
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/common/common/src/toposort.rs
📚 Learning: 2025-10-12T22:22:11.435Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7388
File: lib/shard/src/segment_holder/flush.rs:167-191
Timestamp: 2025-10-12T22:22:11.435Z
Learning: In Qdrant's SegmentHolder flush logic, when calculating the minimum unsaved version for WAL acknowledgment, the code intentionally returns `segment_persisted_version` (not `segment_persisted_version + 1`) when there are unsaved changes. This conservative approach assumes the last persisted version might not have been fully applied and allows the system to safely retry operations when versions match, ensuring WAL consistency.
Applied to files:
lib/common/common/src/toposort.rs
📚 Learning: 2025-08-11T00:37:34.100Z
Learnt from: IvanPleshkov
Repo: qdrant/qdrant PR: 6986
File: lib/segment/src/vector_storage/quantized/quantized_ram_storage.rs:46-55
Timestamp: 2025-08-11T00:37:34.100Z
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/common/common/src/toposort.rs
📚 Learning: 2025-04-17T10:52:59.516Z
Learnt from: ffuugoo
Repo: qdrant/qdrant PR: 6352
File: lib/segment/src/id_tracker/mutable_id_tracker.rs:289-299
Timestamp: 2025-04-17T10:52:59.516Z
Learning: In Qdrant's MutableIdTracker, FileVersionTracker flushers are designed to capture the pending version at creation time (not at flush time) to ensure they remain in sync with pending_mappings flusher. This design ensures consistency between data and version information during flush operations.
Applied to files:
lib/common/common/src/toposort.rs
⏰ 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). (11)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: test-consistency
- GitHub Check: integration-tests-consensus
- GitHub Check: test-consensus-compose
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: e2e-tests
- GitHub Check: integration-tests
- GitHub Check: lint
- GitHub Check: Test Python bindings
🔇 Additional comments (7)
lib/common/common/src/toposort.rs (7)
1-10: LGTM!The struct design is sound. Using
HashMap<T, HashMap<T, V>>to store dependencies with associated values is appropriate for this use case.
12-23: LGTM!Good adherence to coding guidelines with explicit field destructuring without
.., ensuring compile-time safety when new fields are added.
25-31: LGTM!Standard Default implementation.
33-67: LGTM!The methods are well-documented and logically sound. The
retainmethod correctly prunes both individual dependencies and elements with no remaining dependencies. Theadd_dependencybehavior of overwriting existing values is properly documented.
78-92: LGTM!The
into_unordered_vecmethod correctly collects the remaining elements (those not yet yielded by the iterator) by gathering the ready queue plus nodes with non-zero in-degree. This is the appropriate behavior for handling cycles or early termination.
138-158: LGTM!The Iterator implementation correctly follows Kahn's algorithm for topological sorting. The use of
remove(line 145) instead ofgetensures each node's dependents are processed exactly once, preventing double-decrements.
160-253: LGTM!Comprehensive test coverage including edge cases: empty graphs, simple chains, diamonds, circular dependencies, and missing dependencies. The circular dependency test effectively demonstrates the
into_unordered_vecfallback mechanism.
| impl<T: Eq + std::hash::Hash + Copy> TopoSortIter<T> { | ||
| fn new<V>(dependencies: &HashMap<T, HashMap<T, V>>, all_nodes: &[T]) -> Self { | ||
| // Build in-degree count and reverse dependency index | ||
| let mut in_degree: HashMap<T, usize> = HashMap::with_capacity(all_nodes.len()); | ||
| let mut dependents: HashMap<T, Vec<T>> = HashMap::with_capacity(all_nodes.len()); | ||
|
|
||
| // Initialize all nodes with 0 in-degree | ||
| for node in all_nodes { | ||
| in_degree.insert(*node, 0); | ||
| } | ||
|
|
||
| // Build the in-degree counts and reverse index | ||
| for (node, deps) in dependencies { | ||
| if !in_degree.contains_key(node) { | ||
| // Ignore nodes, which are not in the provided all_nodes list | ||
| continue; | ||
| } | ||
| let mut number_of_deps = 0; | ||
| for dep in deps.keys() { | ||
| if !in_degree.contains_key(dep) { | ||
| // Ignore dependencies, which are not in the provided all_nodes list | ||
| continue; | ||
| } | ||
| dependents.entry(*dep).or_default().push(*node); | ||
| number_of_deps += 1; | ||
| } | ||
| in_degree.insert(*node, number_of_deps); | ||
| } | ||
|
|
||
| // Find all nodes with no dependencies (in-degree == 0) | ||
| let ready: VecDeque<T> = all_nodes | ||
| .iter() | ||
| .copied() | ||
| .filter(|node| in_degree.get(node).copied().unwrap_or(0) == 0) | ||
| .collect(); | ||
|
|
||
| Self { | ||
| in_degree, | ||
| dependents, | ||
| ready, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Document or validate that all_nodes must not contain duplicates.
The implementation assumes all_nodes contains unique elements. If duplicates are present, the same node could appear multiple times in the ready queue (line 124-128), leading to potential panics when decrementing in_degree past zero (line 148 in the Iterator impl, where usize -= 1 on a zero value would panic).
Consider either:
- Documenting that callers must ensure
all_nodescontains no duplicates, or - Deduplicating
all_nodesat the start of this method using aHashSet.
📝 Option 1: Add documentation
impl<T: Eq + std::hash::Hash + Copy> TopoSortIter<T> {
+ /// Creates a new topological sort iterator.
+ ///
+ /// # Panics
+ ///
+ /// Panics if `all_nodes` contains duplicate elements.
fn new<V>(dependencies: &HashMap<T, HashMap<T, V>>, all_nodes: &[T]) -> Self {🛡️ Option 2: Add deduplication (more robust)
+use std::collections::HashSet;
+
impl<T: Eq + std::hash::Hash + Copy> TopoSortIter<T> {
fn new<V>(dependencies: &HashMap<T, HashMap<T, V>>, all_nodes: &[T]) -> Self {
+ // Deduplicate all_nodes to prevent panics from duplicate processing
+ let unique_nodes: HashSet<T> = all_nodes.iter().copied().collect();
+ let all_nodes: Vec<T> = unique_nodes.into_iter().collect();
+ let all_nodes = all_nodes.as_slice();
+
// Build in-degree count and reverse dependency index📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl<T: Eq + std::hash::Hash + Copy> TopoSortIter<T> { | |
| fn new<V>(dependencies: &HashMap<T, HashMap<T, V>>, all_nodes: &[T]) -> Self { | |
| // Build in-degree count and reverse dependency index | |
| let mut in_degree: HashMap<T, usize> = HashMap::with_capacity(all_nodes.len()); | |
| let mut dependents: HashMap<T, Vec<T>> = HashMap::with_capacity(all_nodes.len()); | |
| // Initialize all nodes with 0 in-degree | |
| for node in all_nodes { | |
| in_degree.insert(*node, 0); | |
| } | |
| // Build the in-degree counts and reverse index | |
| for (node, deps) in dependencies { | |
| if !in_degree.contains_key(node) { | |
| // Ignore nodes, which are not in the provided all_nodes list | |
| continue; | |
| } | |
| let mut number_of_deps = 0; | |
| for dep in deps.keys() { | |
| if !in_degree.contains_key(dep) { | |
| // Ignore dependencies, which are not in the provided all_nodes list | |
| continue; | |
| } | |
| dependents.entry(*dep).or_default().push(*node); | |
| number_of_deps += 1; | |
| } | |
| in_degree.insert(*node, number_of_deps); | |
| } | |
| // Find all nodes with no dependencies (in-degree == 0) | |
| let ready: VecDeque<T> = all_nodes | |
| .iter() | |
| .copied() | |
| .filter(|node| in_degree.get(node).copied().unwrap_or(0) == 0) | |
| .collect(); | |
| Self { | |
| in_degree, | |
| dependents, | |
| ready, | |
| } | |
| } | |
| } | |
| impl<T: Eq + std::hash::Hash + Copy> TopoSortIter<T> { | |
| /// Creates a new topological sort iterator. | |
| /// | |
| /// # Panics | |
| /// | |
| /// Panics if `all_nodes` contains duplicate elements. | |
| fn new<V>(dependencies: &HashMap<T, HashMap<T, V>>, all_nodes: &[T]) -> Self { | |
| // Build in-degree count and reverse dependency index | |
| let mut in_degree: HashMap<T, usize> = HashMap::with_capacity(all_nodes.len()); | |
| let mut dependents: HashMap<T, Vec<T>> = HashMap::with_capacity(all_nodes.len()); | |
| // Initialize all nodes with 0 in-degree | |
| for node in all_nodes { | |
| in_degree.insert(*node, 0); | |
| } | |
| // Build the in-degree counts and reverse index | |
| for (node, deps) in dependencies { | |
| if !in_degree.contains_key(node) { | |
| // Ignore nodes, which are not in the provided all_nodes list | |
| continue; | |
| } | |
| let mut number_of_deps = 0; | |
| for dep in deps.keys() { | |
| if !in_degree.contains_key(dep) { | |
| // Ignore dependencies, which are not in the provided all_nodes list | |
| continue; | |
| } | |
| dependents.entry(*dep).or_default().push(*node); | |
| number_of_deps += 1; | |
| } | |
| in_degree.insert(*node, number_of_deps); | |
| } | |
| // Find all nodes with no dependencies (in-degree == 0) | |
| let ready: VecDeque<T> = all_nodes | |
| .iter() | |
| .copied() | |
| .filter(|node| in_degree.get(node).copied().unwrap_or(0) == 0) | |
| .collect(); | |
| Self { | |
| in_degree, | |
| dependents, | |
| ready, | |
| } | |
| } | |
| } |
|
Another crasher run just in case: https://github.com/qdrant/crasher/actions/runs/20820617456 |
* CoW defines flush topology * add explicit logging for staging builds * clippy * partial cleanup of topology after flush * Complete comment * Format remaining segments in debug assertion * fix typo * clarify comment * Fix comment * Minor tweaks --------- Co-authored-by: Tim Visée <tim+github@visee.me> Co-authored-by: Arnaud Gourlay <arnaud.gourlay@gmail.com> Co-authored-by: timvisee <tim@visee.me>
Instead of implicit flush ordering, defined by segement type (which is flacky, as segment type might change randomly because of optimizations or snapshot process), we define explicit order based on used CoW operations.
This is an (hopefully easier) alternative for 2-stage flush.