Skip to content
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
15 changes: 13 additions & 2 deletions docs/hgraph.md
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,21 @@ means that the index uses PQ quantization with 64 subspaces, enables reordering
- **Optional Values**: 1 to INT_MAX
- **Default Value**: Must be provided (no default value)

### max_duplicates_per_group
- **Parameter Type**: int
- **Parameter Description**: Maximum number of additional duplicate IDs returned for each
duplicate group during KNN graph search
- **Optional Values**: -1 (unlimited), 0 (do not expand duplicate IDs), or any positive integer
- **Default Value**: -1
- **Notes**: Only applies when `support_duplicate` is enabled; range search keeps its existing
expansion behavior

## Examples for Search Parameter String
```json
"hgraph": {
"ef_search": 200
"ef_search": 200,
"max_duplicates_per_group": 2
}
```
means that the search will use an ef_search value of 200 to control the search quality and performance trade-off.
means that the search uses an ef_search value of 200 and returns at most two additional IDs from
each duplicate group.
1 change: 1 addition & 0 deletions include/vsag/constants.h
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ extern const char* const HGRAPH_PRECISE_IO_TYPE;
extern const char* const HGRAPH_PRECISE_FILE_PATH;
extern const char* const HGRAPH_PARAMETER_EF_RUNTIME;
extern const char* const HGRAPH_PARAMETER_HOPS_LIMIT;
extern const char* const HGRAPH_PARAMETER_MAX_DUPLICATES_PER_GROUP;
extern const char* const HGRAPH_EXTRA_INFO_SIZE;
extern const char* const HGRAPH_SUPPORT_DUPLICATE;
extern const char* const HGRAPH_DUPLICATE_DISTANCE_THRESHOLD;
Expand Down
22 changes: 21 additions & 1 deletion src/algorithm/hgraph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -933,10 +933,20 @@ HGraph::KnnSearch(const DatasetPtr& query,
auto search_result = DistanceHeap::MakeInstanceBySize<true, false>(ctx.alloc, k);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note] Test - please ignore

const auto* query_data = get_data(query);
if (is_last_filter) {
if (const auto* pending_duplicates = iter_filter_ctx->GetPendingDuplicates();
pending_duplicates != nullptr) {
for (const auto& [pending_id, pending_dist] : *pending_duplicates) {
if (iter_filter_ctx->CheckPoint(pending_id)) {
search_result->Push(pending_dist, pending_id);
}
}
}
while (!iter_filter_ctx->Empty()) {
uint32_t cur_inner_id = iter_filter_ctx->GetTopID();
float cur_dist = iter_filter_ctx->GetTopDist();
search_result->Push(cur_dist, cur_inner_id);
if (not iter_filter_ctx->IsPendingDuplicate(cur_inner_id)) {
search_result->Push(cur_dist, cur_inner_id);
}
iter_filter_ctx->PopDiscard();
}
} else {
Expand Down Expand Up @@ -964,6 +974,7 @@ HGraph::KnnSearch(const DatasetPtr& query,
search_param.is_inner_id_allowed = ft;
search_param.topk = static_cast<int64_t>(search_param.ef);
search_param.consider_duplicate = this->label_table_->CompressDuplicateData();
search_param.max_duplicates_per_group = params.max_duplicates_per_group;
search_param.parallel_search_thread_count = params.parallel_search_thread_count;
search_param.min_distance = params.min_distance;

Expand Down Expand Up @@ -1410,6 +1421,9 @@ HGraph::Serialize(StreamWriter& writer) const {
if (this->use_attribute_filter_ and this->attr_filter_index_ != nullptr) {
this->attr_filter_index_->Serialize(writer);
}
if (this->label_table_->CompressDuplicateData()) {
this->label_table_->SerializeDuplicateRecords(writer);
}
return;
}

Expand Down Expand Up @@ -1474,6 +1488,11 @@ HGraph::Deserialize(StreamReader& reader) {
if (this->use_attribute_filter_ and this->attr_filter_index_ != nullptr) {
this->attr_filter_index_->Deserialize(reader);
}
if (this->label_table_->CompressDuplicateData()) {
const auto logical_element_count =
Comment thread
jac0626 marked this conversation as resolved.
static_cast<uint64_t>(this->label_table_->GetTotalCount());
this->label_table_->DeserializeDuplicateRecords(reader, logical_element_count);
Comment thread
jac0626 marked this conversation as resolved.
}
} else { // create like `else if ( ver in [v0.15, v0.17] )` here if need in the future
logger::debug("parse with new version format");

Expand Down Expand Up @@ -2222,6 +2241,7 @@ HGraph::SearchWithRequest(const SearchRequest& request) const {
search_param.topk, static_cast<int64_t>(static_cast<float>(k) * params.topk_factor));
}
search_param.consider_duplicate = true;
search_param.max_duplicates_per_group = params.max_duplicates_per_group;
if (params.enable_time_record) {
search_param.time_cost = std::make_shared<Timer>();
search_param.time_cost->SetThreshold(params.timeout_ms);
Expand Down
18 changes: 18 additions & 0 deletions src/algorithm/hgraph_parameter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

#include "hgraph_parameter.h"

#include <nlohmann/json.hpp>

#include "datacell/extra_info_datacell_parameter.h"
#include "datacell/flatten_datacell_parameter.h"
#include "datacell/graph_datacell_parameter.h"
Expand Down Expand Up @@ -223,6 +225,22 @@ HGraphSearchParameters::FromJson(const std::string& json_string) {
if (params[INDEX_TYPE_HGRAPH].Contains("min_distance")) {
obj.min_distance = params[INDEX_TYPE_HGRAPH]["min_distance"].GetFloat();
}
if (params[INDEX_TYPE_HGRAPH].Contains(HGRAPH_PARAMETER_MAX_DUPLICATES_PER_GROUP)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JsonWrapper class already provides IsNumberUnsigned() and GetUint64() methods (see src/json_wrapper.h). The existing ef_search parsing in this same function (lines 335-336) already uses the correct pattern:

if (ef_search_json.IsNumberUnsigned()) {
    CHECK_ARGUMENT(ef_search_json.GetUint64() <= ...);
}

Including <nlohmann/json.hpp> and using GetInnerJson() to call is_number_unsigned() / get<uint64_t>() directly on the underlying nlohmann::json pointer is inconsistent with the codebase pattern. The JsonWrapper abstraction exists precisely to avoid direct nlohmann::json usage in parameter parsing code.

Suggested fix: remove the #include <nlohmann/json.hpp> and use max_duplicates.IsNumberUnsigned() / max_duplicates.GetUint64() instead.

const auto& max_duplicates =
params[INDEX_TYPE_HGRAPH][HGRAPH_PARAMETER_MAX_DUPLICATES_PER_GROUP];
CHECK_ARGUMENT(max_duplicates.IsNumberInteger(),
"max_duplicates_per_group must be an integer");
const auto* max_duplicates_json = max_duplicates.GetInnerJson();
if (max_duplicates_json->is_number_unsigned()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note] The unsigned overflow check via GetInnerJson()->is_number_unsigned() is a pragmatic approach to detect values that exceed int64_t range. However, this relies on nlohmann's internal number representation which could change between library versions. A more robust approach would be to parse the raw JSON string value and validate it with std::from_chars or similar. This is a minor portability concern — the current approach works correctly with the nlohmann versions used by this project.

CHECK_ARGUMENT(max_duplicates_json->get<uint64_t>() <=
static_cast<uint64_t>(std::numeric_limits<int64_t>::max()),
"max_duplicates_per_group exceeds int64_t range");
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note] Direct use of nlohmann::json internals via GetInnerJson() bypasses the project's JsonType wrapper abstraction. The #include <nlohmann/json.hpp> is added solely for is_number_unsigned() and get<uint64_t>(). Consider adding IsUnsignedInteger() / GetUint64() to JsonType to keep the JSON library dependency encapsulated, or use GetInt() with a separate range check for negative values.

obj.max_duplicates_per_group = max_duplicates.GetInt();
CHECK_ARGUMENT(obj.max_duplicates_per_group >= -1,
fmt::format("max_duplicates_per_group({}) must be >= -1",
obj.max_duplicates_per_group));
}

return obj;
}
Expand Down
3 changes: 3 additions & 0 deletions src/algorithm/hgraph_parameter.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@ class HGraphSearchParameters : public IndexSearchParameter {
bool use_reorder{false};
bool use_extra_info_filter{false};
float min_distance{std::numeric_limits<float>::lowest()};
// Maximum additional duplicate IDs returned per group during KNN graph search.
// -1 means unlimited; 0 disables duplicate expansion.
int64_t max_duplicates_per_group{-1};

private:
HGraphSearchParameters() = default;
Expand Down
18 changes: 18 additions & 0 deletions src/algorithm/hgraph_parameter_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,24 @@ TEST_CASE("HGraph maps support_duplicate to graph parameter", "[ut][HGraphParame
REQUIRE(typed_param->duplicate_distance_threshold == 0.25F);
}

TEST_CASE("HGraph Search Parameters validate max_duplicates_per_group",
"[ut][HGraphSearchParameters][duplicate]") {
REQUIRE(vsag::HGraphSearchParameters::FromJson(R"({"hgraph": {"ef_search": 32}})")
.max_duplicates_per_group == -1);
REQUIRE(vsag::HGraphSearchParameters::FromJson(
R"({"hgraph": {"ef_search": 32, "max_duplicates_per_group": 0}})")
.max_duplicates_per_group == 0);
REQUIRE(vsag::HGraphSearchParameters::FromJson(
R"({"hgraph": {"ef_search": 32, "max_duplicates_per_group": 2}})")
.max_duplicates_per_group == 2);
REQUIRE_THROWS(vsag::HGraphSearchParameters::FromJson(
R"({"hgraph": {"ef_search": 32, "max_duplicates_per_group": -2}})"));
REQUIRE_THROWS(vsag::HGraphSearchParameters::FromJson(
R"({"hgraph": {"ef_search": 32, "max_duplicates_per_group": 1.5}})"));
REQUIRE_THROWS(vsag::HGraphSearchParameters::FromJson(
R"({"hgraph": {"ef_search": 32, "max_duplicates_per_group": 9223372036854775808}})"));
}

TEST_CASE("HGraph maps label_remap_type to inner index parameter", "[ut][HGraphParameter]") {
auto param = vsag::JsonType::Parse(R"({
"base_quantization_type": "fp32",
Expand Down
1 change: 1 addition & 0 deletions src/constants.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ const char* const HGRAPH_PRECISE_IO_TYPE = "precise_io_type";
const char* const HGRAPH_PRECISE_FILE_PATH = "precise_file_path";
const char* const HGRAPH_PARAMETER_EF_RUNTIME = "ef_search";
const char* const HGRAPH_PARAMETER_HOPS_LIMIT = "hops_limit";
const char* const HGRAPH_PARAMETER_MAX_DUPLICATES_PER_GROUP = "max_duplicates_per_group";
const char* const HGRAPH_EXTRA_INFO_SIZE = "extra_info_size";
const char* const HGRAPH_SUPPORT_DUPLICATE = "support_duplicate";
const char* const HGRAPH_DUPLICATE_DISTANCE_THRESHOLD = "duplicate_distance_threshold";
Expand Down
3 changes: 3 additions & 0 deletions src/impl/inner_search_param.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ class InnerSearchParam {

// use in search process with duplicate ids
bool consider_duplicate{false};
int64_t max_duplicates_per_group{-1};

// skip results with dist <= min_distance (for search iterator)
float min_distance{std::numeric_limits<float>::lowest()};
Expand All @@ -87,6 +88,8 @@ class InnerSearchParam {
factor = other.factor;
first_order_scan_ratio = other.first_order_scan_ratio;
parallel_search_thread_count = other.parallel_search_thread_count;
consider_duplicate = other.consider_duplicate;
max_duplicates_per_group = other.max_duplicates_per_group;
}
return *this;
}
Expand Down
134 changes: 111 additions & 23 deletions src/impl/label_table.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include <mutex>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note] In DeserializeDuplicateRecords, the cleanup loop on exception iterates over all entries in restored_records and calls allocator_->Delete(record) on each, including the nullptr entries from the initial Vector construction. While most allocator implementations handle Delete(nullptr) as a no-op, the DefaultAllocator (and any custom allocator) should be verified to safely accept null pointers in Delete. If not, this could cause a double-fault during the exception handler.

Consider adding an explicit if (record != nullptr) guard before calling Delete.

#include <tuple>
#include <utility>
#include <vector>

#include "storage/stream_reader.h"
#include "storage/stream_writer.h"
Expand Down Expand Up @@ -284,23 +285,28 @@ class LabelTable {
Serialize(StreamWriter& writer) const {
StreamWriter::WriteVector(writer, label_table_);
if (compress_duplicate_data_) {
StreamWriter::WriteObj(writer, duplicate_count_);
for (InnerIdType i = 0; i < label_table_.size(); ++i) {
if (duplicate_records_[i] != nullptr) {
StreamWriter::WriteObj(writer, i);
Vector<InnerIdType> id_list(allocator_);
for (const auto& duplicate_id : duplicate_records_[i]->duplicate_ids) {
id_list.push_back(duplicate_id);
}
StreamWriter::WriteVector(writer, id_list);
}
}
this->SerializeDuplicateRecords(writer);
}
if (support_tombstone_) {
StreamWriter::WriteObj(writer, deleted_ids_);
}
}

void
SerializeDuplicateRecords(StreamWriter& writer) const {
StreamWriter::WriteObj(writer, duplicate_count_);
for (InnerIdType i = 0; i < label_table_.size(); ++i) {
if (duplicate_records_[i] != nullptr) {
StreamWriter::WriteObj(writer, i);
Vector<InnerIdType> id_list(allocator_);
for (const auto& duplicate_id : duplicate_records_[i]->duplicate_ids) {
id_list.push_back(duplicate_id);
}
StreamWriter::WriteVector(writer, id_list);
}
}
}

void
Deserialize(lvalue_or_rvalue<StreamReader> reader) {
StreamReader::ReadVector(reader, label_table_);
Expand All @@ -312,25 +318,107 @@ class LabelTable {
}
}
if (compress_duplicate_data_) {
StreamReader::ReadObj(reader, duplicate_count_);
duplicate_records_.resize(label_table_.size(), nullptr);
for (InnerIdType i = 0; i < duplicate_count_; ++i) {
InnerIdType id;
StreamReader::ReadObj<InnerIdType>(reader, id);
duplicate_records_[id] = allocator_->New<DuplicateRecord>(allocator_);
Vector<InnerIdType> id_list(allocator_);
StreamReader::ReadVector(reader, id_list);
for (const auto& duplicate_id : id_list) {
duplicate_records_[id]->duplicate_ids.insert(duplicate_id);
}
}
this->DeserializeDuplicateRecords(reader, label_table_.size());
Comment thread
jac0626 marked this conversation as resolved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] The native-format Deserialize() path in label_table.h passes label_table_.size() (capacity) to DeserializeDuplicateRecords, while the v0.14 path in hgraph.cpp correctly passes the logical element count via GetTotalCount().

HGraph resizes label_table_ to max_capacity_ before serialization, so label_table_.size() includes unused slots. Passing capacity as logical_element_count weakens the validation in DeserializeDuplicateRecords: a blob for 3 elements with capacity 1024 would allow member ID 3 to pass the id >= logical_element_count check, and search could then return that slot's default label or score an uninitialized code slot.

The v0.14 path already does this correctly:

const auto logical_element_count = static_cast<uint64_t>(this->label_table_->GetTotalCount());
this->label_table_->DeserializeDuplicateRecords(reader, logical_element_count);

Consider using GetTotalCount() in the native Deserialize() path as well, or alternatively, pass the logical count from the footer (which already stores total_count).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Native path passes max_capacity_ instead of logical element count

In the native (v0.15+) deserialization path, LabelTable::Deserialize calls DeserializeDuplicateRecords(reader, label_table_.size()). After ReadVector, label_table_.size() equals max_capacity_ (since HGraph serializes at max capacity). This means validation checks use the capacity rather than the actual element count.

The v0.14 path correctly uses GetTotalCount() (the remap size = actual logical element count). The native path should similarly use the actual serialized element count (e.g., total_count_ after it is set on line 326) rather than label_table_.size().

This is a defense-in-depth concern.

@LHT129 LHT129 Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note] The native-format deserialization path in Deserialize() (line 321) passes label_table_.size() as the logical_element_count to DeserializeDuplicateRecords. In the new format, label_table_ is serialized at max_capacity_ which may be larger than the actual logical element count (GetTotalCount()). The v0.14 path (line 1535-1537 in hgraph.cpp) correctly uses GetTotalCount(), but the native path still uses the potentially larger label_table_.size().

This means duplicate validation in the native path accepts IDs up to max_capacity_ - 1 instead of total_count_ - 1. While properly constructed indexes should not contain out-of-range duplicate IDs, using GetTotalCount() would provide a tighter validation bound consistent with the v0.14 path.

Consider either:

  • Passing GetTotalCount() (or total_count_) instead of label_table_.size() at line 321, or
  • Adding a comment explaining why label_table_.size() is the correct bound for the native format.

}
if (support_tombstone_) {
StreamReader::ReadObj(reader, deleted_ids_);
}
this->total_count_.store(label_table_.size());
}

void
DeserializeDuplicateRecords(lvalue_or_rvalue<StreamReader> reader,
uint64_t logical_element_count) {
if (logical_element_count > label_table_.size()) {
throw VsagException(ErrorType::INVALID_BINARY,
fmt::format("logical element count {} exceeds label capacity {}",
logical_element_count,
label_table_.size()));
}

uint64_t duplicate_count = 0;
StreamReader::ReadObj(reader, duplicate_count);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note] The validation duplicate_count > logical_element_count / 2 (label_table.h line ~340) is correct — each duplicate group must have at least one member distinct from the head, so the maximum number of groups is logical_element_count / 2. However, the error message says "duplicate group count {} exceeds logical element limit {}" which could be clearer. Consider rewording to something like "duplicate group count {} exceeds maximum possible groups {} (logical_element_count / 2)" to make the rationale self-documenting.

if (duplicate_count > logical_element_count / 2) {
throw VsagException(
ErrorType::INVALID_BINARY,
fmt::format("duplicate group count {} exceeds logical element limit {}",
duplicate_count,
logical_element_count / 2));
}

std::vector<std::pair<InnerIdType, Vector<InnerIdType>>> duplicate_groups;
Comment thread
jac0626 marked this conversation as resolved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] The DeserializeDuplicateRecords method uses std::vector<std::pair<InnerIdType, Vector<InnerIdType>>> for the temporary duplicate_groups buffer (line 349 of label_table.h). The outer std::vector uses the default std::allocator, while the rest of the codebase consistently uses the custom Allocator (e.g. Vector, UnorderedSet).

While this is a local temporary that is immediately consumed and moved into allocator-managed storage, using std::vector with the default allocator bypasses the custom allocation tracking. Consider using Vector<std::pair<InnerIdType, Vector<InnerIdType>>> (which would use allocator_) for consistency, or add a brief comment explaining why the standard allocator is acceptable here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] The DeserializeDuplicateRecords method allocates a std::vector<std::pair<InnerIdType, Vector<InnerIdType>>> temporary buffer (line 349) using the default allocator. In this codebase, container types like Vector and UnorderedSet consistently use the custom Allocator for memory tracking. Using std::vector with the default allocator here breaks this pattern and could cause memory tracking gaps. Consider using Vector<std::pair<InnerIdType, Vector<InnerIdType>>> with the allocator, or alternatively process records one-at-a-time (since v0.14 format is already sequential) to avoid the temporary buffer entirely.

duplicate_groups.reserve(duplicate_count);
UnorderedSet<InnerIdType> assigned_ids(allocator_);
for (uint64_t i = 0; i < duplicate_count; ++i) {
InnerIdType id;
StreamReader::ReadObj<InnerIdType>(reader, id);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note] DeserializeDuplicateRecords uses std::vector (with the default allocator) for the temporary duplicate_groups staging container, while the rest of LabelTable consistently uses the custom allocator_-backed Vector type. This is fine for a stack-local temporary that is destroyed before the function returns, but it means the temporary allocations bypass the custom allocator. If the allocator is used for memory tracking/limiting, large duplicate group counts could momentarily escape those limits.

This is not a blocker — the final duplicate_records_ storage correctly uses allocator_.

if (id >= logical_element_count) {
throw VsagException(ErrorType::INVALID_BINARY,
fmt::format("duplicate head id {} exceeds logical element "
"count {}",
id,
logical_element_count));
}
if (not assigned_ids.insert(id).second) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note] DeserializeDuplicateRecords uses std::vector<std::pair<InnerIdType, Vector<InnerIdType>>> for the temporary duplicate_groups buffer, which allocates with the default (system) allocator. The rest of this method correctly uses allocator_ for assigned_ids, id_list, and restored_records.

This is acceptable since duplicate_groups is a short-lived stack variable that is destroyed before the function returns, but it is inconsistent with the codebase convention of routing all allocations through the custom allocator. If the custom allocator tracks allocation statistics, this temporary allocation will not be accounted for.

throw VsagException(ErrorType::INVALID_BINARY,
fmt::format("id {} belongs to multiple duplicate groups", id));
}

uint64_t member_count = 0;
StreamReader::ReadObj(reader, member_count);
const auto assigned_count = static_cast<uint64_t>(assigned_ids.size());
if (member_count == 0 or member_count > logical_element_count - assigned_count) {
throw VsagException(
ErrorType::INVALID_BINARY,
fmt::format("duplicate member count {} exceeds remaining logical element "
"count {}",
member_count,
logical_element_count - assigned_count));
}

Vector<InnerIdType> id_list(allocator_);
id_list.resize(member_count);
reader->Read(reinterpret_cast<char*>(id_list.data()),
member_count * sizeof(InnerIdType));
for (const auto duplicate_id : id_list) {
if (duplicate_id >= logical_element_count) {
throw VsagException(
ErrorType::INVALID_BINARY,
fmt::format("duplicate member id {} exceeds logical element count {}",
duplicate_id,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note] The catch block calls allocator_->Delete(record) for every entry in restored_records, including entries that are still nullptr (not yet allocated). While DefaultAllocator::Delete likely tolerates nullptr, custom allocator implementations may not. The existing code in Deserialize also calls Delete on duplicate_records_ entries which may be nullptr, so this is consistent with existing patterns. No action required unless the project plans to support allocators that reject null Delete.

logical_element_count));
}
if (not assigned_ids.insert(duplicate_id).second) {
throw VsagException(
ErrorType::INVALID_BINARY,
fmt::format("id {} belongs to multiple duplicate groups", duplicate_id));
}
}
duplicate_groups.emplace_back(id, std::move(id_list));
}

Vector<DuplicateRecord*> restored_records(label_table_.size(), nullptr, allocator_);
try {
for (auto& [id, id_list] : duplicate_groups) {
restored_records[id] = allocator_->New<DuplicateRecord>(allocator_);
for (const auto& duplicate_id : id_list) {
restored_records[id]->duplicate_ids.insert(duplicate_id);
}
}
} catch (...) {
for (auto* record : restored_records) {
allocator_->Delete(record);
}
throw;
}

for (auto* record : duplicate_records_) {
allocator_->Delete(record);
}
duplicate_records_ = std::move(restored_records);
duplicate_count_ = duplicate_count;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note] duplicate_count_ is set to duplicate_count which represents the number of duplicate groups (i.e., how many distinct head IDs have associated duplicate members), not the total number of duplicate member IDs across all groups. This is consistent with how duplicate_count_ is used in SerializeDuplicateRecords (writes group count) and SetDuplicateId (increments once per new group). However, the name duplicate_count_ could be misleading to readers who expect it to represent the total count of duplicate members. Consider renaming to duplicate_group_count_ for clarity, or adding a brief comment at the declaration site.

}

void

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note] In DeserializeDuplicateRecords, the cleanup loop on exception iterates over all entries in restored_records and calls allocator_->Delete(record) on each, including the nullptr entries from the initial Vector construction. While most allocator implementations handle Delete(nullptr) as a no-op, the DefaultAllocator (and any custom allocator) should be verified to safely accept null pointers in Delete. If not, this could cause a double-fault during the exception handler.

Consider adding an explicit if (record != nullptr) guard before calling Delete.

Resize(uint64_t new_size) {
if (new_size < total_count_) {
Expand Down
Loading
Loading