feat(hgraph): restore conjugate graph enhancement - #2675
Conversation
|
/label status/waiting-for-review |
Merge Protections🟢 All 3 merge protections satisfied — ready to merge. Show 3 satisfied protections🟢 Require kind label
🟢 Require version label
🟢 Require linked issue for feature/bug PRs
|
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Restores and integrates the HGraph conjugate-graph enhancement end-to-end (parameters, feedback/pretrain APIs, search-time result enhancement, and binary/streaming serialization), plus updates example and EN/ZH docs.
Changes:
- Added HGraph build/search parameters to enable conjugate-graph construction and toggled search-time usage.
- Implemented
Feedback/Pretrain/UpdateIdconjugate-graph integration, including streaming/binary serialization and memory accounting. - Migrated the C++ runnable example and synchronized English/Chinese documentation for the feature.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_hgraph.cpp | Adds functional coverage for conjugate-graph feedback, ID updates, and serialization roundtrips. |
| src/storage/serialization_tags.h | Introduces a new streaming block tag for conjugate-graph payloads. |
| src/impl/conjugate_graph.h | Adds StreamWriter-based serialization API for ConjugateGraph. |
| src/impl/conjugate_graph.cpp | Implements StreamWriter serialization for conjugate-graph state and footer. |
| src/algorithm/hgraph/hgraph_serialize.cpp | Serializes/deserializes conjugate-graph in binary + streaming formats and exposes metadata/memory usage. |
| src/algorithm/hgraph/hgraph_search.cpp | Enhances search results using conjugate-graph edges when enabled. |
| src/algorithm/hgraph/hgraph_parameter_test.cpp | Adds unit tests for parameter mapping and search parameter parsing. |
| src/algorithm/hgraph/hgraph_parameter.h | Adds new build/search parameter fields for conjugate-graph enablement and search toggle. |
| src/algorithm/hgraph/hgraph_parameter.cpp | Parses/serializes the new parameters and includes them in compatibility checks. |
| src/algorithm/hgraph/hgraph_param_mapping.cpp | Maps external param key(s) for conjugate-graph into internal HGraph config. |
| src/algorithm/hgraph/hgraph_enhance.cpp | Implements Feedback, Pretrain, and UpdateId conjugate-graph wiring. |
| src/algorithm/hgraph/hgraph.h | Adds conjugate-graph fields/mutex and new public API overrides. |
| src/algorithm/hgraph/hgraph.cpp | Initializes conjugate-graph based on build params. |
| src/algorithm/hgraph/CMakeLists.txt | Adds the new enhancement implementation unit to the build. |
| examples/cpp/304_feature_enhance_graph.cpp | Updates the example to HGraph + new parameter structure. |
| docs/docs/zh/src/resources/index_parameters.md | Documents use_conjugate_graph and use_conjugate_graph_search (ZH). |
| docs/docs/zh/src/indexes/hgraph.md | Documents the new build parameter (ZH). |
| docs/docs/zh/src/advanced/enhance_graph.md | Updates enhancement docs to HGraph APIs and new JSON structure (ZH). |
| docs/docs/en/src/resources/index_parameters.md | Documents use_conjugate_graph and use_conjugate_graph_search (EN). |
| docs/docs/en/src/indexes/hgraph.md | Documents the new build parameter (EN). |
| docs/docs/en/src/advanced/enhance_graph.md | Updates enhancement docs to HGraph APIs and new JSON structure (EN). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
LHT129
left a comment
There was a problem hiding this comment.
Code review for feat(hgraph): restore conjugate graph enhancement
09c0a4d to
98d83af
Compare
LHT129
left a comment
There was a problem hiding this comment.
Automated review by vsag-pr-review-agent
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/algorithm/hgraph/hgraph_search.cpp:754
- In the conjugate-graph enhancement path, results are converted inner_id -> label and then label -> inner_id again. When duplicate external IDs are enabled, LabelTable can have multiple inner IDs for the same label, and TryGetIdByLabel() returns an arbitrary one. That can cause mismatched (distance, inner_id) pairs (distances from one duplicate but extra_info/inner_id from another) when pushing back into search_result.
while (not label_results.empty()) {
const auto record = label_results.top();
label_results.pop();
const auto [found, inner_id] = this->label_table_->TryGetIdByLabel(record.second, true);
if (found and (ft == nullptr or ft->CheckValid(inner_id))) {
examples/cpp/304_feature_enhance_graph.cpp:67
- If CreateIndex() fails, hgraph remains null but is still dereferenced (hgraph->Build), which will crash the example. The example should exit/return on create failure and ideally surface the factory error message.
std::shared_ptr<vsag::Index> hgraph;
if (auto index = vsag::Factory::CreateIndex("hgraph", hgraph_build_parameters);
index.has_value()) {
hgraph = index.value();
} else {
98d83af to
4844741
Compare
LHT129
left a comment
There was a problem hiding this comment.
Review Summary
This PR restores conjugate graph enhancement for HGraph with comprehensive coverage: Feedback/Pretrain methods, search-time enhancement integration, UpdateId support, memory accounting, and both binary and streaming serialization. The implementation is well-structured and the test coverage is thorough.
Issues from previous review rounds
All previously reported issues have been addressed:
- Lock ordering:
label_lookup_mutex_is consistently acquired beforeconjugate_graph_mutex_across all code paths (Feedback,Pretrain,UpdateId, search enhancement). - Filter semantics: The enhancement result rebuild now holds a shared label lock and re-checks
ft->CheckValid(inner_id)before pushing results back. - Performance:
flattenandcomputerare created once outside the per-label lambda. - Streaming deserialization: Now correctly rejects a conjugate-graph block when the target HGraph was created without the feature, matching binary deserialization behavior.
- Typo fix: "constains" → "contains" in the example.
Current assessment
The code is clean, well-tested, and all critical concerns from earlier rounds have been resolved. No new issues found in this revision.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
src/impl/conjugate_graph.cpp:185
- The streaming format writes adjacency entries without an explicit
conjugate_graph_entry count, which makes the payload harder to evolve and validate (the reader must infer termination via footer mechanics). Consider serializing the number of nodes/entries up front (e.g.,conjugate_graph_.size()) so deserialization can be bounded and format changes are easier to manage.
StreamWriter::WriteObj(out_stream, memory_usage_);
for (const auto& [tag_id, neighbor_ptr] : conjugate_graph_) {
StreamWriter::WriteObj(out_stream, tag_id);
uint64_t neighbor_set_size = neighbor_ptr->size();
StreamWriter::WriteObj(out_stream, neighbor_set_size);
for (const auto neighbor_tag_id : *neighbor_ptr) {
StreamWriter::WriteObj(out_stream, neighbor_tag_id);
}
}
src/impl/conjugate_graph.cpp:190
- Serializing the footer through a temporary
std::stringstreamforces an extra buffering + string copy (str()), which can be noticeable for large serialized payloads or frequent serialization. Consider adding aFooter::Serialize(StreamWriter&)(or writing the footer into a pre-sized buffer once) to avoid the extra allocation/copy.
std::stringstream footer_stream(std::ios::in | std::ios::out | std::ios::binary);
footer_.Serialize(footer_stream);
const auto footer_data = footer_stream.str();
out_stream.Write(footer_data.data(), footer_data.size());
}
src/algorithm/hgraph/hgraph_search.cpp:754
- The
label_lookup_mutex_shared-lock is held across potentially expensive work (flatten->Query(...)andEnhanceResult(...)). This can increase lock contention and delay concurrent operations that need the label table (e.g., updates). Consider narrowing the lock scope: extract/copy the needed label mappings (or the initial label list) under the lock, then release it before computing distances and enhancing results; reacquire only for the finalTryGetIdByLabelre-mapping (or precompute those mappings too).
std::shared_lock label_lock(this->label_lookup_mutex_);
while (not search_result->Empty()) {
const auto record = search_result->Top();
search_result->Pop();
label_results.emplace(record.first, this->label_table_->GetLabelById(record.second));
}
const auto flatten = use_custom_distance ? nullptr : this->get_precise_codes();
const auto computer = use_custom_distance ? nullptr : flatten->FactoryComputer(raw_query);
const auto distance_of_label = [&](int64_t label) {
const auto [found, inner_id] = this->label_table_->TryGetIdByLabel(label, true);
if (not found or (ft != nullptr and not ft->CheckValid(inner_id))) {
return std::numeric_limits<float>::max();
}
float distance = std::numeric_limits<float>::max();
if (use_custom_distance) {
request.distance_batch_func_(&label, 1, &distance);
} else {
flatten->Query(&distance, computer, &inner_id, 1, &ctx);
}
if (request.threshold_.has_value() and distance > request.threshold_.value()) {
return std::numeric_limits<float>::max();
}
return distance;
};
{
std::shared_lock graph_lock(this->conjugate_graph_mutex_);
(void)this->conjugate_graph_->EnhanceResult(label_results, distance_of_label);
}
while (not label_results.empty()) {
const auto record = label_results.top();
label_results.pop();
const auto [found, inner_id] = this->label_table_->TryGetIdByLabel(record.second, true);
if (found and (ft == nullptr or ft->CheckValid(inner_id))) {
search_result->Push(record.first, inner_id);
}
}
examples/cpp/304_feature_enhance_graph.cpp:69
- If
CreateIndex(...)fails, the example continues and will dereferencehgraphlater (e.g.,hgraph->Build(...)), which will crash. The example should exit/return on failure (or throw) right after printing the error.
std::shared_ptr<vsag::Index> hgraph;
if (auto index = vsag::Factory::CreateIndex("hgraph", hgraph_build_parameters);
index.has_value()) {
hgraph = index.value();
} else {
std::cout << "Create HGraph Error" << std::endl;
}
fe8538a to
3311525
Compare
LHT129
left a comment
There was a problem hiding this comment.
Code Review for PR #2675 (updated commit 3311525)
Thank you for the amendments — most of the previously flagged issues have been addressed. Below are the remaining substantive observations:
[suggestion] memory_usage_ is uint32_t but tracks cumulative byte counts
File: src/impl/conjugate_graph.h:71
GetMemoryUsage() returns uint64_t via a widening cast, so callers see a correct type, but the internal accumulator silently wraps at 4 GiB. For large-scale deployments with many conjugate edges this ceiling could be reached. Consider changing memory_usage_ to uint64_t and updating the serialization format accordingly (the header currently writes sizeof(memory_usage_) bytes, so the format version would need to be bumped or a format discriminator added).
[suggestion] UpdateId return value from ConjugateGraph::UpdateId is discarded
File: src/algorithm/hgraph/hgraph_enhance.cpp:152
HGraph::UpdateId calls (void)this->conjugate_graph_->UpdateId(old_id, new_id), discarding the return value. ConjugateGraph::UpdateId returns false when both old_tag_id and new_tag_id already exist as keys in the conjugate graph. While InnerIndexInterface::UpdateId updates the label table first, if the label table update succeeds but the conjugate graph update fails (e.g. both IDs already have conjugate entries), the conjugate graph becomes silently inconsistent with the label table. Consider at least logging a warning, or propagating the error.
[note] Pretrain O(n²) search complexity
File: src/algorithm/hgraph/hgraph_enhance.cpp:99
Pretrain calls KnnSearch for each base tag ID (with GENERATE_SEARCH_K=50), then calls Feedback for each neighbor found. This is O(base_tag_ids × GENERATE_SEARCH_K × KnnSearch). For large datasets this can be extremely expensive. This is by design for an offline training phase, but worth documenting the expected cost in the API docs.
Overall assessment
The implementation is well-structured with proper locking, serialization, and error handling. The test coverage (test_hgraph.cpp lines 579-700) covers the core feedback/pretrain/update/serialization flows including edge cases (empty index, disabled conjugate graph, mismatched serialization). The example (304_feature_enhance_graph.cpp) demonstrates the intended usage pattern clearly.
LHT129
left a comment
There was a problem hiding this comment.
Code Review for PR #2675 (updated commit 3311525)
Thank you for the amendments — most of the previously flagged issues have been addressed. Below are the remaining substantive observations:
[suggestion] memory_usage_ is uint32_t but tracks cumulative byte counts
File: src/impl/conjugate_graph.h:71
GetMemoryUsage() returns uint64_t via a widening cast, so callers see a correct type, but the internal accumulator silently wraps at 4 GiB. For large-scale deployments with many conjugate edges this ceiling could be reached. Consider changing memory_usage_ to uint64_t and updating the serialization format accordingly (the header currently writes sizeof(memory_usage_) bytes, so the format version would need to be bumped or a format discriminator added).
[suggestion] UpdateId return value from ConjugateGraph::UpdateId is discarded
File: src/algorithm/hgraph/hgraph_enhance.cpp:152
HGraph::UpdateId calls (void)this->conjugate_graph_->UpdateId(old_id, new_id), discarding the return value. ConjugateGraph::UpdateId returns false when both old_tag_id and new_tag_id already exist as keys in the conjugate graph. While InnerIndexInterface::UpdateId updates the label table first, if the label table update succeeds but the conjugate graph update fails (e.g. both IDs already have conjugate entries), the conjugate graph becomes silently inconsistent with the label table. Consider at least logging a warning, or propagating the error.
[note] Pretrain O(n²) search complexity
File: src/algorithm/hgraph/hgraph_enhance.cpp:99
Pretrain calls KnnSearch for each base tag ID (with GENERATE_SEARCH_K=50), then calls Feedback for each neighbor found. This is O(base_tag_ids × GENERATE_SEARCH_K × KnnSearch). For large datasets this can be extremely expensive. This is by design for an offline training phase, but worth documenting the expected cost in the API docs.
Overall assessment
The implementation is well-structured with proper locking, serialization, and error handling. The test coverage (test_hgraph.cpp lines 579-700) covers the core feedback/pretrain/update/serialization flows including edge cases (empty index, disabled conjugate graph, mismatched serialization). The example (304_feature_enhance_graph.cpp) demonstrates the intended usage pattern clearly.
LHT129
left a comment
There was a problem hiding this comment.
[critical] Missing constant definitions for GENERATE_SEARCH_K, GENERATE_SEARCH_L, and GENERATE_OMEGA
In src/algorithm/hgraph/hgraph_enhance.cpp, three constants are used but never defined anywhere in the PR diff:
GENERATE_SEARCH_L(line 105) — used infmt::formatfor ef_searchGENERATE_SEARCH_K(line 119) — used as k for KnnSearch in PretrainGENERATE_OMEGA(line 138) — used as interpolation weight for vector generation
This will cause a compilation failure. They should be defined as static constexpr values, either locally in hgraph_enhance.cpp or in hgraph.h / common.h.
LHT129
left a comment
There was a problem hiding this comment.
[suggestion] Type mismatch in Deserialize loop variable
In src/impl/conjugate_graph.cpp, the Deserialize method declares neighbor_size as uint64_t but uses int i as the loop variable:
uint64_t neighbor_size = 0;
// ...
for (int i = 0; i < neighbor_size; i++) {While MAXIMUM_DEGREE=128 makes overflow unlikely in practice, the signed/unsigned mismatch is a static analysis warning and a latent bug if the limit ever changes. Consider using uint64_t i or size_t i for consistency.
LHT129
left a comment
There was a problem hiding this comment.
[note] Race window between exact search and KnnSearch in Feedback auto mode
When global_optimum_tag_id == max(), the Feedback method:
- Acquires
global_lock, performs exact search, finds the global optimum, releasesglobal_lock - Calls
KnnSearchwithout any lock - Acquires
label_lock(shared) +graph_lock(unique) to add conjugate edges
Between step 1 and step 2, concurrent Add operations can change the index state. This means the KnnSearch results in step 2 may not correspond to the same index state where the exact nearest neighbor was found in step 1. The resulting conjugate edges (mapping KnnSearch results → exact nearest neighbor) are still semantically valid, but the mapping may be less precise than expected.
This is likely acceptable given the online learning nature of Feedback, but worth documenting as a known behavior.
LHT129
left a comment
There was a problem hiding this comment.
[note] Pretrain passes raw parameters to Feedback, which may enable conjugate graph search during training
In Pretrain, the neighbor-finding KnnSearch uses generate_parameters with use_conjugate_graph_search: false, which is correct. However, the subsequent Feedback call passes the raw parameters argument directly:
inserted += this->Feedback(generated, k, parameters, base_tag_id);If the caller passes search parameters with use_conjugate_graph_search: true, the KnnSearch inside Feedback will use conjugate graph enhancement during training. This creates a feedback loop where the conjugate graph influences its own training data. Consider whether Feedback inside Pretrain should always disable conjugate graph search, similar to how neighbor search does.
LHT129
left a comment
There was a problem hiding this comment.
[suggestion] Return value of conjugate_graph_->UpdateId is discarded
In src/algorithm/hgraph/hgraph_enhance.cpp, the UpdateId override:
if (updated and this->use_conjugate_graph_) {
std::unique_lock graph_lock(this->conjugate_graph_mutex_);
(void)this->conjugate_graph_->UpdateId(old_id, new_id);
}ConjugateGraph::UpdateId returns tl::expected<bool, Error> and can fail (e.g., when both old and new IDs already exist in the graph). Discarding this result means the conjugate graph may silently become inconsistent with the label table after an UpdateId call. Consider either propagating the error or at least logging a warning when it fails.
LHT129
left a comment
There was a problem hiding this comment.
[critical] Missing constant definitions for GENERATE_SEARCH_K, GENERATE_SEARCH_L, and GENERATE_OMEGA
In src/algorithm/hgraph/hgraph_enhance.cpp, three constants are used but never defined anywhere in the PR diff:
GENERATE_SEARCH_L(line 105) — used infmt::formatfor ef_searchGENERATE_SEARCH_K(line 119) — used as k for KnnSearch in PretrainGENERATE_OMEGA(line 138) — used as interpolation weight for vector generation
This will cause a compilation failure. They should be defined as static constexpr values, either locally in hgraph_enhance.cpp or in hgraph.h / common.h.
LHT129
left a comment
There was a problem hiding this comment.
[suggestion] Type mismatch in Deserialize loop variable
In src/impl/conjugate_graph.cpp, the Deserialize method declares neighbor_size as uint64_t but uses int i as the loop variable:
uint64_t neighbor_size = 0;
// ...
for (int i = 0; i < neighbor_size; i++) {While MAXIMUM_DEGREE=128 makes overflow unlikely in practice, the signed/unsigned mismatch triggers compiler warnings (-Wsign-compare) and is a latent bug if the degree limit ever increases. Consider using uint64_t i or size_t i for consistency.
LHT129
left a comment
There was a problem hiding this comment.
[note] Race window between exact search and KnnSearch in Feedback auto mode
When global_optimum_tag_id == max(), the Feedback method:
- Acquires
global_lock, performs exact search, finds the global optimum, releasesglobal_lock - Calls
KnnSearchwithout any lock - Acquires
label_lock(shared) +graph_lock(unique) to add conjugate edges
Between step 1 and step 2, concurrent Add operations can change the index state. This means the KnnSearch results in step 2 may not correspond to the same index state where the exact nearest neighbor was found in step 1. The resulting conjugate edges (mapping KnnSearch results → exact nearest neighbor) are still semantically valid, but the mapping may be less precise than expected.
This is likely acceptable given the online learning nature of Feedback, but worth documenting as a known behavior.
LHT129
left a comment
There was a problem hiding this comment.
[note] Pretrain passes raw parameters to Feedback, which may enable conjugate graph search during training
In Pretrain, the neighbor-finding KnnSearch correctly uses generate_parameters with use_conjugate_graph_search: false. However, the subsequent Feedback call passes the raw parameters argument directly:
inserted += this->Feedback(generated, k, parameters, base_tag_id);If the caller passes search parameters with use_conjugate_graph_search: true, the KnnSearch inside Feedback will use conjugate graph enhancement during training. This creates a feedback loop where the conjugate graph influences its own training data. Consider whether Feedback inside Pretrain should always disable conjugate graph search, similar to how neighbor search does.
LHT129
left a comment
There was a problem hiding this comment.
[suggestion] Return value of conjugate_graph_->UpdateId is discarded
In src/algorithm/hgraph/hgraph_enhance.cpp, the UpdateId override:
if (updated and this->use_conjugate_graph_) {
std::unique_lock graph_lock(this->conjugate_graph_mutex_);
(void)this->conjugate_graph_->UpdateId(old_id, new_id);
}ConjugateGraph::UpdateId returns tl::expected<bool, Error> and can fail (e.g., when both old and new IDs already exist in the graph). Discarding this result means the conjugate graph may silently become inconsistent with the label table after an UpdateId call. Consider either propagating the error or at least logging a warning when it fails.
| if (this->use_conjugate_graph_ && !loaded_conjugate_graph) { | ||
| throw VsagException(ErrorType::READ_ERROR, | ||
| "HGraph streaming serialization conjugate graph block is missing"); | ||
| } |
There was a problem hiding this comment.
[suggestion] The streaming deserialization path (line 862) makes the CONJUGATE_GRAPH block mandatory whenever use_conjugate_graph_ is true, even when the serialized stream was produced by an older index that never had conjugate-graph data. This differs from the non-streaming Deserialize(StreamReader&) path (line 1083), which keys the decision off the serialized metadata (has_conjugate_graph).
Consider adding a has_conjugate_graph flag to the streaming header metadata (similar to how has_precise_reorder() and extra_info_size_ are handled) so that a stream without conjugate-graph data can still be deserialized into a conjugate-graph-enabled target, leaving an empty conjugate graph.
| void | ||
| Serialize(StreamWriter& out_stream) const; | ||
|
|
||
| tl::expected<void, Error> |
There was a problem hiding this comment.
[suggestion] memory_usage_ is uint32_t (max ~4 GB). For a conjugate graph with many labels and edges, this could overflow. The AddNeighbor check (memory_usage_ + sizeof(int64_t) > std::numeric_limits<uint32_t>::max()) prevents overflow but silently drops edges once the limit is reached, which could silently degrade recall.
Consider using size_t for memory_usage_ to avoid the 4 GB ceiling, or at minimum log a warning when edges are dropped due to the limit.
|
|
||
| bool | ||
| HGraph::UpdateId(int64_t old_id, int64_t new_id) { | ||
| const auto updated = InnerIndexInterface::UpdateId(old_id, new_id); |
There was a problem hiding this comment.
[note] The Pretrain loop calls Feedback for each generated query. If Feedback fails for one query (e.g. brute_force_search returns empty), the loop continues silently. This is reasonable for best-effort pretraining, but a counter of failed queries might help diagnose when the pretraining dataset is too sparse to produce useful conjugate edges.
| fmt::format("pretrain base id {} does not belong to the index", base_tag_id)); | ||
| base_inner_id = inner_id; | ||
| } | ||
| this->GetVectorByInnerId(base_inner_id, base_vector.data()); |
There was a problem hiding this comment.
[note] base_inner_id is obtained under label_lookup_mutex_ (line 111-116) but GetVectorByInnerId(base_inner_id, ...) on line 118 is called after the lock is released. If a concurrent Remove or UpdateId invalidates base_inner_id between these two points, GetVectorByInnerId could access a stale or invalid slot.
The same pattern applies to neighbor_inner_id on lines 127-135.
Since Pretrain is typically an offline/batch operation where concurrent modifications are not expected, this is low risk. Consider adding a comment noting the assumption, or holding the lock across the GetVectorByInnerId call if concurrent safety is desired.
| const auto* ids = result->GetIds(); | ||
| const auto result_size = result->GetDim(); | ||
| uint32_t inserted = 0; | ||
| std::shared_lock label_lock(this->label_lookup_mutex_); |
There was a problem hiding this comment.
[suggestion] The label_lock (shared) on line 53 is held across the entire AddNeighbor loop (lines 57-71), which includes acquiring the conjugate_graph_mutex_ (unique) and performing hash-table insertions. If AddNeighbor triggers a rehash or is otherwise slow, this shared lock blocks all label-table writers for the duration of the feedback loop.
Consider narrowing the scope: validate global_optimum_tag_id under label_lock, release it, then acquire graph_lock and perform the loop. The per-result TryGetIdByLabel calls inside the loop (line 61) operate on ids[i] which came from KnnSearch — these labels are already validated to exist at search time, so the CHECK_ARGUMENT there is a defense-in-depth check that could use a shorter-lived lock or be relaxed to a non-locking lookup if the label table supports it.
| } | ||
| memory_usage_ += sizeof(to_tag_id); | ||
| search_key->second->insert(to_tag_id); | ||
| memory_usage_ += static_cast<uint32_t>(added_bytes); |
There was a problem hiding this comment.
[suggestion] The static_cast<uint32_t>(added_bytes) truncation here and the uint32_t memory_usage_ field together mean that once the conjugate graph exceeds 4 GiB of tracked memory, memory_usage_ wraps around silently. The overflow guard above (added_bytes > UINT32_MAX - memory_usage_) only protects against a single AddNeighbor call exceeding the remaining headroom, but does not prevent cumulative silent wraparound from many small additions. Since GetMemoryUsage() already returns uint64_t, consider changing memory_usage_ to uint64_t and removing the static_cast.
| generated_vector[d] = | ||
| GENERATE_OMEGA * base_vector[d] + (1.0F - GENERATE_OMEGA) * neighbor_vector[d]; | ||
| } | ||
| inserted += this->Feedback(generated, k, parameters, base_tag_id); |
There was a problem hiding this comment.
[note] Pretrain passes the caller-supplied parameters directly to Feedback (line 140), which uses them for its internal KnnSearch. If the caller enables use_conjugate_graph_search: true in these parameters, the conjugate graph will be consulted during the feedback search itself. While this does not cause infinite recursion (Feedback does not call itself), it means the search results used for edge insertion may be influenced by an incomplete/in-progress conjugate graph, potentially reducing feedback quality. Consider documenting that callers should disable use_conjugate_graph_search in the parameters passed to Pretrain, or force-disable it internally as is done for the neighbor-lookup search (generate_parameters).
| float radius, | ||
| QueryContext* ctx, | ||
| const std::optional<float>& threshold) const { | ||
| std::shared_lock codes_lock(this->persistent_codes_mutex_); |
There was a problem hiding this comment.
[note] The brute_force_search changes (adding persistent_codes_mutex_ lock and fixing the dedup-storage total-count calculation) are unrelated bug fixes bundled with this feature PR. While these fixes are necessary for Feedback to safely call brute_force_search, they also affect all other callers of brute_force_search. Consider mentioning these ancillary fixes in the PR description for visibility, since they change behavior outside the conjugate-graph feature.
LHT129
left a comment
There was a problem hiding this comment.
经过对最新 commit (3311525) 的全面审查,确认之前 Copilot 和其他 reviewer 发现的所有重要问题都已在修正后的代码中得到解决:
已确认修复的问题:
- 锁顺序问题 —
Feedback中现在正确使用label_lookup_mutex_(shared) →conjugate_graph_mutex_(unique) →memory_usage_mutex_(unique) 的顺序 - 过滤语义 — conjugate graph 增强后重新检查
is_allowed(inner_id)再 push 回结果 - flatten/computer 性能 — 在 lambda 外部创建,避免重复构造
- label-table 无锁访问 —
Pretrain中已使用TryGetIdByLabel并持有label_lookup_mutex_ - streaming deserialization 一致性 — 现在 streaming 路径也拒绝 mismatched conjugate graph 块
- 示例 typo — 已修正
memory_usage_溢出 —AddNeighbor中已添加 overflow checkUpdateId锁范围 — 已缩小为仅包裹 conjugate graph 更新
代码质量评估:
- 新文件
hgraph_enhance.cpp结构清晰,错误处理完善 - 序列化支持完整(binary + streaming),版本兼容性处理得当
- 测试覆盖全面:基本功能、并发安全、range search 不受影响、参数解析、序列化往返
- 内存统计正确集成到
cal_memory_usage()和GetMemoryUsageDetail() - 参数映射遵循现有模式,
CheckCompatibility正确包含use_conjugate_graph
没有发现新的阻塞性问题。
| std::shared_lock label_lock(this->label_lookup_mutex_); | ||
| const auto [found, inner_id] = | ||
| this->label_table_->TryGetIdByLabel(neighbor_tag_id, true); | ||
| CHECK_ARGUMENT(found, |
There was a problem hiding this comment.
[suggestion] Pretrain acquires and releases label_lookup_mutex_, conjugate_graph_mutex_, and memory_usage_mutex_ on every inner iteration (once per generated neighbor vector via the Feedback call). For large base_tag_ids × GENERATE_SEARCH_K, this lock churn can become measurable. Consider batching: collect all (neighbor_label, base_label) pairs first under a single label_lock acquisition, then call AddNeighbor in a batch under one graph_lock, and update memory_usage_ once at the end.
| HGraph::UpdateId(int64_t old_id, int64_t new_id) { | ||
| const auto updated = InnerIndexInterface::UpdateId(old_id, new_id); | ||
| if (updated and this->use_conjugate_graph_) { | ||
| std::unique_lock graph_lock(this->conjugate_graph_mutex_); |
There was a problem hiding this comment.
[note] ConjugateGraph::UpdateId returns false when both old_tag_id and new_tag_id exist in the conjugate graph simultaneously. HGraph::UpdateId discards this return with (void). The author notes this path cannot be reached through the current HGraph::UpdateId call chain because InnerIndexInterface::UpdateId rejects an occupied new_id first. However, if the conjugate graph is ever updated through another code path (e.g. future merge/import logic), silently ignoring the failure could lead to data loss. Consider at least logging a warning when UpdateId returns false, or asserting that it cannot happen.
| float radius, | ||
| QueryContext* ctx, | ||
| const std::optional<float>& threshold) const { | ||
| std::shared_lock codes_lock(this->persistent_codes_mutex_); |
There was a problem hiding this comment.
[note] The addition of std::shared_lock codes_lock(this->persistent_codes_mutex_) at the start of brute_force_search changes the locking semantics for all callers of this function, not just the new conjugate-graph feedback path. While this is a correctness improvement (the function accesses flatten and total which require synchronization), it is worth noting that this introduces lock acquisition on a previously lock-free code path.
| } | ||
|
|
||
| auto total = static_cast<InnerIdType>(this->total_count_.load()); | ||
| // Add reserves logical ids before their code slots are published. |
There was a problem hiding this comment.
[note] The new total calculation logic at lines 361-363 uses GetCodeStorageCounts().first and std::min(total, total_count_) to handle the dedup storage case. This diverges from the previous simple total = this->total_count_ assignment. While the intent is clear (accounting for dedup storage where code count may differ from total count), please confirm that GetCodeStorageCounts().first returns the correct count for the brute-force search path in all cases, including when dedup storage is not in use.
| int dim = 128; | ||
| int base_elements = 2000; | ||
| int query_elements = 1000; | ||
| int ef_search = 10; |
There was a problem hiding this comment.
[suggestion] The variable int ef_search = 10 is declared but never used. The search parameters in the JSON strings below hardcode "ef_search": 10 directly. Either remove this unused variable or use it in the parameter strings (e.g., via string formatting) to make the example easier to modify.
Summary
Feedback/PretrainsupportValidation
Fixes: #2650