Skip to content

fix: HGraph dirty-vector search dereferences an empty result heap #2802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vsag-bot wants to merge 1 commit into from
Closed

fix: HGraph dirty-vector search dereferences an empty result heap #2802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 34 additions & 19 deletions src/algorithm/hgraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -968,7 +968,9 @@ HGraph::KnnSearch(const DatasetPtr& query,
search_param,
(VisitedListPtr) nullptr,
&ctx);
search_param.ep = result->Top().second;
if (not result->Empty()) {
search_param.ep = result->Top().second;
Comment thread
wxyucs marked this conversation as resolved.
}
}
}

Expand Down Expand Up @@ -1180,7 +1182,9 @@ HGraph::RangeSearch(const DatasetPtr& query,
search_param,
(VisitedListPtr) nullptr,
&ctx);
search_param.ep = result->Top().second;
if (not result->Empty()) {
search_param.ep = result->Top().second;
}
}

CHECK_ARGUMENT((1 <= params.ef_search) and (params.ef_search <= 1000), // NOLINT
Expand Down Expand Up @@ -1676,6 +1680,7 @@ HGraph::add_one_point(const void* data, int level, InnerIdType inner_id) {
std::unique_lock add_lock(add_mutex_);
if (level >= static_cast<int>(this->route_graphs_.size()) || bottom_graph_->TotalCount() == 0) {
std::scoped_lock<std::shared_mutex> wlock(this->global_mutex_);
const auto previous_route_graph_count = this->route_graphs_.size();
// level maybe a negative number(-1)
for (auto j = static_cast<int>(this->route_graphs_.size()); j <= level; ++j) {
this->route_graphs_.emplace_back(this->generate_one_route_graph());
Expand All @@ -1684,7 +1689,7 @@ HGraph::add_one_point(const void* data, int level, InnerIdType inner_id) {
if (insert_success) {
entry_point_id_ = inner_id;
} else {
this->route_graphs_.pop_back();
this->route_graphs_.resize(previous_route_graph_count);
}
add_lock.unlock();
} else {
Expand Down Expand Up @@ -1712,7 +1717,9 @@ HGraph::graph_add_one(const void* data, int level, InnerIdType inner_id) {
for (auto j = this->route_graphs_.size() - 1; j > level; --j) {
result = search_one_graph(
data, route_graphs_[j], flatten_codes, param, (VisitedListPtr) nullptr, nullptr);
param.ep = result->Top().second;
if (not result->Empty()) {
param.ep = result->Top().second;
}
}

param.ef = this->ef_construct_;
Expand All @@ -1736,13 +1743,16 @@ HGraph::graph_add_one(const void* data, int level, InnerIdType inner_id) {
label_table_->SetDuplicateId(static_cast<InnerIdType>(param.duplicate_id), inner_id);
return false;
}
mutually_connect_new_element(inner_id,
result,
this->bottom_graph_,
flatten_codes,
neighbors_mutex_,
allocator_,
alpha_);
const auto next_entry_point = mutually_connect_new_element(inner_id,
result,
this->bottom_graph_,
flatten_codes,
neighbors_mutex_,
allocator_,
alpha_);
if (next_entry_point == INVALID_ENTRY_POINT) {
return false;
}
} else {
bottom_graph_->InsertNeighborsById(inner_id, Vector<InnerIdType>(allocator_));
}
Expand All @@ -1756,13 +1766,16 @@ HGraph::graph_add_one(const void* data, int level, InnerIdType inner_id) {
// to specify which overloaded function to call
(VisitedListPtr) nullptr,
nullptr);
mutually_connect_new_element(inner_id,
result,
route_graphs_[j],
flatten_codes,
neighbors_mutex_,
allocator_,
alpha_);
const auto next_entry_point = mutually_connect_new_element(inner_id,
result,
route_graphs_[j],
flatten_codes,
neighbors_mutex_,
allocator_,
alpha_);
if (next_entry_point == INVALID_ENTRY_POINT) {
return false;
}
} else {
route_graphs_[j]->InsertNeighborsById(inner_id, Vector<InnerIdType>(allocator_));
}
Expand Down Expand Up @@ -2257,7 +2270,9 @@ HGraph::SearchWithRequest(const SearchRequest& request) const {
for (auto i = static_cast<int64_t>(this->route_graphs_.size() - 1); i >= 0; --i) {
auto result = this->search_one_graph(
raw_query, this->route_graphs_[i], this->basic_flatten_codes_, search_param, vt, &ctx);
search_param.ep = result->Top().second;
if (not result->Empty()) {
search_param.ep = result->Top().second;
}
}

FilterPtr ft = nullptr;
Expand Down
42 changes: 42 additions & 0 deletions src/algorithm/hgraph_add_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <catch2/catch_test_macros.hpp>
#include <functional>
#include <future>
#include <limits>
#include <new>
#include <vector>

Expand Down Expand Up @@ -139,3 +140,44 @@ TEST_CASE("HGraph Tune accepts an incomplete source after Add failure",
REQUIRE(tune_result.has_value());
CHECK(tune_result.value());
}

TEST_CASE("HGraph search handles an empty route result", "[ut][hgraph][search][nonfinite]") {
constexpr int64_t dim = 4;
constexpr int64_t base_count = 64;

auto common_param = MakeCommonParam(dim);
auto hgraph_json = vsag::JsonType::Parse(R"({
"base_quantization_type": "fp32",
"max_degree": 8,
"ef_construction": 32,
"build_thread_count": 1
})");
auto index = std::make_shared<vsag::IndexImpl<vsag::HGraph>>(hgraph_json, common_param);

std::vector<float> base_vectors(base_count * dim);
std::vector<int64_t> base_ids(base_count);
for (int64_t i = 0; i < base_count; ++i) {
base_ids[i] = i;
for (int64_t j = 0; j < dim; ++j) {
base_vectors[i * dim + j] = static_cast<float>(i + j);
}
}
auto base = MakeFloatDataset(base_vectors, base_ids, dim, base_count);
REQUIRE(index->Build(base).has_value());

std::vector<float> query_vectors(dim, std::numeric_limits<float>::quiet_NaN());
std::vector<int64_t> query_ids = {base_count};
auto query = MakeFloatDataset(query_vectors, query_ids, dim, 1);
auto add_result = index->Add(query);
REQUIRE(add_result.has_value());
REQUIRE(add_result.value().empty());

vsag::SearchRequest request;
request.query_ = query;
request.topk_ = 10;
request.params_str_ = R"({"hgraph":{"ef_search":32}})";

auto result = index->SearchWithRequest(request);
REQUIRE(result.has_value());
CHECK(result.value()->GetDim() == 0);
}
4 changes: 2 additions & 2 deletions src/algorithm/pyramid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -897,9 +897,9 @@ Pyramid::add_one_point(const std::shared_ptr<IndexNode>& node,
inner_id);
return;
}
mutually_connect_new_element(
const auto next_entry_point = mutually_connect_new_element(
inner_id, results, node->graph_, codes, points_mutex_, allocator_, alpha_);
if (update_entry_point) {
if (update_entry_point and next_entry_point != INVALID_ENTRY_POINT) {
node->entry_point_ = inner_id;
}
}
Expand Down
8 changes: 6 additions & 2 deletions src/impl/pruning_strategy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

#include "pruning_strategy.h"

#include "common.h"
#include "datacell/flatten_datacell.h"
#include "datacell/graph_interface.h"
#include "impl/heap/standard_heap.h"
Expand Down Expand Up @@ -87,9 +88,12 @@ mutually_connect_new_element(InnerIdType cur_c,
top_candidates->Pop();
}

InnerIdType next_closest_entry_point = selected_neighbors.back();

graph->InsertNeighborsById(cur_c, selected_neighbors);
if (selected_neighbors.empty()) {
return INVALID_ENTRY_POINT;
}

InnerIdType next_closest_entry_point = selected_neighbors.back();

for (auto selected_neighbor : selected_neighbors) {
if (selected_neighbor == cur_c) {
Expand Down
15 changes: 15 additions & 0 deletions src/impl/pruning_strategy_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <memory>
#include <vector>

#include "common.h"
#include "datacell/flatten_datacell.h"
#include "datacell/flatten_datacell_parameter.h"
#include "datacell/graph_datacell_parameter.h"
Expand Down Expand Up @@ -194,6 +195,20 @@ TEST_CASE("Pruning Strategy Select Edges With Heuristic", "[ut][pruning_strategy
graph->GetNeighbors(0, neighbors_0);
REQUIRE(neighbors_0.size() == 1);
}

SECTION("Mutual connection supports no candidates") {
auto graph_param = std::make_shared<GraphDataCellParameter>();
graph_param->io_parameter_ = std::make_shared<MemoryIOParameter>();
graph_param->max_degree_ = 4;
auto graph = GraphInterface::MakeInstance(graph_param, common_param);

auto candidates = std::make_shared<StandardHeap<true, false>>(allocator.get(), -1);
auto mutexes = std::make_shared<EmptyMutex>();
auto entry_point =
mutually_connect_new_element(0, candidates, graph, flatten, mutexes, allocator.get());

REQUIRE(entry_point == INVALID_ENTRY_POINT);
}
}

} // namespace vsag
Loading