Conversation
📝 WalkthroughWalkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/shard/src/update.rs (3)
182-182: Optional: Make processing order deterministic.AHashMap key iteration is non-deterministic; sorting ids makes behavior reproducible without changing semantics.
Apply this diff:
- let ids: Vec<PointIdType> = points_map.keys().copied().collect(); + let mut ids: Vec<PointIdType> = points_map.keys().copied().collect(); + ids.sort_unstable();
169-172: Docs drift: “random segment” vs current behavior; clarify duplicate-id semantics.The code inserts into the smallest appendable segment and now deduplicates input ids (last occurrence wins). Update the comment.
Apply this diff:
-/// Checks point id in each segment, update point if found. -/// All not found points are inserted into random segment. -/// Returns: number of updated points. +/// Checks each point id in all segments and updates it in place if found. +/// Non-existent ids are inserted into the smallest appendable segment. +/// Duplicate ids in the input are deduplicated (last occurrence wins). +/// Returns: number of updated points.
201-203: Follow guideline: prefer explicit From over Into.Use VectorNameBuf::from(name) instead of name.into().
Apply this diff:
- for (name, vec) in point.get_vectors() { - vectors.insert(name.into(), vec.to_owned()); - } + for (name, vec) in point.get_vectors() { + vectors.insert(VectorNameBuf::from(name), vec.to_owned()); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/shard/src/update.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/*.rs: Prefer explicit SomeType::from(x) over implicit x.into() in Rust code
Do not use transmute_from_u8, transmute_to_u8, transmute_from_u8_to_slice, transmute_from_u8_to_mut_slice, transmute_to_u8_slice in new code; use bytemuck or zerocopy instead
Files:
lib/shard/src/update.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/src/**/*.rs: Prefer exhaustive match arms over a catch-all _ arm to avoid missing new enum variants (except in tests/benchmarks or when provably safe)
Prefer explicit field ignoring with : _ over .. in struct patterns (except in tests/benchmarks or when provably safe)
Files:
lib/shard/src/update.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: generall
PR: qdrant/qdrant#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.
📚 Learning: 2025-08-10T18:30:02.986Z
Learnt from: generall
PR: qdrant/qdrant#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/update.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). (12)
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: integration-tests
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: test-consistency
- GitHub Check: e2e-tests
- GitHub Check: test-consensus-compose
- GitHub Check: lint
- GitHub Check: integration-tests-consensus
- GitHub Check: storage-compat-test
🔇 Additional comments (1)
lib/shard/src/update.rs (1)
181-181: Duplicate IDs correctly deduplicated (last-wins) via id->point map.This fixes the regression by guaranteeing at most one upsert per id and preventing multiple inserts for duplicated ids. Good change.
|
Future work. We need a test capturing our current behavior with duplicated point ids to avoid such silent change in the future. |
Revert incorrect behavior introduced in #7130
There might be duplicated ids.