-
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 all 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 |
|---|---|---|
|
|
@@ -933,10 +933,20 @@ HGraph::KnnSearch(const DatasetPtr& query, | |
| auto search_result = DistanceHeap::MakeInstanceBySize<true, false>(ctx.alloc, k); | ||
| 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 { | ||
|
|
@@ -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; | ||
|
|
||
|
|
@@ -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; | ||
| } | ||
|
|
||
|
|
@@ -1442,10 +1456,53 @@ HGraph::Serialize(StreamWriter& writer) const { | |
| footer->Write(writer); | ||
| } | ||
|
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 |
||
|
|
||
| void | ||
| HGraph::Deserialize(std::istream& in_stream) { | ||
|
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 This is acceptable because:
However, if someone accidentally passes a native-format blob through |
||
| if (not this->use_old_serial_format_) { | ||
| InnerIndexInterface::Deserialize(in_stream); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| uint64_t cursor = 0; | ||
|
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. [suggestion] The Since the
The current partial catch gives a false impression of comprehensive error handling. |
||
| auto read_func = [&](uint64_t offset, uint64_t size, void* data) { | ||
| if (offset != cursor) { | ||
| throw VsagException(ErrorType::UNSUPPORTED_INDEX_OPERATION, | ||
| "v0.14 sequential stream does not support seek"); | ||
| } | ||
| in_stream.read(static_cast<char*>(data), static_cast<int64_t>(size)); | ||
| if (in_stream.gcount() != static_cast<int64_t>(size)) { | ||
| throw VsagException( | ||
| ErrorType::READ_ERROR, | ||
| fmt::format("Attempted to read: {} bytes. Remaining content size: {} bytes.", | ||
| size, | ||
| in_stream.gcount())); | ||
| } | ||
| cursor += size; | ||
| }; | ||
| ReadFuncStreamReader reader(read_func, 0, 0); | ||
|
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 Consider either passing the actual stream size if available, or adding a comment noting that
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] |
||
| this->deserialize(reader, true); | ||
|
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 |
||
| if (reader.GetCursor() != cursor) { | ||
|
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. [suggestion] The dual-cursor tracking between the lambda-captured The post-deserialization check could rely solely on |
||
| throw VsagException(ErrorType::UNSUPPORTED_INDEX_OPERATION, | ||
| "v0.14 sequential stream does not support seek"); | ||
| } | ||
| } catch (const std::bad_alloc& e) { | ||
|
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 |
||
| throw VsagException(ErrorType::NO_ENOUGH_MEMORY, "failed to Deserialize: ", e.what()); | ||
|
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 |
||
| } | ||
| } | ||
|
|
||
| void | ||
| HGraph::Deserialize(StreamReader& reader) { | ||
| // try to deserialize footer (only in new version) | ||
| auto footer = Footer::Parse(reader); | ||
| this->deserialize(reader, false); | ||
| } | ||
|
|
||
| void | ||
| HGraph::deserialize(StreamReader& reader, bool force_v0_14) { | ||
| FooterPtr footer = nullptr; | ||
| if (not force_v0_14) { | ||
| // try to deserialize footer (only in new version) | ||
| footer = Footer::Parse(reader); | ||
| } | ||
|
|
||
| if (footer == nullptr) { // old format, DON'T EDIT, remove in the future | ||
| logger::debug("parse with v0.14 version format"); | ||
|
|
@@ -1474,6 +1531,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 = | ||
|
jac0626 marked this conversation as resolved.
|
||
| static_cast<uint64_t>(this->label_table_->GetTotalCount()); | ||
| this->label_table_->DeserializeDuplicateRecords(reader, logical_element_count); | ||
|
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"); | ||
|
|
||
|
|
@@ -2222,6 +2284,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); | ||
|
|
||
| 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 |
|---|---|---|
|
|
@@ -508,6 +508,9 @@ HGraphAnalyzer::GetDegreeDistribution() { | |
| Vector<uint32_t> in_degree(this->total_count_, allocator_); | ||
| Vector<uint32_t> out_degree(this->total_count_, allocator_); | ||
| for (InnerIdType i = 0; i < this->total_count_; ++i) { | ||
| if (is_duplicate_ids_[i]) { | ||
|
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] This is a straightforward fix — duplicate IDs have no real graph edges, so including them in the degree distribution would produce misleading zero-degree entries. The same |
||
| continue; | ||
| } | ||
| Vector<InnerIdType> neighbors(allocator_); | ||
| hgraph_->bottom_graph_->GetNeighbors(i, neighbors); | ||
| out_degree[i] = neighbors.size(); | ||
|
|
||
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