feat: expose per-search distance evaluation statistics - #2545
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
|
b503d2f to
cfca2cb
Compare
cfca2cb to
2be5988
Compare
2be5988 to
c2020bb
Compare
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (520 changed lines across 20 files).
Submitted 8 inline comments.
Reviewed commit c2020bb.
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (673 changed lines across 30 files).
Submitted 8 inline comments.
Reviewed commit f1ec40e.
f1ec40e to
a4bbf90
Compare
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
Commit a4bbf900 addresses the issues flagged in the previous review round:
✅ P1 & P2 Issues Resolved
- P1 (
attach_statisticsreadingother_resultfrom legacy callers): Fixed by introducing astatistics_requestsset keyed bySearchResult_t*, eliminating the dependency onother_result. - P2 (Sparse term counting sums posting-list lengths): Fixed by using
UnorderedSet<uint16_t>to count unique document IDs inSparseTermDataCell::Query(). - P2 (
make_empty_search_resultonly handles BruteForce): Fixed by expanding to a switch covering all index types.
Observations
sparse_dmq_datacell.h:backend_is hardcoded toSPARSE_SQ8while all other datacells derive it from the quantizer name viaQuantizerDistanceBackendorBackendFromName. This is acceptable sinceSparseDMQDataCellis a concrete class with a fixed quantizer, but worth noting for consistency.- The
BackendFromNamefunction ordering correctly handles substring overlaps (e.g., "fp16" before "fp32", "bf16" before "fp16", "sq8_uniform" before "sq8", "pq_fastscan" before "pq").
Overall
The implementation is clean, well-tested, and the critical issues from the previous review have been properly addressed. No new blocking issues found.
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (788 changed lines across 36 files).
Submitted 4 inline comments.
Reviewed commit a4bbf90.
a4bbf90 to
fdd1a2b
Compare
fdd1a2b to
7f7dd05
Compare
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
I have thoroughly reviewed all 30 changed files in this PR. The implementation adds per-search distance evaluation statistics across all maintained index types (HGraph, BruteForce, IVF, Pyramid, SINDI, SIMQ) with JSON output, phase/backend breakdowns, and C/Python API accessors.
Prior Review Rounds
All issues identified in previous review rounds by LHT129 and vsag-bot have been properly addressed:
- SaturatingAdd logic: Fixed to report saturation only after successful CAS with simplified overflow condition.
- BackendName hot-path allocation: Replaced with enum-based
DistanceEvaluationBackendcached at data cell construction time. - SINDI backend selection: Extracted into a clean
switch-basedsparse_backend()helper. - GNO-IMI routing counter: Instrumented the actual
ClassifyDatasForSearchpath. - PQ FastScan classification:
pqfsquantizer name now recognized before genericpqbranch. - Route-only IVF counting: Now counts each valid returned centroid distance evaluation.
- Empty BruteForce statistics: Zero statistics contract added to all empty result paths.
- C API opt-in lifecycle: Statistics are explicitly opt-in via
enable_statistics/get_statistics/destroy_statistics, tracked externally without reading uninitializedother_result. - SINDI sparse complexity: Replaced full-window sweep with
SparseEvaluationTrackerthat counts unique candidate IDs via generation-based deduplication. - Missing datacell counters: Added
AddDistancecalls to RaBitQSplitDataCell, SparseVectorDataCell, SparseDmqDataCell, and MultiVectorDataCell Query overrides. - BruteForce parallel atomics: Worker threads set
local_query_context.stats = nullptrto avoid per-candidate CAS contention, aggregating once per thread after the loop. - Empty index contract:
make_empty_search_resultnow covers all six index types (HGraph, IVF, Pyramid, BruteForce, SINDI, SIMQ).
Current Assessment
The code is well-structured and consistent. Key design decisions are sound:
- Phase tracking:
QueryContext::distance_phaseflows through the search pipeline, withflatten_reorder.cpptemporarily switching toRERANKand restoring the previous phase. - Backend caching:
FlattenInterface::backend_is set once at construction viaBackendFromName(quantizer->Name()), avoiding string lookups in hot paths. - Thread safety: All
SearchStatisticscounters usestd::atomicwithmemory_order_relaxed;AddDistanceuses CAS loops with saturating arithmetic. - Test coverage: Unit tests cover distance contract, unknown backend incompleteness, saturation, backend name classification, empty result contract, and C API statistics lifecycle.
No new issues found. The implementation is complete and correct.
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary — PR #2545
This PR adds per-search distance evaluation statistics across all index algorithms (HGraph, BruteForce, IVF, Pyramid, SINDI, SIMQ), with JSON output, phase/backend breakdowns, and C/Python API accessors. The current push (commit fdd1a2b) addresses all issues raised in prior review rounds by LHT129 and vsag-bot.
Prior Issues — All Resolved ✓
| # | Issue | Status |
|---|---|---|
| 1 | WORKBOARD.md committed | Removed |
| 2 | SaturatingAdd return value subtlety | Fixed — returns void, caller reads counter directly |
| 3 | BackendName called in hot path | Fixed — backend_ cached at construction in FlattenDataCell, BucketDataCell, etc. |
| 4 | Sparse backend ternary chain readability | Fixed — extracted sparse_backend() helper with switch |
| 5 | GNO-IMI search routing never reaches counter | Fixed — inner_joint_classify_datas now adds routing statistics |
| 6 | PQ FastScan classified as ordinary PQ | Fixed — PQ_FASTSCAN enum value added |
| 7 | Route-only IVF omits centroid distance evaluations | Fixed — route-only path counts centroid evaluations in routing phase |
| 8 | Empty BruteForce searches return no statistics | Fixed — make_empty_search_result covers all 6 index types |
| 9 | FlattenDataCell backend_ not set | Fixed — set in constructor via SearchStatistics::BackendFromName |
| 10 | SparseEvaluationTracker generation overflow | Fixed — BeginWindow() increments generation with uint16_t wrap |
| 11 | SparseTermDataCell query not using tracker | Fixed — uses evaluation_tracker.Mark() and returns evaluation_tracker.Count() |
| 12 | SINDI mutable path missing stats | Fixed — both mutable and immutable paths instrumented |
Overall Assessment
- Correctness: The statistics pipeline is well-designed with proper opt-in semantics, thread-safe CAS-based counting, and consistent phase/backend tracking across all index types.
- Performance: Backend names are cached at construction time to avoid hot-path string allocation. Per-thread local aggregation in BruteForce avoids CAS contention.
- Test coverage: New unit tests cover distance contract, saturation, backend name classification, SparseEvaluationTracker, C API lifecycle, and empty result statistics.
- Maintainability: Clean separation between QueryContext (pipeline state) and SearchStatistics (aggregation). JSON output format is extensible.
Verdict: Approve. All prior concerns have been addressed. No new issues found.
LHT129
left a comment
There was a problem hiding this comment.
Test review from LHT129 via API
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary — PR #2545
This PR adds per-search distance evaluation statistics across all index algorithms (HGraph, BruteForce, IVF, Pyramid, SINDI, SIMQ), with JSON output, phase/backend breakdowns, and C/Python API accessors. The current push (commit fdd1a2b) addresses all issues raised in prior review rounds by LHT129 and vsag-bot.
Prior Issues — All Resolved ✓
| # | Issue | Status |
|---|---|---|
| 1 | WORKBOARD.md committed | Removed |
| 2 | SaturatingAdd return value subtlety | Fixed — returns void, caller reads counter directly |
| 3 | BackendName called in hot path | Fixed — backend_ cached at construction in FlattenDataCell, BucketDataCell, etc. |
| 4 | Sparse backend ternary chain readability | Fixed — extracted sparse_backend() helper with switch |
| 5 | GNO-IMI search routing never reaches counter | Fixed — inner_joint_classify_datas now adds routing statistics |
| 6 | PQ FastScan classified as ordinary PQ | Fixed — PQ_FASTSCAN enum value added |
| 7 | Route-only IVF omits centroid distance evaluations | Fixed — route-only path counts centroid evaluations in routing phase |
| 8 | Empty BruteForce searches return no statistics | Fixed — make_empty_search_result covers all 6 index types |
| 9 | FlattenDataCell backend_ not set | Fixed — set in constructor via SearchStatistics::BackendFromName |
| 10 | SparseEvaluationTracker generation overflow | Fixed — BeginWindow() increments generation with uint16_t wrap |
| 11 | SparseTermDataCell query not using tracker | Fixed — uses evaluation_tracker.Mark() and returns evaluation_tracker.Count() |
| 12 | SINDI mutable path missing stats | Fixed — both mutable and immutable paths instrumented |
Overall Assessment
- Correctness: The statistics pipeline is well-designed with proper opt-in semantics, thread-safe CAS-based counting, and consistent phase/backend tracking across all index types.
- Performance: Backend names are cached at construction time to avoid hot-path string allocation. Per-thread local aggregation in BruteForce avoids CAS contention.
- Test coverage: New unit tests cover distance contract, saturation, backend name classification, SparseEvaluationTracker, C API lifecycle, and empty result statistics.
- Maintainability: Clean separation between QueryContext (pipeline state) and SearchStatistics (aggregation). JSON output format is extensible.
Verdict: Approve. All prior concerns have been addressed. No new issues found.
Signed-off-by: Xiangyu Wang <wxy407827@antgroup.com> Assisted-by: OpenAI Codex:gpt-5
7f7dd05 to
8467218
Compare
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (925 changed lines across 41 files).
Submitted 2 inline comments.
Reviewed commit 8467218.
| uint64_t coarse_probe_count = 0; | ||
| auto coarse_results = coarse_search( | ||
| query_mvs[0].vectors_, query_mvs[0].len_, coarse_k, &coarse_dist_cmp, &coarse_probe_count); | ||
| stats.AddDistance(SearchStatistics::DistancePhase::ROUTING, "fp32", coarse_dist_cmp); |
There was a problem hiding this comment.
[P2] Use nested HGraph distance evaluations for SIMQ routing
coarse_dist_cmp is populated from the nested HGraph's legacy dist_cmp, but that counter increments only for visited neighbors and omits the entry-point distance computed at the start of every graph traversal. For example, a one-centroid HGraph can evaluate its entry point while returning dist_cmp == 0, so SIMQ reports zero routing evaluations despite doing one. Preserve the legacy SIMQ field, but aggregate the nested result's new distance_evaluations value into this contract.
| return make_empty_dataset_with_stats(); | ||
| } | ||
| if (iter_filter_ctx->IsFirstUsed()) { | ||
| ctx.distance_phase = DistanceEvaluationPhase::ROUTING; |
There was a problem hiding this comment.
[P2] Preserve statistics for empty iterator results
In this iterator-search path, a reject-all filter can still perform routing/base distance evaluations but leave search_result empty. The function then returns DatasetImpl::MakeEmptyDataset() at the earlier empty-result branch before the statistics attachment, so the result has an empty statistics string and loses all counts produced under this phase. Attach stats.Dump() to that empty result just as the non-iterator path does.
| case IndexType::SINDI: | ||
| case IndexType::SIMQ: { | ||
| SearchStatistics statistics; | ||
| result->Statistics(statistics.Dump()); |
There was a problem hiding this comment.
[note] Pyramid is listed in the documentation and make_empty_search_result() as a maintained index with statistics support, but no search-path instrumentation exists for Pyramid (unlike HGraph, BruteForce, IVF, SINDI, and SIMQ which all have explicit AddDistance calls in their search implementations). Non-empty Pyramid searches will return an empty statistics string rather than the documented contract. Either add instrumentation to the Pyramid search path or remove Pyramid from the documented supported list.
Implements the maintained-v1 per-search logical distance-evaluation statistics contract for HGraph, BruteForce, IVF, Pyramid, SINDI, and SIMQ.
Adds additive C++ JSON statistics with phase/backend breakdowns, completeness, saturating uint64 aggregation, Python opt-in results, and C opaque statistics accessors. Legacy HNSW and DiskANN are explicit non-goals.
Fixes: #2544