lib/shard/src/segment_holder/tests.rs (4)

6-6: Nit: avoid mixing fully-qualified and imported only_default_vector usage in the same file

The new import brings only_default_vector into scope, but most existing tests in this file still call it via the fully-qualified path. Consider unifying on one style for consistency (either keep the import and migrate older call sites later, or drop the import and use the fully-qualified path here).

If you prefer to keep the existing style in this file, apply the following edits:

- use segment::data_types::vectors::{DEFAULT_VECTOR_NAME, VectorInternal, only_default_vector};
+ use segment::data_types::vectors::{DEFAULT_VECTOR_NAME, VectorInternal};

And in the new test (see suggested edit below in the relevant hunk), call segment::data_types::vectors::only_default_vector(...) instead of the unqualified name.

---

613-621: Guard against accidental empty/extra proxies before indexing

We index inner_proxies[0] directly. While the setup creates exactly one segment today, an explicit assertion improves failure diagnostics and prevents a panic if the setup changes.

     // check inner proxy contains points
+    assert_eq!(inner_proxies.len(), 1, "expected exactly one inner proxy");
     let points = inner_proxies[0]
         .1
         .get()
         .read()
         .read_range(Some(1.into()), None);

---

633-645: Minor clarity: use [0.0; 4] for vector construction (matches the rest of the file)

Elsewhere in this file tests use the [0.0; 4] shorthand. Using it here keeps the style consistent.

Also see previous comment if you opt to keep fully-qualified calls to only_default_vector.

-        .upsert_point(
+        .upsert_point(
             100,
             1.into(),
-            only_default_vector(&[0.0, 0.0, 0.0, 0.0]),
+            only_default_vector(&[0.0; 4]),
             &hw_counter,
         )

If you choose the fully-qualified form for consistency with the rest of the file:

-            only_default_vector(&[0.0; 4]),
+            segment::data_types::vectors::only_default_vector(&[0.0; 4]),

---

660-667: Strengthen the postcondition: verify the upserted point resides in the new segment only

Asserting the presence of the newly appended segment is good. To make the test more robust (and closer to the bug’s behavioral contract), assert that:

• the new segment contains point 1, and
• the original segment no longer contains point 1 (deleted via the inner proxy and not reintroduced there).

     // Check that we have one new segment
     let diff: HashSet<_> = after_segment_ids.difference(&before_segment_ids).collect();
     assert_eq!(
         diff.len(),
         1,
         "There should be one new segment after unproxying"
     );
+
+    // Additionally, ensure point `1` moved to the new temp segment and is absent from the original
+    let &new_sid = diff.iter().next().expect("new segment id must exist");
+    let &old_sid = before_segment_ids.iter().next().expect("original segment id must exist");
+    let new_seg_read = holder.get(*new_sid).unwrap().get().read();
+    assert!(new_seg_read.has_point(1.into()), "new segment should contain point 1 after upsert");
+    drop(new_seg_read);
+    let old_seg_read = holder.get(old_sid).unwrap().get().read();
+    assert!(
+        !old_seg_read.has_point(1.into()),
+        "original segment should not contain point 1 after delete"
+    );

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.

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.