Repository navigation
Conversation
Encode each PiPNN representative once and bind duplicate logical IDs to its physical slot. Initialize reorder and raw-vector readers after slot wrapping. Cover quantizers, serialization, filtering and incremental Add; document the supported combination. Signed-off-by: LightWant <2510035537@qq.com> Assisted-by: Codex:GPT-6
Merge Protections🟢 All 2 merge protections satisfied — ready to merge. Show 2 satisfied protections🟢 Require kind label
🟢 Require version label
|
LHT129
left a comment
There was a problem hiding this comment.
Review Summary
This PR introduces shared vector storage for PiPNN duplicates, reusing the existing CodeSlotMap deduplication infrastructure that was previously limited to NSW graphs. The core idea is sound and the implementation is well-structured.
What's good
- Clean reuse of
CodeSlotMap/MakeCodeSlotFlattenAdapterinfrastructure materialize_pipnn_storagecorrectly handles the representative/non-representative slot mapping- Parallel encoding with proper exception aggregation
- Comprehensive new test file (
hgraph_pipnn_storage_test.cpp) covering quantizers, filtering, incremental Add, and edge cases - Documentation updated in both English and Chinese
Items to consider
- [suggestion]
check_and_init_raw_vectoris called withis_create_new=falsein the constructor, but the method name suggests it's for initialization of a new raw vector. Consider a more descriptive name or a comment explaining the deserialization path. - [suggestion] The ordering dependency between slot wrapping and
init_resize_bit_and_reorder()is implicit. A brief comment would help future readers. - [suggestion] If encoding fails in
materialize_pipnn_storage, slots allocated viaAllocateSlot()are leaked. Consider cleanup on the failure path. - [note] The "Drain every task" comment in
materialize_pipnn_storageis slightly misleading — the code drains futures after encoding, not during scheduling. Consider rewording. - [note] The lambda in
materialize_pipnn_storagecapturesdataby reference. This is safe today becausedataoutlives the futures, but a comment would help. - [note] The
REQUIRE_NOTHROWtest change validates construction no longer throws, but doesn't verify the index is functional afterward.
Verdict
The changes are correct and well-tested. The suggestions above are non-blocking improvements. No critical issues found.
Use provisional physical positions until all workers finish successfully, retain the dataset in each task, and verify that reorder sources are wrapped. Cover encoding failures with one and four build threads. Signed-off-by: LightWant <2510035537@qq.com> Assisted-by: Codex:GPT-6
LHT129
left a comment
There was a problem hiding this comment.
Thanks for the update. The second commit (55884fda) cleanly addresses the prior review feedback:
- Slot allocation: Provisional positions before encoding,
AllocateSlotonly on success — no more slot leak on failure. - Dataset capture: Lambda now captures
databy value (shared_ptr), making each task self-contained. - Reorder safety: The new runtime check in
init_resize_bit_and_reorder()catches unwrapped reorder codes early, which is a good defensive measure. - Failure test: The
FailingStorageFlatten+HGraphPiPNNStorageTestAccesstest verifies that slots are not consumed on encoding failure across both 1 and 4 threads.
The overall approach — encoding each PiPNN representative once and binding all logical IDs to its physical slot — is sound. The deferred publication of slot mappings until all encoding workers complete is the correct ordering.
Left one minor [suggestion] about the &representatives reference capture in the worker lambdas — safe as written given the drain loop, but worth a comment for future readers.
LHT129
left a comment
There was a problem hiding this comment.
Review Summary (55884fd)
This follow-up commit addresses all five items raised in the previous review round:
- Comment wording — Reworded the "Drain every task" comment for clarity.
- Lambda capture — Changed
datacapture from reference to by-value (DatasetPtrcopy), making the worker lambda self-contained. check_and_init_raw_vectornaming — Added an inline comment explaining theis_create_new=falsedeserialization path.- Ordering dependency — Added a runtime invariant check in
init_resize_bit_and_reorder()that verifies reorder codes are wrapped when dedup storage is enabled. - Slot leak on failure — Redesigned
materialize_pipnn_storageto use provisional physical slots (first_slot + i) for encoding, withAllocateSlot()+PublishSlot()deferred until after all encoding tasks succeed. If any task fails, the provisional slots are never published and the physical storage is effectively reclaimed on the next Build.
The implementation is correct and the test coverage is thorough. No new issues found.
LHT129
left a comment
There was a problem hiding this comment.
This is a well-structured PR that enables deduplicate_storage for PiPNN graph type. The implementation is careful about correctness — encoding into provisional slots before publishing mappings, draining all workers before any slot allocation, and ordering init_resize_bit_and_reorder after the slot adapter wrapping.
Summary of what was reviewed and addressed:
-
Slot lifecycle safety — The initial approach of
AllocateSlotbefore encoding has been replaced with provisional physical positions, so a failed encode task does not consume or leak slot IDs. The injected-failure regression test confirms this. -
Lambda capture hygiene — Worker lambdas now capture
DatasetPtrby value, matching the codebase convention. Therepresentativesreference capture is safe because all futures are drained before the function returns. -
Reorder initialization ordering —
init_resize_bit_and_reorder()now runs afterMakeCodeSlotFlattenAdapterwrapping, with a runtime invariant check that dedup storage requires wrapped reorder codes. -
Raw vector rebinding — The constructor correctly rebinds an existing FP32 raw alias to the wrapped base/reorder codes when dedup storage is enabled.
-
Test coverage — The new
hgraph_pipnn_storage_test.cppcovers quantizers (fp32, sq8+reorder, rabitq), serialization round trips, adaptive alpha, filtering, extra info, concurrent Add, and failure injection. Existing add tests are extended to coverpipnngraph type. -
Documentation — Both English and Chinese docs are updated to reflect the new PiPNN support for deduplicated storage, including byte-exact grouping semantics for Build and the existing threshold/code-based semantics for Add.
No blocking issues remain. The PR description is thorough and the compatibility/risk analysis is honest about what is and is not supported.
Change Type
What Changed
HGraph previously rejected
graph_type: "pipnn"withdeduplicate_storage: true. PiPNN builds can now encode each duplicate-group representative once and bind every logical ID to its physical code slot, retaining independent labels, attributes and extra information.Related storage infrastructure: #2244. This PR does not change PiPNN candidate generation or alpha pruning.
Test Evidence
Release selected suites: 200/201 unit cases and 125/128 functional cases passed. The three distinct failures concern NaN/Inf handling:
HGraph dynamic session rechecks mutable filters and thresholds without traversaland the twoHGraph returns non-finite duplicate labels ... within efcases. Rebuilding with both changed production implementations restored to main commitd9768883reproduced the same failures; the only remaining production difference was an unused method declaration. All three pass in Debug. Release production and test compilation use-Ofast.The final implementation was restored, rebuilt, and the focused suite passed again. Daily stress tests and whole-library coverage collection were not run.
Compatibility Impact
The existing flags enable the combination; defaults and public APIs remain unchanged. Current serialization supports shared storage. Force removal, cache-assisted build, Merge and v0.14 serialization remain unsupported with storage deduplication. Updating a vector shared by multiple labels remains unsupported.
Performance and Concurrency Impact
Controlled GIST-derived dataset: 100k byte-distinct 960D vectors repeated ten times under 1M independent labels (90% exact duplicates), FP32/L2, degree 32, alpha 1.2, adaptive step 0.06. Build uses 16 threads; search uses one thread. Both sides enable graph duplicate grouping. Each run serializes/reloads all 1M labels before querying.
Recall@10 is 100% at ef_search ≥ 64 in all four runs. Measurements are single runs with 200 queries and are not results on the original GIST1M distribution; QPS regresses by 1.8–4.2% at ef=64. The main gains are storage size and build time.
Physical capacity is reserved before parallel encoding, all workers finish before slot mappings are published, and existing Add synchronization is retained.
Documentation Impact
Risk and Rollback
Storage mapping and reader aliasing are the primary risks, covered across base/reorder/raw storage and round trips. Revert the storage integration and follow-up commits, then rebuild with storage deduplication disabled to roll back.
Checklist