feat: support pyramid raw vector retrieval - #2503
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
/label status/waiting-for-review |
|
Automated pull request review completed. Review effort: Submitted 3 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
|
There was a problem hiding this comment.
Pull request overview
Adds Pyramid support for preserving and retrieving FP32 raw vectors (including through streaming/legacy serialization), along with detailed live memory reporting to enable accurate HGraph vs Pyramid memory comparisons.
Changes:
- Implement Pyramid raw-vector storage/reuse logic and expose it via
GetVectorByInnerId/GetRawVectorByIds, including streaming + legacy serialization support. - Add Pyramid
GetMemoryUsage()/GetMemoryUsageDetail()with hierarchy graph accounting and alias-aware raw-vector reporting. - Update unit tests, docs (EN/ZH), and add a C++ example to reproduce HGraph/Pyramid memory comparisons.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/quantization/fp32_quantizer_parameter.cpp | Persist hold_molds in FP32 quantizer JSON. |
| src/quantization/fp32_quantizer_parameter_test.cpp | Extend FP32 parameter JSON roundtrip coverage for hold_molds. |
| src/algorithm/pyramid/pyramid.h | Add raw-vector members/hooks and memory usage APIs to Pyramid declarations. |
| src/algorithm/pyramid/pyramid.cpp | Implement raw-vector reuse/storage, serialization blocks, and memory usage reporting. |
| src/algorithm/pyramid/pyramid_zparameters.cpp | Backward-compat restore for legacy FP32 hold_molds and compatibility checks for raw-vector params. |
| src/algorithm/pyramid/pyramid_test.cpp | Add regression tests covering raw vectors, (de)serialization, cosine behavior, and memory reporting. |
| examples/cpp/README.md | Document new memory comparison example. |
| examples/cpp/CMakeLists.txt | Build target for the new memory comparison example. |
| examples/cpp/325_feature_compare_hgraph_pyramid_memory.cpp | New example program to compare RSS/allocator/index-reported memory. |
| docs/docs/zh/src/indexes/pyramid.md | Document new Pyramid raw-vector parameters (ZH). |
| docs/docs/zh/src/advanced/new_serialization.md | Document Pyramid streaming raw_vector block (ZH). |
| docs/docs/en/src/indexes/pyramid.md | Document new Pyramid raw-vector parameters (EN). |
| docs/docs/en/src/advanced/new_serialization.md | Document Pyramid streaming raw_vector block (EN). |
|
/retest |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/algorithm/pyramid/pyramid.cpp:978
SUPPORT_GET_RAW_VECTOR_BY_IDSis enabled wheneverraw_vector_ != nullptr, but for cosine metrics an FP32 flatten withoutHoldMolds()stores normalized vectors, soGetRawVectorByIdswould not return the original/raw vectors. This should follow the same gating used by BruteForce/HGraph: only advertise raw-vector support for cosine when molds are preserved.
if (raw_vector_ != nullptr) {
this->index_feature_list_->SetFeature(IndexFeature::SUPPORT_GET_RAW_VECTOR_BY_IDS);
}
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (531 changed lines across 10 files).
Submitted 4 inline comments.
Reviewed commit c519510.
08b4de1 to
360048a
Compare
|
/version 1.1 |
cff0f1c to
220ef23
Compare
LHT129
left a comment
There was a problem hiding this comment.
[suggestion] ExportModel does not export raw_vector_ data. When create_new_raw_vector_ is true, the exported model will lose raw vector storage, causing GetRawVectorByIds to fail on the exported index. Consider adding:
if (create_new_raw_vector_) {
this->raw_vector_->ExportModel(index->raw_vector_);
}in Pyramid::ExportModel (pyramid.cpp, around line 877).
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (811 changed lines across 8 files).
Submitted 2 inline comments.
Reviewed commit 220ef23.
|
/retest |
220ef23 to
44608a1
Compare
|
Review follow-up: I also evaluated the non-inline ExportModel suggestion and intentionally made no change. Pyramid::ExportModel exports reusable quantizer/model state, not indexed vector data. A dedicated Pyramid raw cell is constrained to FP32, which has no trained model state to transfer; the target is already constructed with an empty raw cell from the same parameters. Copying raw vectors would violate the existing empty-model contract rather than fix retrieval. |
Signed-off-by: jac0626 <jac0626@users.noreply.github.com> Assisted-by: Codex:gpt-5
Signed-off-by: jac0626 <jac0626@users.noreply.github.com> Assisted-by: Codex:gpt-5
Signed-off-by: jac0626 <jac0626@users.noreply.github.com> Assisted-by: Codex:gpt-5
Signed-off-by: jc543239 <jc543239@antgroup.com> Assisted-by: Codex:gpt-5
Signed-off-by: jac0626 <jac0626@users.noreply.github.com> Assisted-by: Codex:gpt-5
44608a1 to
d8dc386
Compare
| std::unordered_map<std::string, uint64_t> | ||
| Pyramid::GetMemoryUsageDetail() const { | ||
| std::lock_guard count_lock(cur_element_count_mutex_); | ||
| std::unique_lock resize_lock(resize_mutex_); |
There was a problem hiding this comment.
[suggestion] GetMemoryUsageDetail() acquires unique_lock on resize_mutex_, which blocks concurrent search, GetVectorByInnerId, and distance-by-id calls. Since this method only reads state, a shared_lock would be sufficient and would avoid serializing all read operations against memory-usage queries.
Consider changing std::unique_lock resize_lock(resize_mutex_) to std::shared_lock resize_lock(resize_mutex_).
| for (const auto& [key, child] : children_) { | ||
| memory += key.capacity() + 1; | ||
| memory += child->GetMemoryUsage(); | ||
| } |
There was a problem hiding this comment.
[suggestion] IndexNode::GetMemoryUsage() multiplies children_.bucket_count() by value_type size, which overestimates memory because bucket_count() is the hash-table bucket count, not the number of stored entries. Use children_.size() instead to get the actual number of child nodes.
| "{RAW_VECTOR_KEY}": { | ||
| "{IO_PARAMS_KEY}": { | ||
| "{TYPE_KEY}": "{IO_TYPE_VALUE_BLOCK_MEMORY_IO}", | ||
| "{IO_FILE_PATH_KEY}": "{DEFAULT_FILE_PATH_VALUE}" |
There was a problem hiding this comment.
[suggestion] The raw_vector cell defaults to the same ./default_file_path used by base and precise codes. With a documented configuration such as SQ8 base_io_type=buffer_io and raw_vector_io_type=buffer_io, both cells would open the same file path, leading to corruption. Consider using a distinct default path (e.g. ./default_raw_vector_path) or requiring an explicit path when the raw-vector backend is file-backed.
LHT129
left a comment
There was a problem hiding this comment.
Additional observations on the latest push.
|
|
||
| void | ||
| Pyramid::check_and_init_raw_vector(const FlattenInterfaceParamPtr& raw_vector_param, | ||
| const IndexCommonParam& common_param) { |
There was a problem hiding this comment.
[suggestion] check_and_init_raw_vector checks raw_vector_param->io_parameter->GetTypeName() to decide whether to attempt aliasing, but the aliasing decision should be based on the candidate cell's IO type (i.e. whether base_codes_ or precise_codes_ is in-memory FP32), not the requested raw-vector backend. If the user requests raw_vector_io_type=buffer_io but base_codes_ happens to be in-memory FP32, aliasing is still desirable and safe.
Consider calling find_raw_vector_source(true) unconditionally (regardless of raw_vector_param IO type) and only falling back to MakeInstance when no alias is found.
| auto io_type_name = raw_vector_param->io_parameter->GetTypeName(); | ||
| if (io_type_name == IO_TYPE_VALUE_BLOCK_MEMORY_IO || io_type_name == IO_TYPE_VALUE_MEMORY_IO) { | ||
| raw_vector_ = find_raw_vector_source(true); | ||
| } |
There was a problem hiding this comment.
[note] check_and_init_raw_vector sets has_raw_vector_ = raw_vector_ != nullptr at line 1546, but raw_vector_ is guaranteed non-null at this point (either from find_raw_vector_source or MakeInstance). The null check is redundant; has_raw_vector_ will always be true when this code is reached. Consider setting has_raw_vector_ = true directly.
LHT129
left a comment
There was a problem hiding this comment.
Code Review for PR #2503
[critical] Missing has_raw_vector_ declaration in pyramid.h
has_raw_vector_ is assigned in check_and_init_raw_vector() (pyramid.cpp line ~1558) and read in InitFeatures() (pyramid.cpp line ~999), but this member variable is never declared in pyramid.h. The header only declares create_new_raw_vector_. This will cause a compilation error.
Fix: Add bool has_raw_vector_{false}; to the member variables section of pyramid.h, alongside create_new_raw_vector_:
bool create_new_raw_vector_{false}; // whether raw_vector_ owns separate storage
bool has_raw_vector_{false}; // whether raw vector retrieval is available[suggestion] Unnecessary FlattenInterface allocation in check_and_init_raw_vector
In check_and_init_raw_vector(), FlattenInterface::MakeInstance(raw_vector_param, common_param) is always called first (line ~1543), even when raw_vector_ will ultimately alias base_codes_ or precise_codes_ via find_raw_vector_source(). When aliasing succeeds, the newly constructed instance is leaked (overwritten by the aliased pointer). Consider checking for an aliasable source first, and only constructing a new instance when necessary.
[note] CalDistanceById precedence order
Both CalcDistanceById overloads override flat with raw_vector_ when raw_vector_ != nullptr && calculate_precise_distance. This means raw_vector_ takes precedence over precise_codes_ which takes precedence over base_codes_. This is correct behavior (raw vectors are always the most precise), but the precedence chain is implicit. A brief comment noting the priority would help future readers.
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (855 changed lines across 9 files).
Submitted 3 inline comments.
Reviewed commit d8dc386.
| "{RAW_VECTOR_KEY}": { | ||
| "{IO_PARAMS_KEY}": { | ||
| "{TYPE_KEY}": "{IO_TYPE_VALUE_BLOCK_MEMORY_IO}", | ||
| "{IO_FILE_PATH_KEY}": "{DEFAULT_FILE_PATH_VALUE}" |
There was a problem hiding this comment.
[P1] Give raw storage a cell-specific backing file
This uses the same DEFAULT_FILE_PATH_VALUE as the base and precise cells. With accepted settings such as SQ8 base_io_type=buffer_io plus raw_vector_io_type=buffer_io and omitted paths, both cells truncate and write the same file from offset zero; FP32 raw writes then overwrite the quantized search codes, silently corrupting the index. Reject shared file paths or derive a raw-specific default.
| precise_codes_->InsertVector(data_vectors + dim_ * i, inner_id); | ||
| } | ||
| if (create_new_raw_vector_) { | ||
| raw_vector_->InsertVector(data_vectors + dim_ * i, inner_id); |
There was a problem hiding this comment.
[P2] Roll back counts when the raw-vector write fails
FlattenDataCell::InsertVector advances its count before performing the IO write, and base/precise cells have already advanced by this line. If the separate raw backend fails here, no label, logical count, or hierarchy node is published, but GetNumElements() still observes the advanced base count; in a multi-vector Add, retrying can reuse inner ID 0 and overwrite an earlier published vector because cur_element_count_ was never updated. Make the cell writes transactional or explicitly restore every affected count/state on failure.
| if (create_new_raw_vector_) { | ||
| raw_vector_->InsertVector(data_vectors + dim_ * i, inner_id); | ||
| } | ||
| label_table_->Insert(inner_id, data_ids[i]); |
There was a problem hiding this comment.
[P2] Synchronize label publication with raw-vector lookup
This mutates LabelTable while holding only cur_element_count_mutex_, whereas the newly exposed GetRawVectorByIds path delegates to GetVectorByIds, which calls GetIdByLabel without that mutex or label_lookup_mutex_. A raw lookup concurrent with Add can therefore race the label vector/hash map and invoke undefined behavior even though the vector bytes are written first. Publish and resolve labels under the same lock.
Change Type
Linked Issue
What Changed
store_raw_vectorsupport and expose raw vectors throughGetRawVectorByIds/GetVectorByInnerId.GetMemoryUsage()/GetMemoryUsageDetail()reporting, including hierarchy graphs and separate raw-vector storage without double-counting aliases.hold_molds, validate legacy deserialization parameters before reading the body, and preserve backward compatibility for older Pyramid parameter snapshots.Test Evidence
make fmtmake lintmake testmake cov, run tests, and collect coverageTest details:
Compatibility Impact
store_raw_vector=true; FP32 parameter snapshots now retainhold_moldsPerformance and Concurrency Impact
Documentation Impact
README.mdDEVELOPMENT.mdCONTRIBUTING.mdRisk and Rollback
Checklist
kind/bugandkind/feature; see "Linked Issue" above)[skip ci]prefix)