From a4437e7daa0a63192985ab28b95f6f99b568c31d Mon Sep 17 00:00:00 2001 From: foxspy Date: Sat, 18 Jul 2026 10:44:30 +0800 Subject: [PATCH] enhance: adapt segcore to knowhere iterator error-code interface (#51017) --- internal/core/src/common/QueryResult.h | 36 +++++++++++++++---- internal/core/src/index/VectorDiskIndex.cpp | 12 ++++--- internal/core/src/index/VectorMemIndex.cpp | 12 ++++--- .../core/src/query/CachedSearchIterator.cpp | 29 ++++++++++++--- .../src/query/SearchBruteForceSparseTest.cpp | 4 +-- .../core/thirdparty/knowhere/CMakeLists.txt | 5 ++- internal/core/unittest/test_indexing.cpp | 32 +++++++++-------- 7 files changed, 91 insertions(+), 39 deletions(-) diff --git a/internal/core/src/common/QueryResult.h b/internal/core/src/common/QueryResult.h index 7cd35373a4..3104990d66 100644 --- a/internal/core/src/common/QueryResult.h +++ b/internal/core/src/common/QueryResult.h @@ -28,6 +28,7 @@ #include #include "common/BitsetView.h" +#include "common/EasyAssert.h" #include "common/FieldMeta.h" #include "common/ArrayOffsets.h" #include "common/OffsetMapping.h" @@ -178,10 +179,18 @@ class ChunkMergeIterator : public VectorIterator { if (!heap_.empty()) { auto top = heap_.top(); heap_.pop(); - if (iterators_[top->GetIteratorIdx()]->HasNext()) { - auto origin_pair = iterators_[top->GetIteratorIdx()]->Next(); + auto& iter = iterators_[top->GetIteratorIdx()]; + auto has_next = iter->HasNext(); + AssertInfo(has_next.has_value(), + "knowhere iterator HasNext failed: {}", + has_next.what()); + if (has_next.value()) { + auto origin_pair = iter->Next(); + AssertInfo(origin_pair.has_value(), + "knowhere iterator Next failed: {}", + origin_pair.what()); auto off_dis_pair = std::make_shared( - origin_pair, top->GetIteratorIdx()); + origin_pair.value(), top->GetIteratorIdx()); heap_.push(off_dis_pair); } auto result = top->GetOffDis(); @@ -205,12 +214,25 @@ class ChunkMergeIterator : public VectorIterator { void seal() { sealed = true; - int idx = 0; - for (auto& iter : iterators_) { - if (iter->HasNext()) { + // idx must track each chunk's position in iterators_, because Next() + // refills from iterators_[OffsetDisPair::GetIteratorIdx()]. Incrementing + // only for non-empty iterators would misalign the index once a leading + // iterator is empty (e.g. emptied by a bitset filter), stamping later + // chunks with the wrong iterator and silently dropping their remaining + // results from the merged top-k. + for (int idx = 0; idx < static_cast(iterators_.size()); ++idx) { + auto& iter = iterators_[idx]; + auto has_next = iter->HasNext(); + AssertInfo(has_next.has_value(), + "knowhere iterator HasNext failed: {}", + has_next.what()); + if (has_next.value()) { auto origin_pair = iter->Next(); + AssertInfo(origin_pair.has_value(), + "knowhere iterator Next failed: {}", + origin_pair.what()); auto off_dis_pair = - std::make_shared(origin_pair, idx++); + std::make_shared(origin_pair.value(), idx); heap_.push(off_dis_pair); } } diff --git a/internal/core/src/index/VectorDiskIndex.cpp b/internal/core/src/index/VectorDiskIndex.cpp index b0a9771d07..d2b7b0e8fb 100644 --- a/internal/core/src/index/VectorDiskIndex.cpp +++ b/internal/core/src/index/VectorDiskIndex.cpp @@ -83,13 +83,15 @@ struct EmptyEmbListState { class DiskEmptyVectorIterator : public knowhere::IndexNode::iterator { public: - std::pair - Next() override { - throw std::runtime_error("empty vector iterator has no next result"); + knowhere::expected> + Next() noexcept override { + return knowhere::expected>::Err( + knowhere::Status::knowhere_inner_error, + "empty vector iterator has no next result"); } - bool - HasNext() override { + knowhere::expected + HasNext() noexcept override { return false; } }; diff --git a/internal/core/src/index/VectorMemIndex.cpp b/internal/core/src/index/VectorMemIndex.cpp index 6a2979104d..46c57440ac 100644 --- a/internal/core/src/index/VectorMemIndex.cpp +++ b/internal/core/src/index/VectorMemIndex.cpp @@ -91,13 +91,15 @@ struct EmptyEmbListState { class EmptyVectorIterator : public knowhere::IndexNode::iterator { public: - std::pair - Next() override { - throw std::runtime_error("empty vector iterator has no next result"); + knowhere::expected> + Next() noexcept override { + return knowhere::expected>::Err( + knowhere::Status::knowhere_inner_error, + "empty vector iterator has no next result"); } - bool - HasNext() override { + knowhere::expected + HasNext() noexcept override { return false; } }; diff --git a/internal/core/src/query/CachedSearchIterator.cpp b/internal/core/src/query/CachedSearchIterator.cpp index 9cfcff7c58..4a550198c9 100644 --- a/internal/core/src/query/CachedSearchIterator.cpp +++ b/internal/core/src/query/CachedSearchIterator.cpp @@ -278,8 +278,18 @@ CachedSearchIterator::GetNextValidResult( const std::optional& radius, const std::optional& range_filter) { auto& iterator = iterators_[iterator_idx]; - while (iterator->HasNext()) { - auto result = ConvertIteratorResult(iterator->Next()); + while (true) { + auto has_next = iterator->HasNext(); + AssertInfo(has_next.has_value(), + "knowhere iterator HasNext failed: {}", + has_next.what()); + if (!has_next.value()) { + break; + } + auto next = iterator->Next(); + AssertInfo( + next.has_value(), "knowhere iterator Next failed: {}", next.what()); + auto result = ConvertIteratorResult(next.value()); if (IsValid(result, last_bound, radius, range_filter)) { return result; } @@ -342,8 +352,19 @@ CachedSearchIterator::GetBatchedNextResults(size_t query_idx, if (num_chunks_ == 1) { auto& iterator = iterators_[query_idx]; - while (iterator->HasNext() && rst.size() < batch_size_) { - auto result = ConvertIteratorResult(iterator->Next()); + while (rst.size() < batch_size_) { + auto has_next = iterator->HasNext(); + AssertInfo(has_next.has_value(), + "knowhere iterator HasNext failed: {}", + has_next.what()); + if (!has_next.value()) { + break; + } + auto next = iterator->Next(); + AssertInfo(next.has_value(), + "knowhere iterator Next failed: {}", + next.what()); + auto result = ConvertIteratorResult(next.value()); if (IsValid(result, last_bound, radius, range_filter)) { rst.emplace_back(result); } diff --git a/internal/core/src/query/SearchBruteForceSparseTest.cpp b/internal/core/src/query/SearchBruteForceSparseTest.cpp index 21612e2000..b53c06765f 100644 --- a/internal/core/src/query/SearchBruteForceSparseTest.cpp +++ b/internal/core/src/query/SearchBruteForceSparseTest.cpp @@ -184,8 +184,8 @@ class TestSparseFloatSearchBruteForce : public ::testing::Test { auto q = *(query.get() + i); auto last_dis = std::numeric_limits::max(); // we should see strict decreasing distances for brute force iterator. - while (it->HasNext()) { - auto [offset, dis] = it->Next(); + while (it->HasNext().value()) { + auto [offset, dis] = it->Next().value(); ASSERT_LE(dis, last_dis); last_dis = dis; ASSERT_FLOAT_EQ(dis, base[offset].dot(q)); diff --git a/internal/core/thirdparty/knowhere/CMakeLists.txt b/internal/core/thirdparty/knowhere/CMakeLists.txt index 04a245f896..bc8fa3c509 100644 --- a/internal/core/thirdparty/knowhere/CMakeLists.txt +++ b/internal/core/thirdparty/knowhere/CMakeLists.txt @@ -14,7 +14,10 @@ # Update KNOWHERE_VERSION for the first occurrence milvus_add_pkg_config("knowhere") set_property(DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} PROPERTY INCLUDE_DIRECTORIES "") -set( KNOWHERE_VERSION 49507ef3 ) +# zilliztech/knowhere#1699 (iterator error-code interface) that this segcore +# adaptation depends on has landed on knowhere main and is included in the +# v3.0.6 release tag, so pin to the tagged release instead of a raw commit. +set( KNOWHERE_VERSION v3.0.6 ) set( GIT_REPOSITORY "https://github.com/zilliztech/knowhere.git") message(STATUS "Knowhere repo: ${GIT_REPOSITORY}") diff --git a/internal/core/unittest/test_indexing.cpp b/internal/core/unittest/test_indexing.cpp index 6bc02fde39..bd4570308e 100644 --- a/internal/core/unittest/test_indexing.cpp +++ b/internal/core/unittest/test_indexing.cpp @@ -535,9 +535,9 @@ TEST(Indexing, Iterator) { ASSERT_TRUE(kw_iterators.has_value()); ASSERT_EQ(kw_iterators.value().size(), 1); auto iterator = kw_iterators.value()[0]; - ASSERT_TRUE(iterator->HasNext()); - while (iterator->HasNext()) { - auto [off, dis] = iterator->Next(); + ASSERT_TRUE(iterator->HasNext().value()); + while (iterator->HasNext().value()) { + auto [off, dis] = iterator->Next().value(); ASSERT_TRUE(off >= 0); ASSERT_TRUE(dis >= 0); } @@ -940,8 +940,8 @@ TEST(Indexing, HnswEmbListBuildAllNullNullableFromBinlog) { iterator_dataset, iterator_conf, nullptr); ASSERT_TRUE(iterators.has_value()) << iterators.what(); ASSERT_EQ(iterators.value().size(), 2); - EXPECT_FALSE(iterators.value()[0]->HasNext()); - EXPECT_FALSE(iterators.value()[1]->HasNext()); + EXPECT_FALSE(iterators.value()[0]->HasNext().value()); + EXPECT_FALSE(iterators.value()[1]->HasNext().value()); std::vector emb_list_iterator_queries(3 * dim, 0.1F); auto emb_list_iterator_dataset = @@ -955,9 +955,10 @@ TEST(Indexing, HnswEmbListBuildAllNullNullableFromBinlog) { emb_list_iterator_dataset, iterator_conf, nullptr); ASSERT_TRUE(emb_list_iterators.has_value()) << emb_list_iterators.what(); ASSERT_EQ(emb_list_iterators.value().size(), 4); - EXPECT_TRUE(std::all_of(emb_list_iterators.value().begin(), - emb_list_iterators.value().end(), - [](const auto& iter) { return !iter->HasNext(); })); + EXPECT_TRUE( + std::all_of(emb_list_iterators.value().begin(), + emb_list_iterators.value().end(), + [](const auto& iter) { return !iter->HasNext().value(); })); milvus::SearchInfo iterator_search_info; iterator_search_info.topk_ = 3; @@ -1117,8 +1118,8 @@ TEST(Indexing, HnswEmbListBuildAllValidEmptyListsFromBinlog) { iterator_dataset, iterator_conf, nullptr); ASSERT_TRUE(iterators.has_value()) << iterators.what(); ASSERT_EQ(iterators.value().size(), 2); - EXPECT_FALSE(iterators.value()[0]->HasNext()); - EXPECT_FALSE(iterators.value()[1]->HasNext()); + EXPECT_FALSE(iterators.value()[0]->HasNext().value()); + EXPECT_FALSE(iterators.value()[1]->HasNext().value()); std::vector ids = {0, 3, 7}; auto ids_ds = GenIdsDataset(ids.size(), ids.data()); @@ -1143,9 +1144,10 @@ TEST(Indexing, HnswEmbListBuildAllValidEmptyListsFromBinlog) { emb_list_iterator_dataset, emb_list_iterator_conf, nullptr); ASSERT_TRUE(emb_list_iterators.has_value()) << emb_list_iterators.what(); ASSERT_EQ(emb_list_iterators.value().size(), 4); - EXPECT_TRUE(std::all_of(emb_list_iterators.value().begin(), - emb_list_iterators.value().end(), - [](const auto& iter) { return !iter->HasNext(); })); + EXPECT_TRUE( + std::all_of(emb_list_iterators.value().begin(), + emb_list_iterators.value().end(), + [](const auto& iter) { return !iter->HasNext().value(); })); std::vector query(dim, 0.1F); auto xq_dataset = knowhere::GenDataSet(1, dim, query.data()); @@ -1834,8 +1836,8 @@ TEST(Indexing, DiskAnnEmbListBuildAllNullNullableFromBinlog) { iterator_dataset, iterator_conf, nullptr); ASSERT_TRUE(iterators.has_value()) << iterators.what(); ASSERT_EQ(iterators.value().size(), 2); - EXPECT_FALSE(iterators.value()[0]->HasNext()); - EXPECT_FALSE(iterators.value()[1]->HasNext()); + EXPECT_FALSE(iterators.value()[0]->HasNext().value()); + EXPECT_FALSE(iterators.value()[1]->HasNext().value()); milvus::SearchInfo search_info; search_info.topk_ = 3;