From eff245d84079d34ff765f5d916fbe4a2d7f45dec Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Luis=20Coss=C3=ADo?= <luis.cossio@qdrant.com>
Date: Sat, 29 Jun 2024 16:38:02 -0400
Subject: [PATCH] order_by: Order by value, then id. Dedup by both (#4580)

* order by value, then id. Dedup by both

* order values properly, but dedup by id only

---------

Co-authored-by: generall <andrey@vasnetsov.com>
---
 lib/collection/src/collection/point_ops.rs | 10 ++++++----
 lib/collection/src/tests/points_dedup.rs   |  2 +-
 lib/segment/src/data_types/order_by.rs     |  9 +++++++++
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/lib/collection/src/collection/point_ops.rs b/lib/collection/src/collection/point_ops.rs
index 480f7bad7a..dc1a4a50e0 100644
--- a/lib/collection/src/collection/point_ops.rs
+++ b/lib/collection/src/collection/point_ops.rs
@@ -319,11 +319,13 @@ impl Collection {
                         })
                     })
                     // Get top results
-                    .kmerge_by(|(value_a, _), (value_b, _)| match order_by.direction() {
-                        Direction::Asc => value_a <= value_b,
-                        Direction::Desc => value_a >= value_b,
+                    .kmerge_by(|(value_a, record_a), (value_b, record_b)| {
+                        match order_by.direction() {
+                            Direction::Asc => (value_a, record_a.id) < (value_b, record_b.id),
+                            Direction::Desc => (value_a, record_a.id) > (value_b, record_b.id),
+                        }
                     })
-                    // Add each point only once, deduplicate point IDs
+                    // Only keep the point with the most "valuable" order value
                     .dedup_by(|(_, record_a), (_, record_b)| record_a.id == record_b.id)
                     .map(|(_, record)| api::rest::Record::from(record))
                     .take(limit)
diff --git a/lib/collection/src/tests/points_dedup.rs b/lib/collection/src/tests/points_dedup.rs
index 3019a478fe..519c1f1855 100644
--- a/lib/collection/src/tests/points_dedup.rs
+++ b/lib/collection/src/tests/points_dedup.rs
@@ -195,7 +195,7 @@ async fn test_scroll_dedup() {
     for point_id in result.points.iter().map(|point| point.id) {
         assert!(
             seen.insert(point_id),
-            "got point id {point_id} more than once, they should be deduplicated",
+            "got point id {point_id:?} more than once, they should be deduplicated",
         );
     }
 }
diff --git a/lib/segment/src/data_types/order_by.rs b/lib/segment/src/data_types/order_by.rs
index 795330c8af..240eff5fd4 100644
--- a/lib/segment/src/data_types/order_by.rs
+++ b/lib/segment/src/data_types/order_by.rs
@@ -147,6 +147,15 @@ pub enum OrderValue {
     Float(FloatPayloadType),
 }
 
+#[cfg(any(test, feature = "testing"))]
+impl std::hash::Hash for OrderValue {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        match self {
+            OrderValue::Int(i) => i.hash(state),
+            OrderValue::Float(f) => f.to_bits().hash(state),
+        }
+    }
+}
 impl OrderValue {
     const MAX: Self = Self::Float(f64::NAN);
     const MIN: Self = Self::Float(f64::MIN);