From 144216bcdffa2d1e35e84303005c42bcab83f818 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Sun, 23 Aug 2026 14:26:07 +0300 Subject: [PATCH 1/6] [MOD-17795] Compare merge scores exactly, so a mid-ingest label is not returned twice A tiered top-K or range query merges the frontend and the backend result lists, and for a single-value index it relies on a shared id reaching both merge cursors at the same time rather than tracking seen ids. That holds only while the comparator agrees with the order the two lists arrive in. It did not. Both lists are ordered by exact score - HNSWIndex::topKQuery and BruteForceIndex::topKQuery drain a max-heap keyed on (distance, label), and the batch iterators sort by score then id - while cmpVecSimQueryResultByScoreThenId() called any two scores within 1e-6 equal and fell back to ordering by id. Wherever those two orders disagree, the copy of a label held by the flat buffer and the copy held by the backend miss each other, and both end up in the reply. A label is in both indexes for as long as an ingest job takes to get from inserting into HNSW to erasing from the flat buffer, so a corpus dense enough to put unequal distances within 1e-6 of each other duplicates documents under any sustained write load - and each duplicate costs a real document out of a full reply. Drop the tolerance. The rewrite also fixes the id comparison, which truncated a 64-bit label difference to int and so gave the wrong sign for ids more than 2^31 apart. Co-Authored-By: Claude Opus 5 (1M context) --- .../hnsw/hnsw_tiered_tests_friends.h | 1 + src/VecSim/utils/query_result_utils.h | 21 +++++--- src/VecSim/vec_sim_tiered_index.h | 7 ++- tests/unit/test_hnsw_tiered.cpp | 50 +++++++++++++++++++ 4 files changed, 67 insertions(+), 12 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h b/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h index d4d5cd999..531a68500 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h +++ b/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) diff --git a/src/VecSim/utils/query_result_utils.h b/src/VecSim/utils/query_result_utils.h index 5bb8f35c9..746c06f30 100644 --- a/src/VecSim/utils/query_result_utils.h +++ b/src/VecSim/utils/query_result_utils.h @@ -11,15 +11,18 @@ #include "VecSim/query_result_definitions.h" #include -#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 @@ -93,7 +96,9 @@ std::pair merge_results(VecSimQueryResultContainer &results, // 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. +// tracking seen ids — enabling a more efficient merge. Note that "identical" means bit-identical: +// a shared id whose two scores merely round to the same value does not align, so a backend that +// scores a vector differently than the frontend does (a compressed one, say) needs withSet=true. template VecSimQueryReply *merge_result_lists(VecSimQueryReply *first, VecSimQueryReply *second, size_t limit) { diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index d097b1ae4..4043c9970 100644 --- a/src/VecSim/vec_sim_tiered_index.h +++ b/src/VecSim/vec_sim_tiered_index.h @@ -101,10 +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. + // For both topK and range, see merge_results() for when withSet=false is sound. It holds for a + // frontend/backend merge only while the two indexes score a given vector identically, which a + // compressed backend does not - see TieredSVSIndex, which overrides both to pass withSet=true. template VecSimQueryReply *topKQueryImp(const void *queryBlob, size_t k, VecSimQueryParams *queryParams) const; diff --git a/tests/unit/test_hnsw_tiered.cpp b/tests/unit/test_hnsw_tiered.cpp index a0927790b..2b91c882a 100644 --- a/tests/unit/test_hnsw_tiered.cpp +++ b/tests/unit/test_hnsw_tiered.cpp @@ -918,6 +918,56 @@ TYPED_TEST(HNSWTieredIndexTestBasic, MergeMulti) { runTopKSearchTest(tiered_index, query, 5, [](size_t _, double __, size_t ___) {}); } +// A label that is mid-ingest lives in the HNSW index and in the flat buffer at once, so +// a top-K query merges two lists that both carry it. Both lists are ordered by exact score, while +// cmpVecSimQueryResultByScoreThenId() used to call scores within 1e-6 equal and fall back to +// ordering by id. Where those two orders disagree the shared label does not reach the two merge +// cursors at the same time, and a merge that dedups by cursor alignment alone emits it twice. +TYPED_TEST(HNSWTieredIndexTestBasic, MergeSingleWithNearTiedScores) { + size_t dim = 4; + + HNSWParams params = { + .type = TypeParam::get_index_type(), + .dim = dim, + .metric = VecSimMetric_L2, + .multi = false, + }; + VecSimParams hnsw_params = CreateParams(params); + auto mock_thread_pool = tieredIndexMock(); + + auto *tiered_index = this->CreateTieredHNSWIndex(hnsw_params, mock_thread_pool); + auto allocator = tiered_index->getAllocator(); + + // Label 1 is the farther of the two from the query, and carries the smaller id. That + // combination is what makes the orders disagree: by exact score label 1 comes second, by id it + // comes first. + GenerateAndAddVector(tiered_index->backendIndex, dim, 1, 0.5000001); + GenerateAndAddVector(tiered_index->backendIndex, dim, 2, 0.5); + // An ingest job has already inserted label 1 into HNSW but not yet erased it from the flat + // buffer, so both indexes hold it - with the same vector, and so the same score. + GenerateAndAddVector(tiered_index->frontendIndex, dim, 1, 0.5000001); + + TEST_DATA_T query[dim]; + GenerateVector(query, dim, 0); + + // The regression needs the two distances to differ, but by less than the tolerance the merge + // comparator used to treat as equality. Assert it, so that a change in the vectors or in the + // data type fails here rather than silently retiring the test. + auto backend_res = + VecSimIndex_TopKQuery(tiered_index->backendIndex, query, 2, nullptr, BY_SCORE); + ASSERT_EQ(VecSimQueryReply_Len(backend_res), 2); + double closer = VecSimQueryResult_GetScore(backend_res->results.data()); + double farther = VecSimQueryResult_GetScore(backend_res->results.data() + 1); + VecSimQueryReply_Free(backend_res); + ASSERT_LT(closer, farther); + ASSERT_LT(farther - closer, 1e-6); + + // k exceeds the label count on purpose: when the merge fills exactly k results the truncation + // hides the second copy, which is why a query for as many results as the index holds does not + // expose this. + runTopKSearchTest(tiered_index, query, 3, [](size_t _, double __, size_t ___) {}); +} + TYPED_TEST(HNSWTieredIndexTest, deleteFromHNSWBasic) { // Create TieredHNSW index instance with a mock queue. size_t dim = 4; From 9f1b185389b0e829174e0d01224143734339921f Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Tue, 25 Aug 2026 09:17:43 +0300 Subject: [PATCH 2/6] [MOD-17795] Optimize one-shot tier result deduplication --- src/VecSim/utils/query_result_utils.h | 141 +++++++++++++++++++++++++- tests/unit/test_common.cpp | 41 ++++++++ 2 files changed, 181 insertions(+), 1 deletion(-) diff --git a/src/VecSim/utils/query_result_utils.h b/src/VecSim/utils/query_result_utils.h index 746c06f30..ac09c81fa 100644 --- a/src/VecSim/utils/query_result_utils.h +++ b/src/VecSim/utils/query_result_utils.h @@ -11,6 +11,10 @@ #include "VecSim/query_result_definitions.h" #include +#include +#include +#include + // 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. @@ -25,6 +29,91 @@ inline int cmpVecSimQueryResultByScoreThenId(const VecSimQueryResultContainer::i return res1->id > res2->id ? 1 : -1; } +// One-shot tier merges only need to track IDs that occur in the smaller input: an ID absent from +// that input cannot be a cross-tier duplicate. Keeping the emitted bit in the same open-addressed +// slot avoids the allocation and cache cost of inserting every output into a node-based set. +class CrossTierIdTracker { + enum class State : uint8_t { Empty, NotEmitted, Emitted }; + + struct Slot { + size_t id = 0; + State state = State::Empty; + }; + + vecsim_stl::vector slots; + size_t mask = 0; + + static size_t hashId(size_t id) { + if constexpr (sizeof(size_t) == sizeof(uint64_t)) { + uint64_t value = id; + value ^= value >> 30; + value *= UINT64_C(0xbf58476d1ce4e5b9); + value ^= value >> 27; + value *= UINT64_C(0x94d049bb133111eb); + value ^= value >> 31; + return value; + } else { + uint32_t value = id; + value ^= value >> 16; + value *= UINT32_C(0x7feb352d); + value ^= value >> 15; + value *= UINT32_C(0x846ca68b); + value ^= value >> 16; + return value; + } + } + + Slot *find(size_t id) { + if (slots.empty()) { + return nullptr; + } + size_t position = hashId(id) & mask; + while (slots[position].state != State::Empty) { + if (slots[position].id == id) { + return &slots[position]; + } + position = (position + 1) & mask; + } + return nullptr; + } + +public: + explicit CrossTierIdTracker(const VecSimQueryResultContainer &smaller) + : slots(smaller.getAllocator()) { + if (smaller.empty()) { + return; + } + + assert(smaller.size() <= std::numeric_limits::max() / 4); + size_t capacity = 2; + while (capacity < smaller.size() * 2) { + capacity *= 2; + } + slots.resize(capacity); + mask = capacity - 1; + + for (const auto &result : smaller) { + size_t position = hashId(result.id) & mask; + while (slots[position].state != State::Empty && slots[position].id != result.id) { + position = (position + 1) & mask; + } + slots[position] = {.id = result.id, .state = State::NotEmitted}; + } + } + + bool shouldEmit(size_t id) { + auto *slot = find(id); + if (slot == nullptr) { + return true; + } + if (slot->state == State::Emitted) { + return false; + } + slot->state = State::Emitted; + return true; + } +}; + // Append the current result to the merged results, after verifying that it did not added yet (if // verification is needed). Also update the set, limit and the current result. template @@ -92,6 +181,51 @@ std::pair merge_results(VecSimQueryResultContainer &results, return {cur_first - first.begin(), cur_second - second.begin()}; } +// Each index query returns at most one result per label, so duplicates in a one-shot tier merge can +// only occur across the inputs. Batch iterators cannot use this optimization because they also need +// to remember labels returned by earlier calls. +inline std::pair +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; + CrossTierIdTracker tracker(smaller); + auto cur_first = first.begin(); + auto cur_second = second.begin(); + + auto maybe_append = [&](auto ¤t) { + if (tracker.shouldEmit(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 { + 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. // 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 @@ -104,7 +238,12 @@ VecSimQueryReply *merge_result_lists(VecSimQueryReply *first, VecSimQueryReply * size_t limit) { auto mergedResults = new VecSimQueryReply(first->results.getAllocator()); - merge_results(mergedResults->results, first->results, second->results, limit); + if constexpr (withSet) { + merge_results_with_cross_tier_dedup(mergedResults->results, first->results, second->results, + limit); + } else { + merge_results(mergedResults->results, first->results, second->results, limit); + } VecSimQueryReply_Free(first); VecSimQueryReply_Free(second); diff --git a/tests/unit/test_common.cpp b/tests/unit/test_common.cpp index 35a269311..a937d42cf 100644 --- a/tests/unit/test_common.cpp +++ b/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" @@ -884,6 +885,46 @@ TEST(CommonAPITest, SearchDifferentScores) { runRangeTieredIndexSearchTest(tiered_index, query_0, range, verify_by_score, k, BY_SCORE); } +TEST(CommonAPITest, MergeResultListsWithCrossTierDedup) { + const auto allocator = VecSimAllocator::newVecsimAllocator(); + const size_t large_id = std::numeric_limits::max() - 1; + const std::vector 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 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 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); + } +} + class CommonTypeMetricTests : public testing::TestWithParam> { protected: template From c045b8b0f998039bade00bbb9924f2f300da4311 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Tue, 25 Aug 2026 10:45:46 +0300 Subject: [PATCH 3/6] [MOD-17795] Deduplicate public one-shot tier queries --- src/VecSim/vec_sim_tiered_index.h | 19 ++----------------- tests/unit/test_common.cpp | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index 4043c9970..714460cac 100644 --- a/src/VecSim/vec_sim_tiered_index.h +++ b/src/VecSim/vec_sim_tiered_index.h @@ -220,14 +220,7 @@ template VecSimQueryReply * VecSimTieredIndex::topKQuery(const void *queryBlob, size_t k, VecSimQueryParams *queryParams) const { - if (this->backendIndex->isMultiValue()) { - return this->topKQueryImp(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(queryBlob, k, queryParams); - } + return this->topKQueryImp(queryBlob, k, queryParams); } template @@ -235,15 +228,7 @@ VecSimQueryReply * VecSimTieredIndex::rangeQuery(const void *queryBlob, double radius, VecSimQueryParams *queryParams, VecSimQueryReply_Order order) const { - if (this->backendIndex->isMultiValue()) { - return this->rangeQueryImp(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(queryBlob, radius, queryParams, order); - } + return this->rangeQueryImp(queryBlob, radius, queryParams, order); } template diff --git a/tests/unit/test_common.cpp b/tests/unit/test_common.cpp index a937d42cf..0855b68fc 100644 --- a/tests/unit/test_common.cpp +++ b/tests/unit/test_common.cpp @@ -871,18 +871,31 @@ 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(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(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 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) { From e0c12b589472b8baad25f9a09d9ad6c9bbf5588c Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Tue, 25 Aug 2026 11:20:15 +0300 Subject: [PATCH 4/6] [MOD-17795] Make one-shot tier deduplication mandatory --- src/VecSim/algorithms/svs/svs_tiered.h | 15 ---------- src/VecSim/utils/query_result_utils.h | 38 +++++++------------------- src/VecSim/vec_sim_tiered_index.h | 17 ++++-------- tests/unit/test_common.cpp | 2 +- tests/unit/unit_test_utils.cpp | 31 --------------------- tests/unit/unit_test_utils.h | 12 -------- 6 files changed, 16 insertions(+), 99 deletions(-) diff --git a/src/VecSim/algorithms/svs/svs_tiered.h b/src/VecSim/algorithms/svs/svs_tiered.h index 535920365..edb950121 100644 --- a/src/VecSim/algorithms/svs/svs_tiered.h +++ b/src/VecSim/algorithms/svs/svs_tiered.h @@ -1066,21 +1066,6 @@ class TieredSVSIndex : public VecSimTieredIndex { 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(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(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. diff --git a/src/VecSim/utils/query_result_utils.h b/src/VecSim/utils/query_result_utils.h index ac09c81fa..9f0984af0 100644 --- a/src/VecSim/utils/query_result_utils.h +++ b/src/VecSim/utils/query_result_utils.h @@ -43,6 +43,7 @@ class CrossTierIdTracker { vecsim_stl::vector slots; size_t mask = 0; + // The table indexes by masking the low bits, so mix patterned labels before applying the mask. static size_t hashId(size_t id) { if constexpr (sizeof(size_t) == sizeof(uint64_t)) { uint64_t value = id; @@ -227,23 +228,11 @@ merge_results_with_cross_tier_dedup(VecSimQueryResultContainer &results, } // 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. Note that "identical" means bit-identical: -// a shared id whose two scores merely round to the same value does not align, so a backend that -// scores a vector differently than the frontend does (a compressed one, say) needs withSet=true. -template -VecSimQueryReply *merge_result_lists(VecSimQueryReply *first, VecSimQueryReply *second, - size_t limit) { - +inline VecSimQueryReply *merge_result_lists(VecSimQueryReply *first, VecSimQueryReply *second, + size_t limit) { auto mergedResults = new VecSimQueryReply(first->results.getAllocator()); - if constexpr (withSet) { - merge_results_with_cross_tier_dedup(mergedResults->results, first->results, second->results, - limit); - } else { - merge_results(mergedResults->results, first->results, second->results, limit); - } + merge_results_with_cross_tier_dedup(mergedResults->results, first->results, second->results, + limit); VecSimQueryReply_Free(first); VecSimQueryReply_Free(second); @@ -259,8 +248,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 -void filter_results_by_id(VecSimQueryReply *results) { +inline void filter_results_by_id(VecSimQueryReply *results) { if (VecSimQueryReply_Len(results) < 2) { return; } @@ -271,17 +259,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. diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index 714460cac..db17c02c5 100644 --- a/src/VecSim/vec_sim_tiered_index.h +++ b/src/VecSim/vec_sim_tiered_index.h @@ -101,14 +101,9 @@ class VecSimTieredIndex : public VecSimIndexInterface { public: int getMainIndexGuardWriteLockCount() const { return mainIndexGuard_write_lock_count; } #endif - // For both topK and range, see merge_results() for when withSet=false is sound. It holds for a - // frontend/backend merge only while the two indexes score a given vector identically, which a - // compressed backend does not - see TieredSVSIndex, which overrides both to pass withSet=true. - template VecSimQueryReply *topKQueryImp(const void *queryBlob, size_t k, VecSimQueryParams *queryParams) const; - template VecSimQueryReply *rangeQueryImp(const void *queryBlob, double radius, VecSimQueryParams *queryParams, VecSimQueryReply_Order order) const; @@ -166,7 +161,6 @@ class VecSimTieredIndex : public VecSimIndexInterface { }; template -template VecSimQueryReply * VecSimTieredIndex::topKQueryImp(const void *queryBlob, size_t k, VecSimQueryParams *queryParams) const { @@ -213,14 +207,14 @@ VecSimTieredIndex::topKQueryImp(const void *queryBlob, size_ return main_results; } - return merge_result_lists(main_results, flat_results, k); + return merge_result_lists(main_results, flat_results, k); } } template VecSimQueryReply * VecSimTieredIndex::topKQuery(const void *queryBlob, size_t k, VecSimQueryParams *queryParams) const { - return this->topKQueryImp(queryBlob, k, queryParams); + return this->topKQueryImp(queryBlob, k, queryParams); } template @@ -228,11 +222,10 @@ VecSimQueryReply * VecSimTieredIndex::rangeQuery(const void *queryBlob, double radius, VecSimQueryParams *queryParams, VecSimQueryReply_Order order) const { - return this->rangeQueryImp(queryBlob, radius, queryParams, order); + return this->rangeQueryImp(queryBlob, radius, queryParams, order); } template -template VecSimQueryReply * VecSimTieredIndex::rangeQueryImp(const void *queryBlob, double radius, VecSimQueryParams *queryParams, @@ -285,7 +278,7 @@ VecSimTieredIndex::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(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; @@ -293,7 +286,7 @@ VecSimTieredIndex::rangeQueryImp(const void *queryBlob, doub } 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(main_results); + filter_results_by_id(main_results); return main_results; } } diff --git a/tests/unit/test_common.cpp b/tests/unit/test_common.cpp index 0855b68fc..348ca346c 100644 --- a/tests/unit/test_common.cpp +++ b/tests/unit/test_common.cpp @@ -927,7 +927,7 @@ TEST(CommonAPITest, MergeResultListsWithCrossTierDedup) { 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()); + 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) { diff --git a/tests/unit/unit_test_utils.cpp b/tests/unit/unit_test_utils.cpp index 250959fb4..e33e83299 100644 --- a/tests/unit/unit_test_utils.cpp +++ b/tests/unit/unit_test_utils.cpp @@ -114,20 +114,6 @@ void runTopKSearchTest(VecSimIndex *index, const void *query, size_t k, validateTopKSearchTest(index, res, k, ResCB); } -template -void runTopKTieredIndexSearchTest(VecSimTieredIndex *index, const void *query, - size_t k, std::function ResCB, - VecSimQueryParams *params) { - ASSERT_NE(index, nullptr); - VecSimQueryReply *res = index->template topKQueryImp(query, k, params); - validateTopKSearchTest(index, res, k, ResCB); -} - -// Explicit template instantiations for float, float -template void runTopKTieredIndexSearchTest( - VecSimTieredIndex *, const void *, size_t, - std::function, VecSimQueryParams *); - /* * helper function to run batch search iteration, and iterate over the results. ResCB is a callback * that takes the id, score and index of a result, and performs test-specific logic for each. @@ -259,23 +245,6 @@ void runRangeQueryTest(VecSimIndex *index, const void *query, double radius, validateRangeQueryTest(res, ResCB, expected_res_num); } -template -void runRangeTieredIndexSearchTest(VecSimTieredIndex *index, const void *query, - double radius, - const std::function &ResCB, - size_t expected_res_num, VecSimQueryReply_Order order, - VecSimQueryParams *params) { - - VecSimQueryReply *res = index->template rangeQueryImp(query, radius, params, order); - validateRangeQueryTest(res, ResCB, expected_res_num); -} - -// Explicit template instantiations for float, float -template void runRangeTieredIndexSearchTest( - VecSimTieredIndex *, const void *, double, - const std::function &, size_t, VecSimQueryReply_Order, - VecSimQueryParams *); - void compareFlatIndexInfoToIterator(VecSimIndexDebugInfo info, VecSimDebugInfoIterator *infoIter, bool expect_shared_memory) { size_t extra = expect_shared_memory ? 1 : 0; diff --git a/tests/unit/unit_test_utils.h b/tests/unit/unit_test_utils.h index 1d4ec90ba..b6e3538f3 100644 --- a/tests/unit/unit_test_utils.h +++ b/tests/unit/unit_test_utils.h @@ -169,11 +169,6 @@ void runTopKSearchTest(VecSimIndex *index, const void *query, size_t k, VecSimQueryParams *params = nullptr, VecSimQueryReply_Order order = BY_SCORE); -template -void runTopKTieredIndexSearchTest(VecSimTieredIndex *index, const void *query, - size_t k, std::function ResCB, - VecSimQueryParams *params = nullptr); - void runBatchIteratorSearchTest(VecSimBatchIterator *batch_iterator, size_t n_res, std::function ResCB, VecSimQueryReply_Order order = BY_SCORE, @@ -211,13 +206,6 @@ void runRangeQueryTest(VecSimIndex *index, const void *query, double radius, size_t expected_res_num, VecSimQueryReply_Order order = BY_ID, VecSimQueryParams *params = nullptr); -template -void runRangeTieredIndexSearchTest(VecSimTieredIndex *index, const void *query, - double radius, - const std::function &ResCB, - size_t expected_res_num, VecSimQueryReply_Order order = BY_ID, - VecSimQueryParams *params = nullptr); - size_t getLabelsLookupNodeSize(); inline double GetInfVal(VecSimType type) { From 3e66c26ddd0ffe7c8eb74f094bc136be6521c5c2 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Tue, 25 Aug 2026 11:50:14 +0300 Subject: [PATCH 5/6] [MOD-17795] Use the standard map for tier deduplication --- src/VecSim/utils/query_result_utils.h | 120 ++++++-------------------- 1 file changed, 25 insertions(+), 95 deletions(-) diff --git a/src/VecSim/utils/query_result_utils.h b/src/VecSim/utils/query_result_utils.h index 9f0984af0..6c7e66259 100644 --- a/src/VecSim/utils/query_result_utils.h +++ b/src/VecSim/utils/query_result_utils.h @@ -11,10 +11,6 @@ #include "VecSim/query_result_definitions.h" #include -#include -#include -#include - // 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. @@ -29,92 +25,6 @@ inline int cmpVecSimQueryResultByScoreThenId(const VecSimQueryResultContainer::i return res1->id > res2->id ? 1 : -1; } -// One-shot tier merges only need to track IDs that occur in the smaller input: an ID absent from -// that input cannot be a cross-tier duplicate. Keeping the emitted bit in the same open-addressed -// slot avoids the allocation and cache cost of inserting every output into a node-based set. -class CrossTierIdTracker { - enum class State : uint8_t { Empty, NotEmitted, Emitted }; - - struct Slot { - size_t id = 0; - State state = State::Empty; - }; - - vecsim_stl::vector slots; - size_t mask = 0; - - // The table indexes by masking the low bits, so mix patterned labels before applying the mask. - static size_t hashId(size_t id) { - if constexpr (sizeof(size_t) == sizeof(uint64_t)) { - uint64_t value = id; - value ^= value >> 30; - value *= UINT64_C(0xbf58476d1ce4e5b9); - value ^= value >> 27; - value *= UINT64_C(0x94d049bb133111eb); - value ^= value >> 31; - return value; - } else { - uint32_t value = id; - value ^= value >> 16; - value *= UINT32_C(0x7feb352d); - value ^= value >> 15; - value *= UINT32_C(0x846ca68b); - value ^= value >> 16; - return value; - } - } - - Slot *find(size_t id) { - if (slots.empty()) { - return nullptr; - } - size_t position = hashId(id) & mask; - while (slots[position].state != State::Empty) { - if (slots[position].id == id) { - return &slots[position]; - } - position = (position + 1) & mask; - } - return nullptr; - } - -public: - explicit CrossTierIdTracker(const VecSimQueryResultContainer &smaller) - : slots(smaller.getAllocator()) { - if (smaller.empty()) { - return; - } - - assert(smaller.size() <= std::numeric_limits::max() / 4); - size_t capacity = 2; - while (capacity < smaller.size() * 2) { - capacity *= 2; - } - slots.resize(capacity); - mask = capacity - 1; - - for (const auto &result : smaller) { - size_t position = hashId(result.id) & mask; - while (slots[position].state != State::Empty && slots[position].id != result.id) { - position = (position + 1) & mask; - } - slots[position] = {.id = result.id, .state = State::NotEmitted}; - } - } - - bool shouldEmit(size_t id) { - auto *slot = find(id); - if (slot == nullptr) { - return true; - } - if (slot->state == State::Emitted) { - return false; - } - slot->state = State::Emitted; - return true; - } -}; - // Append the current result to the merged results, after verifying that it did not added yet (if // verification is needed). Also update the set, limit and the current result. template @@ -182,21 +92,39 @@ std::pair merge_results(VecSimQueryResultContainer &results, return {cur_first - first.begin(), cur_second - second.begin()}; } -// Each index query returns at most one result per label, so duplicates in a one-shot tier merge can -// only occur across the inputs. Batch iterators cannot use this optimization because they also need -// to remember labels returned by earlier calls. +// 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 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; - CrossTierIdTracker tracker(smaller); + // The mapped bool records whether this tracked ID has already been emitted. + vecsim_stl::unordered_map 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 ¤t) { - if (tracker.shouldEmit(current->id)) { + if (should_emit(current->id)) { results.push_back(*current); limit--; } @@ -210,6 +138,8 @@ merge_results_with_cross_tier_dedup(VecSimQueryResultContainer &results, } 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++; From 8bd95c7e441de8d6c335cfe8ae4e3823370ad149 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Tue, 25 Aug 2026 15:10:28 +0300 Subject: [PATCH 6/6] [MOD-17795] Skip dedup state for trivial merges --- src/VecSim/utils/query_result_utils.h | 9 +++++-- tests/unit/test_common.cpp | 36 +++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/src/VecSim/utils/query_result_utils.h b/src/VecSim/utils/query_result_utils.h index 6c7e66259..6b896f1df 100644 --- a/src/VecSim/utils/query_result_utils.h +++ b/src/VecSim/utils/query_result_utils.h @@ -161,8 +161,13 @@ merge_results_with_cross_tier_dedup(VecSimQueryResultContainer &results, inline VecSimQueryReply *merge_result_lists(VecSimQueryReply *first, VecSimQueryReply *second, size_t limit) { auto mergedResults = new VecSimQueryReply(first->results.getAllocator()); - merge_results_with_cross_tier_dedup(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(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); diff --git a/tests/unit/test_common.cpp b/tests/unit/test_common.cpp index 348ca346c..724c0f74f 100644 --- a/tests/unit/test_common.cpp +++ b/tests/unit/test_common.cpp @@ -936,6 +936,42 @@ TEST(CommonAPITest, MergeResultListsWithCrossTierDedup) { } 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 one_result_first = { + {.id = 42, .score = 0.2}, + {.id = 10, .score = 0.8}, + }; + const std::vector 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 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> {