[MOD-17795] Compare merge scores exactly, so a mid-ingest label is not returned twice

src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h

@@ -45,6 +45,7 @@ INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_insertJobAsync_Test)
 INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_insertJobAsyncMulti_Test)
 INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_KNNSearch_Test)
 INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_MergeMulti_Test)
+INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_MergeSingleWithNearTiedScores_Test)
 INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_deleteFromHNSWMulti_Test)
 INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_deleteFromHNSWMultiLevels_Test)
 INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_AdHocSingle_Test)

src/VecSim/algorithms/svs/svs_tiered.h

@@ -1066,21 +1066,6 @@ class TieredSVSIndex : public VecSimTieredIndex<DataType, float> {
         return infoIterator;
     }
 
-    VecSimQueryReply *topKQuery(const void *queryBlob, size_t k,
-                                VecSimQueryParams *queryParams) const override {
-        // SVS implements it's own distance computation functions which may cause sligthly different
-        // distance values than VecSim Flat Index does, so we always have to merge results with set.
-        return this->template topKQueryImp<true>(queryBlob, k, queryParams);
-    }
-
-    VecSimQueryReply *rangeQuery(const void *queryBlob, double radius,
-                                 VecSimQueryParams *queryParams,
-                                 VecSimQueryReply_Order order) const override {
-        // SVS implements it's own distance computation functions which may cause sligthly different
-        // distance values than VecSim Flat Index does, so we always have to merge results with set.
-        return this->template rangeQueryImp<true>(queryBlob, radius, queryParams, order);
-    }
-
     VecSimBatchIterator *newBatchIterator(const void *queryBlob,
                                           VecSimQueryParams *queryParams) const override {
         // The query blob will be processed and copied by the internal indexes's batch iterator.

src/VecSim/utils/query_result_utils.h

@@ -11,15 +11,18 @@
 #include "VecSim/query_result_definitions.h"
 #include <VecSim/utils/vec_utils.h>
 
-#define VECSIM_EPSILON (1e-6)
-
-inline bool double_eq(double a, double b) { return fabs(a - b) < VECSIM_EPSILON; }
-
-// Compare two results by score, and if the scores are equal, by id.
+// Compare two results by score, and if the scores are equal, by id. The score comparison must be
+// exact: merge_results() walks lists that are ordered by exact score, and any tolerance here makes
+// this comparator disagree with that order, which breaks the merge's duplicate detection.
 inline int cmpVecSimQueryResultByScoreThenId(const VecSimQueryResultContainer::iterator res1,
                                              const VecSimQueryResultContainer::iterator res2) {
-    return !double_eq(res1->score, res2->score) ? (res1->score > res2->score ? 1 : -1)
-                                                : (int)(res1->id - res2->id);
+    if (res1->score != res2->score) {
+        return res1->score > res2->score ? 1 : -1;
+    }
+    if (res1->id == res2->id) {
+        return 0;
+    }
+    return res1->id > res2->id ? 1 : -1;
 }
 
 // Append the current result to the merged results, after verifying that it did not added yet (if
@@ -89,17 +92,82 @@ std::pair<size_t, size_t> merge_results(VecSimQueryResultContainer &results,
     return {cur_first - first.begin(), cur_second - second.begin()};
 }
 
-// Assumes that the arrays are sorted by score firstly and by id secondarily.
-// Use withSet=false if you can guarantee that shared ids between the two lists
-// will also have identical scores. In this case, any duplicates will naturally align
-// at the front of both lists during the merge, so they can be removed without explicitly
-// tracking seen ids — enabling a more efficient merge.
-template <bool withSet>
-VecSimQueryReply *merge_result_lists(VecSimQueryReply *first, VecSimQueryReply *second,
-                                     size_t limit) {
+// Each input contains at most one result per label, so duplicates in a one-shot tier merge can only
+// occur across the two inputs. Therefore, tracking IDs from the smaller input is sufficient. Batch
+// iterators cannot use this optimization because they also need to remember labels returned by
+// earlier calls.
+inline std::pair<size_t, size_t>
+merge_results_with_cross_tier_dedup(VecSimQueryResultContainer &results,
+                                    VecSimQueryResultContainer &first,
+                                    VecSimQueryResultContainer &second, size_t limit) {
+    results.reserve(std::min(limit, first.size() + second.size()));
+    const auto &smaller = first.size() <= second.size() ? first : second;
+    // The mapped bool records whether this tracked ID has already been emitted.
+    vecsim_stl::unordered_map<labelType, bool> tracked_ids(0, smaller.getAllocator());
+    tracked_ids.reserve(smaller.size());
+    for (const auto &result : smaller) {
+        tracked_ids.emplace(result.id, false);
+    }
+    auto cur_first = first.begin();
+    auto cur_second = second.begin();
+
+    auto should_emit = [&](labelType id) {
+        auto it = tracked_ids.find(id);
+        if (it == tracked_ids.end()) {
+            return true;
+        }
+        if (it->second) {
+            return false;
+        }
+        it->second = true;
+        return true;
+    };
+
+    auto maybe_append = [&](auto &current) {
+        if (should_emit(current->id)) {
+            results.push_back(*current);
+            limit--;
+        }
+        current++;
+    };
+
+    while (limit && cur_first != first.end() && cur_second != second.end()) {
+        int cmp = cmpVecSimQueryResultByScoreThenId(cur_first, cur_second);
+        if (cmp > 0) {
+            maybe_append(cur_second);
+        } else if (cmp < 0) {
+            maybe_append(cur_first);
+        } else {
+            // Exact cross-tier duplicate. Per-input label uniqueness means it was not emitted
+            // earlier.
+            results.push_back(*cur_first);
+            cur_first++;
+            cur_second++;
+            limit--;
+        }
+    }
 
+    while (limit && cur_first != first.end()) {
+        maybe_append(cur_first);
+    }
+    while (limit && cur_second != second.end()) {
+        maybe_append(cur_second);
+    }
+
+    return {cur_first - first.begin(), cur_second - second.begin()};
+}
+
+// Assumes that the arrays are sorted by score firstly and by id secondarily.
+inline VecSimQueryReply *merge_result_lists(VecSimQueryReply *first, VecSimQueryReply *second,
+                                            size_t limit) {
     auto mergedResults = new VecSimQueryReply(first->results.getAllocator());
-    merge_results<withSet>(mergedResults->results, first->results, second->results, limit);
+    if (limit <= 1 || first->results.empty() || second->results.empty()) {
+        // At most one result or only one source means a cross-tier duplicate cannot be emitted.
+        merge_results<false>(mergedResults->results, first->results, second->results, limit);
+    } else {
+        merge_results_with_cross_tier_dedup(mergedResults->results, first->results, second->results,
+                                            limit);
+    }
 
     VecSimQueryReply_Free(first);
     VecSimQueryReply_Free(second);
@@ -115,8 +183,7 @@ static inline void concat_results(VecSimQueryReply *first, VecSimQueryReply *sec
 // Sorts the results by id and removes duplicates.
 // Assumes that a result can appear at most twice in the results list.
 // @returns the number of unique results. This should be set to be the new length of the results
-template <bool IsMulti>
-void filter_results_by_id(VecSimQueryReply *results) {
+inline void filter_results_by_id(VecSimQueryReply *results) {
     if (VecSimQueryReply_Len(results) < 2) {
         return;
     }
@@ -127,17 +194,11 @@ void filter_results_by_id(VecSimQueryReply *results) {
         const VecSimQueryResult *cur_res = results->results.data() + i;
         const VecSimQueryResult *next_res = cur_res + 1;
         if (VecSimQueryResult_GetId(cur_res) == VecSimQueryResult_GetId(next_res)) {
-            if (IsMulti) {
-                // On multi value index, scores might be different and we want to keep the lower
-                // score.
-                if (VecSimQueryResult_GetScore(cur_res) < VecSimQueryResult_GetScore(next_res)) {
-                    results->results[cur_end] = *cur_res;
-                } else {
-                    results->results[cur_end] = *next_res;
-                }
-            } else {
-                // On single value index, scores are the same so we can keep any of the results.
+            // Cross-tier copies may have different scores, so keep the better one.
+            if (VecSimQueryResult_GetScore(cur_res) < VecSimQueryResult_GetScore(next_res)) {
                 results->results[cur_end] = *cur_res;
+            } else {
+                results->results[cur_end] = *next_res;
             }
             // Assuming every id can appear at most twice, we can skip the next comparison between
             // the current and the next result.

src/VecSim/vec_sim_tiered_index.h

@@ -101,15 +101,9 @@ class VecSimTieredIndex : public VecSimIndexInterface {
 public:
     int getMainIndexGuardWriteLockCount() const { return mainIndexGuard_write_lock_count; }
 #endif
-    // For both topK and range, Use withSet=false if you can guarantee that shared ids between the
-    // two lists will also have identical scores. In this case, any duplicates will naturally align
-    // at the front of both lists during the merge, so they can be removed without explicitly
-    // tracking seen ids — enabling a more efficient merge.
-    template <bool WithSet>
     VecSimQueryReply *topKQueryImp(const void *queryBlob, size_t k,
                                    VecSimQueryParams *queryParams) const;
 
-    template <bool WithSet>
     VecSimQueryReply *rangeQueryImp(const void *queryBlob, double radius,
                                     VecSimQueryParams *queryParams,
                                     VecSimQueryReply_Order order) const;
@@ -167,7 +161,6 @@ class VecSimTieredIndex : public VecSimIndexInterface {
 };
 
 template <typename DataType, typename DistType>
-template <bool withSet>
 VecSimQueryReply *
 VecSimTieredIndex<DataType, DistType>::topKQueryImp(const void *queryBlob, size_t k,
                                                     VecSimQueryParams *queryParams) const {
@@ -214,41 +207,25 @@ VecSimTieredIndex<DataType, DistType>::topKQueryImp(const void *queryBlob, size_
             return main_results;
         }
 
-        return merge_result_lists<withSet>(main_results, flat_results, k);
+        return merge_result_lists(main_results, flat_results, k);
     }
 }
 template <typename DataType, typename DistType>
 VecSimQueryReply *
 VecSimTieredIndex<DataType, DistType>::topKQuery(const void *queryBlob, size_t k,
                                                  VecSimQueryParams *queryParams) const {
-    if (this->backendIndex->isMultiValue()) {
-        return this->topKQueryImp<true>(queryBlob, k, queryParams); // Multi-value index
-    } else {
-        // Calling with withSet=false for optimized performance, assuming that shared IDs across
-        // lists also have identical scores — in which case duplicates are implicitly avoided by the
-        // merge logic.
-        return this->topKQueryImp<false>(queryBlob, k, queryParams);
-    }
+    return this->topKQueryImp(queryBlob, k, queryParams);
 }
 
 template <typename DataType, typename DistType>
 VecSimQueryReply *
 VecSimTieredIndex<DataType, DistType>::rangeQuery(const void *queryBlob, double radius,
                                                   VecSimQueryParams *queryParams,
                                                   VecSimQueryReply_Order order) const {
-    if (this->backendIndex->isMultiValue()) {
-        return this->rangeQueryImp<true>(queryBlob, radius, queryParams,
-                                         order); // Multi-value index
-    } else {
-        // Calling with withSet=false for optimized performance, assuming that shared IDs across
-        // lists also have identical scores — in which case duplicates are implicitly avoided by the
-        // merge logic.
-        return this->rangeQueryImp<false>(queryBlob, radius, queryParams, order);
-    }
+    return this->rangeQueryImp(queryBlob, radius, queryParams, order);
 }
 
 template <typename DataType, typename DistType>
-template <bool withSet>
 VecSimQueryReply *
 VecSimTieredIndex<DataType, DistType>::rangeQueryImp(const void *queryBlob, double radius,
                                                      VecSimQueryParams *queryParams,
@@ -301,15 +278,15 @@ VecSimTieredIndex<DataType, DistType>::rangeQueryImp(const void *queryBlob, doub
             auto code = main_results->code;
 
             // Merge the sorted results with no limit (all the results are valid).
-            VecSimQueryReply *ret = merge_result_lists<withSet>(main_results, flat_results, -1);
+            VecSimQueryReply *ret = merge_result_lists(main_results, flat_results, -1);
             // Restore the return code and return.
             ret->code = code;
             return ret;
 
         } else { // BY_ID
             // Notice that we don't modify the return code of the main index in any step.
             concat_results(main_results, flat_results);
-            filter_results_by_id<withSet>(main_results);
+            filter_results_by_id(main_results);
             return main_results;
         }
     }

tests/unit/test_common.cpp

@@ -11,6 +11,7 @@
 #include "VecSim/vec_sim.h"
 #include "VecSim/vec_sim_debug.h"
 #include "VecSim/query_result_definitions.h"
+#include "VecSim/utils/query_result_utils.h"
 #include "VecSim/utils/updatable_heap.h"
 #include "VecSim/utils/vec_utils.h"
 #include "unit_test_utils.h"
@@ -870,18 +871,107 @@ TEST(CommonAPITest, SearchDifferentScores) {
     // Verify results ordered by increasing score (distance).
     double prev_score = 0; // all scores are positive
     auto verify_by_score = [&](size_t id, double score, size_t res_index) {
+        ASSERT_LT(res_index, expected_results_by_score.size());
         ASSERT_LT(prev_score, score); // prev_score < score
         prev_score = score;
         ASSERT_EQ(id, expected_results_by_score[res_index].first);
         ASSERT_EQ(score, expected_results_by_score[res_index].second);
     };
 
-    runTopKTieredIndexSearchTest<true>(tiered_index, query_0, k, verify_by_score, nullptr);
+    runTopKSearchTest(tiered_index, query_0, k, verify_by_score);
     // Reset score tracking for range query
     prev_score = 0;
     // Use the largest score as the range to include all vectors
     double range = expected_results_by_score.back().second;
-    runRangeTieredIndexSearchTest<true>(tiered_index, query_0, range, verify_by_score, k, BY_SCORE);
+    runRangeQueryTest(tiered_index, query_0, range, verify_by_score, k, BY_SCORE);
+
+    std::vector<ResultPair> expected_results_by_id = expected_results_by_score;
+    std::sort(expected_results_by_id.begin(), expected_results_by_id.end());
+    size_t prev_id = 0;
+    auto verify_by_id = [&](size_t id, double score, size_t res_index) {
+        ASSERT_LT(res_index, expected_results_by_id.size());
+        ASSERT_LT(prev_id, id);
+        prev_id = id;
+        ASSERT_EQ(id, expected_results_by_id[res_index].first);
+        ASSERT_EQ(score, expected_results_by_id[res_index].second);
+    };
+    runRangeQueryTest(tiered_index, query_0, range, verify_by_id, k, BY_ID);
+}
+
+TEST(CommonAPITest, MergeResultListsWithCrossTierDedup) {
+    const auto allocator = VecSimAllocator::newVecsimAllocator();
+    const size_t large_id = std::numeric_limits<size_t>::max() - 1;
+    const std::vector<VecSimQueryResult> first = {
+        {.id = 10, .score = 0.1},       {.id = 42, .score = 0.4}, {.id = 50, .score = 0.5},
+        {.id = large_id, .score = 0.7}, {.id = 99, .score = 0.8}, {.id = 70, .score = 1.0},
+    };
+    const std::vector<VecSimQueryResult> second = {
+        {.id = 42, .score = 0.2},       {.id = 20, .score = 0.3}, {.id = 50, .score = 0.5},
+        {.id = large_id, .score = 0.6}, {.id = 99, .score = 0.9},
+    };
+    const std::vector<VecSimQueryResult> expected = {
+        {.id = 10, .score = 0.1}, {.id = 42, .score = 0.2},       {.id = 20, .score = 0.3},
+        {.id = 50, .score = 0.5}, {.id = large_id, .score = 0.6}, {.id = 99, .score = 0.8},
+        {.id = 70, .score = 1.0},
+    };
+
+    auto make_reply = [&](const auto &input) {
+        auto *reply = new VecSimQueryReply(allocator);
+        reply->results.insert(reply->results.end(), input.begin(), input.end());
+        return reply;
+    };
+
+    // Exercise both branches that select the smaller input. The result must keep whichever
+    // occurrence appears first in exact (score, id) order and continue past skipped duplicates to
+    // fill the requested number of unique results.
+    for (bool reverse_inputs : {false, true}) {
+        auto *first_reply = make_reply(reverse_inputs ? second : first);
+        auto *second_reply = make_reply(reverse_inputs ? first : second);
+        auto *merged = merge_result_lists(first_reply, second_reply, expected.size());
+
+        ASSERT_EQ(merged->results.size(), expected.size());
+        for (size_t i = 0; i < expected.size(); ++i) {
+            EXPECT_EQ(merged->results[i].id, expected[i].id) << "result index " << i;
+            EXPECT_DOUBLE_EQ(merged->results[i].score, expected[i].score) << "result index " << i;
+        }
+        VecSimQueryReply_Free(merged);
+    }
+
+    // Returning at most one result cannot emit a duplicate, even when the same ID has different
+    // scores in the two inputs. The exact merge order must still retain the better score.
+    const std::vector<VecSimQueryResult> one_result_first = {
+        {.id = 42, .score = 0.2},
+        {.id = 10, .score = 0.8},
+    };
+    const std::vector<VecSimQueryResult> one_result_second = {
+        {.id = 42, .score = 0.7},
+        {.id = 20, .score = 0.9},
+    };
+    for (bool reverse_inputs : {false, true}) {
+        auto *first_reply = make_reply(reverse_inputs ? one_result_second : one_result_first);
+        auto *second_reply = make_reply(reverse_inputs ? one_result_first : one_result_second);
+        auto *merged = merge_result_lists(first_reply, second_reply, 1);
+
+        ASSERT_EQ(merged->results.size(), 1);
+        EXPECT_EQ(merged->results[0].id, 42);
+        EXPECT_DOUBLE_EQ(merged->results[0].score, 0.2);
+        VecSimQueryReply_Free(merged);
+    }
+
+    // With only one source, there cannot be a cross-tier duplicate.
+    for (bool empty_first : {false, true}) {
+        const std::vector<VecSimQueryResult> empty;
+        auto *first_reply = make_reply(empty_first ? empty : first);
+        auto *second_reply = make_reply(empty_first ? first : empty);
+        auto *merged = merge_result_lists(first_reply, second_reply, first.size());
+
+        ASSERT_EQ(merged->results.size(), first.size());
+        for (size_t i = 0; i < first.size(); ++i) {
+            EXPECT_EQ(merged->results[i].id, first[i].id) << "result index " << i;
+            EXPECT_DOUBLE_EQ(merged->results[i].score, first[i].score) << "result index " << i;
+        }
+        VecSimQueryReply_Free(merged);
+    }
 }
 
 class CommonTypeMetricTests : public testing::TestWithParam<std::tuple<VecSimType, VecSimMetric>> {