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/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 5bb8f35c9..6b896f1df 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 @@ -89,17 +92,82 @@ std::pair 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 -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 +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 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 (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(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); @@ -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 -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. diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index d097b1ae4..db17c02c5 100644 --- a/src/VecSim/vec_sim_tiered_index.h +++ b/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 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; @@ -167,7 +161,6 @@ class VecSimTieredIndex : public VecSimIndexInterface { }; template -template VecSimQueryReply * VecSimTieredIndex::topKQueryImp(const void *queryBlob, size_t k, VecSimQueryParams *queryParams) const { @@ -214,21 +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 { - 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 @@ -236,19 +222,10 @@ 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 -template VecSimQueryReply * VecSimTieredIndex::rangeQueryImp(const void *queryBlob, double radius, VecSimQueryParams *queryParams, @@ -301,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; @@ -309,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 35a269311..724c0f74f 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" @@ -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(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) { + 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); + } + + // 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> { 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; 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) {