feat: support custom batch query distances - #2551
Hidden character warning
Conversation
|
/label status/waiting-for-review |
|
Automated pull request review completed. Review effort: Submitted 2 inline comments. |
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
|
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: medium (310 changed lines across 8 files).
Submitted 4 inline comments.
Reviewed commit ca17c0d.
LHT129
left a comment
There was a problem hiding this comment.
Overall this is a well-structured PR that adds request-scoped custom distance callbacks to BruteForce and HGraph, with clean rejection in IVF. The implementation correctly disables features that depend on vector-space semantics (reorder, rabitq, brute-force threshold fallback, duplicate-finding) and forces single-threaded execution when the callback is active.
A few areas worth addressing before merge:
-
HGraph entry-point filtering (basic_searcher.cpp:441): When the entry point is rejected by the filter, the search returns empty immediately instead of traversing through it. Normal HGraph search scores and traverses a rejected entry point; the custom path should do the same.
-
Duplicate external IDs (basic_searcher.cpp:520): When
consider_duplicateis true, duplicate IDs are pushed with the representative's callback score without invoking the callback for each duplicate. Since custom scores are per external ID, this can produce wrong distances. -
Per-batch label allocation (basic_searcher.cpp:427):
score_idsconstructs aVector<int64_t>per callback batch inside the graph expansion hot loop. With the defaultdistance_batch_size_of 1, this is one allocation per scored neighbor. A pre-allocated buffer would avoid this. -
Scalar validation (bruteforce.cpp:385): Skipping
validate_knn_args/validate_range_argsentirely also skips checks fortopk_ == 0,limited_size_ == 0, and negative radii — all of which are independent of the query vector. These should still be validated. -
Missing HGraph error-path tests: BruteForce has validation tests for
distance_batch_size_ == 0and non-finite scores; HGraph should have equivalent coverage, plus a test for the range-search rejection path.
ca17c0d to
fac0f30
Compare
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: medium (361 changed lines across 8 files).
Submitted 3 inline comments.
Reviewed commit fac0f30.
LHT129
left a comment
There was a problem hiding this comment.
All previously reported issues have been addressed in this commit:
- [P2] Scalar search-argument validation: BruteForce now independently validates
topk_,radius_, andlimited_size_for custom distance callbacks instead of skipping the entire validator. - [P2] Preserve traversal when entry point is filtered:
basic_searcher.cppnow scores the entry point viascore_idsand letscheck_funccontrol result admission, preserving graph connectivity. - [P2] Score duplicate external IDs independently: Duplicate IDs now invoke
score_idsindividually in the custom distance path. - [P2] Avoid per-batch label vector allocation:
custom_labelsis now pre-allocated once per search and reused acrossscore_idsinvocations. - [suggestion] HGraph error-path tests: Validation tests for
distance_batch_size_ == 0, non-finite scores, and range search rejection are now present.
No new issues found. The implementation correctly handles parameter validation, graph traversal, batching, and duplicate IDs across BruteForce and HGraph. IVF correctly rejects the callback with a clear error.
fac0f30 to
991a6c8
Compare
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: medium (437 changed lines across 10 files).
Submitted 2 inline comments.
Reviewed commit 991a6c8.
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (573 changed lines across 10 files).
Submitted 3 inline comments.
Reviewed commit 4f8f691.
4f8f691 to
67b945b
Compare
67b945b to
17dd323
Compare
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (810 changed lines across 12 files).
Submitted 5 inline comments.
Reviewed commit 17dd323.
17dd323 to
63b6e11
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The implementation adds IVF callback support but the linked issue/PR description state IVF should reject callbacks, so the intended behavior/scope needs to be reconciled before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds a request-scoped, batched “custom distance” callback to SearchRequest and threads it through search execution so callers can score candidates by stable external IDs at query time (runtime-only, index metric for build remains unchanged).
Changes:
- Introduces
SearchDistanceBatchFunc+distance_batch_size_onvsag::SearchRequestand plumbs them into inner search parameters. - Implements callback-driven scoring in BruteForce and HGraph KNN traversal (including validation and duplicate-handling adjustments).
- Adds IVF callback search path (plus validation), updates API docs (EN/ZH), and adds functional tests for BruteForce/HGraph/IVF callback behavior and invalid configurations.
File summaries
| File | Description |
|---|---|
| tests/test_ivf.cpp | Adds coverage for IVF callback KNN plus rejected modes/configs. |
| tests/test_hgraph.cpp | Adds coverage for HGraph callback KNN plus validation failures. |
| tests/test_brute_force.cpp | Adds callback KNN/range + validation coverage; includes batching assertions. |
| src/impl/searcher/basic_searcher.cpp | Enables callback scoring path inside graph traversal when distance_batch_func is set. |
| src/impl/inner_search_param.h | Carries callback + batch size into inner search params. |
| src/algorithm/ivf/ivf.h | Declares a custom-distance IVF search helper. |
| src/algorithm/ivf/ivf.cpp | Implements callback-driven IVF candidate scoring + request validation gate. |
| src/algorithm/hgraph/hgraph_search.cpp | Validates/rejects unsupported options in callback mode; routes traversal to callback scoring. |
| src/algorithm/bruteforce/bruteforce.cpp | Adds callback scoring path with batching; improves exception handling for parallel futures. |
| include/vsag/search_request.h | Public API: adds callback type + fields and documents behavior. |
| docs/docs/en/src/api/search.md | Documents callback semantics and per-index support/constraints (EN). |
| docs/docs/zh/src/api/search.md | Documents callback semantics and per-index support/constraints (ZH). |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (855 changed lines across 12 files).
Submitted 1 inline comment.
Reviewed commit 63b6e11.
LHT129
left a comment
There was a problem hiding this comment.
This iteration addresses all previously raised issues:
- Entry-point filtering (P2): Entry point is now scored via
score_idsbefore the filter check, preserving graph traversal connectivity. - Duplicate scoring (P2): The
score_duplicateslambda scores each eligible duplicate independently through the callback, with proper batching and overflow protection. - Scalar validation (P2):
topk,radius, andlimited_sizeare now independently validated for custom-distance requests, preventing OOB access on zero-capacity heaps. - Label buffer allocation (P2):
custom_labelsis now a pre-allocatedVectorbounded bymin(distance_batch_size, graph->MaximumDegree()), eliminating per-batch allocations in the hot loop. - Reasoning output (P3): HGraph custom callback searches now retain expected-label reasoning with true distances obtained through the callback.
- Duplicate stat counting (P3):
dist_cmpis incremented for callback-scored duplicates. - HGraph validation tests: Error-path tests for
distance_batch_size_ == 0, non-finite scores, range search rejection, parallel search rejection, andbrute_force_thresholdrejection are now covered.
Additional improvements in this revision:
visited_list_guardRAII wrapper ensures the visited list is always returned to the pool, even on early returns.- BruteForce parallel search now collects exceptions from all futures before rethrowing, preventing abandoned tasks.
- The
start >= endguard in BruteForce parallel chunking avoids submitting empty work items.
No new issues found. The implementation is thorough with good test coverage across BruteForce, HGraph, and IVF.
LHT129
left a comment
There was a problem hiding this comment.
This latest revision (63b6e11) is a clean squashed version that incorporates all previously addressed fixes. After thorough review of the full diff (792 additions, 63 deletions across 12 files), no new issues were found.
Summary of what was verified:
- Public API (
search_request.h): Clean addition ofSearchDistanceBatchFunctype alias and two new fields with clear documentation. - BruteForce (
bruteforce.cpp): Proper batch-distance loop withflush_custom_batch, independent scalar validation for custom mode, exception-safe parallel search withstart >= endguard. - HGraph (
hgraph_search.cpp): Entry-point scoring before filter check,score_duplicateswith independent callback scoring and overflow protection,visited_list_guardRAII, disabled brute_force_threshold/parallel/reorder/rabitq for custom mode. - IVF (
ivf.cpp): Newsearch_with_custom_distancewith proper bucket routing, batch submission, timeout handling, and multi-bucket dedup viastd::unordered_map(consistent with existingIVF::searchpattern). - BasicSearcher (
basic_searcher.cpp): Pre-allocatedcustom_labelsbuffer,score_idsandscore_duplicateslambdas with proper batching and stat counting,find_duplicatedisabled for custom mode. - Tests: Comprehensive coverage across all three index types including happy path, validation errors, and unsupported configuration rejection.
- Docs: Both English and Chinese documentation updated with clear API descriptions and per-index behavior notes.
The implementation is thorough, well-tested, and ready for merge.
Signed-off-by: LHT129 <tianlan.lht@antgroup.com> Co-authored-by: opencode <opencode@anthropic.com>
63b6e11 to
4c2952d
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
IVF callback mode returns before reasoning is initialized/attached, so expected_labels_ is ignored and callback searches don’t emit reasoning traces.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
src/algorithm/ivf/ivf.cpp:1977
- In IVF callback mode, the function returns early before creating/attaching a ReasoningContext, so
expected_labels_is silently ignored even though IVF otherwise supports reasoning. Alsosearch_with_custom_distance(..., ReasoningContext*)is never given a non-nullreasoning_ctx, so bucket selection / visits aren’t recorded for callback searches.
if (use_custom_distance) {
param.search_mode = KNN_SEARCH;
param.topk = request.topk_;
auto search_result = search_with_custom_distance(query, request, param, ctx);
if (search_result == nullptr || search_result->Empty()) {
auto dataset_results = DatasetImpl::MakeEmptyDataset();
dataset_results->Statistics(stats.Dump());
return dataset_results;
}
auto dataset_results = this->pack_knn_result(search_result, ctx.alloc);
dataset_results->Statistics(stats.Dump());
return dataset_results;
}
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
LHT129
left a comment
There was a problem hiding this comment.
Overall Assessment: LGTM with minor suggestions
This PR adds support for custom batch query distance callbacks across BruteForce, HGraph, and IVF indexes. The feature is well-scoped and the implementation is clean.
What this PR does well:
- Consistent
CHECK_ARGUMENTvalidation across all three indexes for invalid parameter combinations - Proper batch scoring with configurable
distance_batch_size_to control memory/performance trade-off - Clear rejection of unsupported modes (range search, parallel search, etc.) with descriptive error messages
- Good test coverage for validation paths in all three indexes
- Documentation updates in both English and Chinese
Issues identified:
- [P3]
std::unordered_mapwithout allocator in IVFsearch_with_custom_distance— inconsistent with the rest of the function which uses allocator-aware containers - [suggestion]
scores.resize()vsreserve()inconsistency in IVF —reserve()would be more appropriate since the callback writes directly intoscores.data()
Prior review rounds: 19 of 30 existing comments remain unresolved. The author has been iterating actively. The current diff addresses several earlier concerns (allocator usage in BruteForce, first_error propagation, visited_list_guard for HGraph).
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (862 changed lines across 12 files).
Submitted 2 inline comments.
Reviewed commit 4c2952d.
|
Tick the box to add this pull request to the merge queue (same as
|
Summary
Validation
Closes #2550