-
Notifications
You must be signed in to change notification settings - Fork 112
fix(hgraph): backport duplicate handling to v0.18 #2615
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.
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
base: 0.18
Are you sure you want to change the base?
Changes from 2 commits
9874492
0c7edee
1f46ea5
34bfc8a
31b32f8
7b796f2
8a4a409
27a9ff4
306bc89
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" | ||
|
|
@@ -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)) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The if (ef_search_json.IsNumberUnsigned()) {
CHECK_ARGUMENT(ef_search_json.GetUint64() <= ...);
}Including Suggested fix: remove the |
||
| 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()) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [note] The unsigned overflow check via |
||
| 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"); | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ | |
| #include <mutex> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [note] In Consider adding an explicit |
||
| #include <tuple> | ||
| #include <utility> | ||
| #include <vector> | ||
|
|
||
| #include "storage/stream_reader.h" | ||
| #include "storage/stream_writer.h" | ||
|
|
@@ -313,13 +314,54 @@ 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) { | ||
| if (duplicate_count_ > label_table_.size()) { | ||
| throw VsagException(ErrorType::INVALID_BINARY, | ||
| fmt::format("duplicate group count {} exceeds label count {}", | ||
| duplicate_count_, | ||
| label_table_.size())); | ||
| } | ||
|
|
||
| std::vector<std::pair<InnerIdType, Vector<InnerIdType>>> duplicate_groups; | ||
| 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); | ||
| duplicate_records_[id] = allocator_->New<DuplicateRecord>(allocator_); | ||
| if (id >= label_table_.size()) { | ||
| throw VsagException(ErrorType::INVALID_BINARY, | ||
| fmt::format("duplicate head id {} exceeds label count {}", | ||
| id, | ||
| label_table_.size())); | ||
| } | ||
| if (not assigned_ids.insert(id).second) { | ||
| throw VsagException( | ||
| ErrorType::INVALID_BINARY, | ||
| fmt::format("id {} belongs to multiple duplicate groups", id)); | ||
| } | ||
|
|
||
| Vector<InnerIdType> id_list(allocator_); | ||
| StreamReader::ReadVector(reader, id_list); | ||
| for (const auto duplicate_id : id_list) { | ||
| if (duplicate_id >= label_table_.size()) { | ||
| throw VsagException( | ||
| ErrorType::INVALID_BINARY, | ||
| fmt::format("duplicate member id {} exceeds label count {}", | ||
| duplicate_id, | ||
| label_table_.size())); | ||
| } | ||
| 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)); | ||
| } | ||
|
|
||
| duplicate_records_.resize(label_table_.size(), nullptr); | ||
| for (auto& [id, id_list] : duplicate_groups) { | ||
| duplicate_records_[id] = allocator_->New<DuplicateRecord>(allocator_); | ||
| for (const auto& duplicate_id : id_list) { | ||
| duplicate_records_[id]->duplicate_ids.insert(duplicate_id); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[note] Test - please ignore