feat: expose split RaBitQ parameters for Pyramid - #2457
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.
Code Review
This pull request extends the RaBitQ x+y split storage and search mode to the Pyramid index, which was previously exclusive to HGraph. It includes comprehensive documentation updates in both English and Chinese, maps the new split parameters from external configurations, updates the Pyramid index logic to handle base-reordering, and adds robust unit tests. The review feedback suggests improving consistency by using the public constant RABITQ_ERROR_RATE for parsing search parameters, and enhancing the map_rabitq_split_param helper function with type safety checks, query bit validation, and cleaner range checks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
6e9a5e9 to
baadeeb
Compare
baadeeb to
321a01c
Compare
Signed-off-by: LightWant <2510035537@qq.com> Assisted-by: Codex:gpt-5
321a01c to
0ed782c
Compare
Signed-off-by: LightWant <2510035537@qq.com> Assisted-by: Codex:gpt-5
|
Automated pull request review completed. Review effort: Submitted 4 inline comments. |
Signed-off-by: LightWant <2510035537@qq.com> Assisted-by: Codex:gpt-5
Signed-off-by: LightWant <2510035537@qq.com> Assisted-by: Codex:gpt-5
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
This PR exposes split RaBitQ parameters for Pyramid, adding x+y split quantization mode support (previously only available in HGraph). The implementation is solid overall with good test coverage, but there are several issues that need attention.
What this PR does well
- Clean separation of the
map_rabitq_split_param()helper in an anonymous namespace - Thorough input validation with clear error messages for all split parameters
- Good test coverage including happy paths, error paths, and compatibility checks
- Proper handling of the
rabitq_one_bit_searchdefault viadefault_rabitq_one_bit_search_ - The ODescent SQ8 flatten optimization is a smart performance improvement for graph construction
Issues found
[critical] get_distance reads build_flatten_interface_ without synchronization
In odescent_graph_builder.h:122-128, the get_distance method reads build_flatten_interface_ (a raw pointer) while prepare_build_flatten() writes to it from a single thread. However, get_distance is called from parallelized tasks (parallelize_task). If prepare_build_flatten() runs concurrently with distance computations, this is a data race. While the current call site in Build() calls prepare_build_flatten() before parallelization, the design is fragile — a future refactor could introduce a race. Consider making build_flatten_interface_ an std::atomic<FlattenInterface*> or documenting the strict ordering requirement.
[suggestion] rabitq_error_rate default is quiet_NaN — fragile sentinel
In pyramid_zparameters.h:105, rabitq_error_rate defaults to quiet_NaN(). This is used as a sentinel to mean "not set". However, NaN has unusual comparison semantics (NaN != NaN is true, NaN < 0.0 is false), which makes it easy to misuse. Consider using std::optional<float> or a separate has_rabitq_error_rate boolean (matching the pattern already used for has_rabitq_one_bit_search).
[suggestion] prepare_build_flatten silently ignores non-RaBitQ quantizers
In odescent_graph_builder.cpp:76, the early return checks flatten_interface_->GetQuantizerName() != QUANTIZATION_TYPE_VALUE_RABITQ. This means if someone uses SQ8 or FP16 quantizers, the optimization is silently skipped. Consider adding a debug log message when the optimization is skipped to aid troubleshooting.
[note] Missing rabitq_error_rate in CheckAndMappingExternalParam mapping table
The rabitq_error_rate search parameter is parsed in PyramidSearchParameters::FromJson() but is not part of the external_mapping table in CheckAndMappingExternalParam(). This is actually correct since it is a search-time parameter, not a build-time parameter. However, the naming could be confusing — consider adding a comment to clarify the distinction.
[note] Test file pyramid_zparameters_test.cpp uses nlohmann::json directly
The test file mixes vsag::JsonType and raw nlohmann::json in several places (e.g., line 152). While this works, it is inconsistent with the rest of the codebase which exclusively uses vsag::JsonType. Consider using vsag::JsonType consistently for maintainability.
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
This is a follow-up review focusing on issues not yet covered by the existing 24+ inline comments.
Issues already well-covered by prior reviews (no need to repeat):
- P1: Split NSW null dereference in
add_one_point(vsag-bot) — appears to be fixed in the current diff - P2: Per-query error rate not consumed in search path (vsag-bot, LHT129)
- P2: SQ8 substitute scope too broad — should be restricted to split storage (vsag-bot, LHT129)
- P2: Redundant SQ8 instances per hierarchy (vsag-bot, LHT129)
- Data race in
get_distance()readingbuild_flatten_interface_(LHT129) - NaN sentinel for
rabitq_error_rate(LHT129) - Silent skip in
prepare_build_flatten(LHT129) reorder_sourceoverwrite inmap_rabitq_split_param(LHT129)- Undocumented
rabitq_one_bit_searchparameter (LHT129) - Internal vs public constant inconsistency (gemini-code-assist)
New findings from this review:
[suggestion] Test relaxation in pyramid_test.cpp masks potential correctness regressions
The assertion at the end of "Pyramid promotes flat node at index minimum size" changed from exact ID match to set membership check. The old test verified that querying with vector i returns itself as the nearest neighbor. The new assertion only checks that the result ID is one of 3 valid dataset IDs — it would pass even if the search returns a completely wrong vector. If the relaxation is intentional because RaBitQ quantization makes exact self-match unreliable, consider adding a comment explaining why, or using a weaker but still meaningful check.
[note] rabitq_error_rate validation uses identical error messages for both checks
A negative value like -1.0 passes std::isfinite but fails > 0.0F, yet both produce "must be finite and positive". Consider differentiating the messages or combining into a single check.
Overall assessment:
The PR is structurally sound — the has_precise_reorder() / get_reorder_codes() abstraction cleanly separates split mode from traditional reorder, and the map_rabitq_split_param() helper correctly validates and configures split parameters. The main areas needing attention are the P2 issues around search-time lower-bound propagation and SQ8 scope restriction, which affect correctness and performance rather than crashes.
Signed-off-by: LightWant <2510035537@qq.com> Assisted-by: Codex:gpt-5
LHT129
left a comment
There was a problem hiding this comment.
Code Review — commit 3c3a50f9 (refactor: keep reorder source internal)
This commit removes PYRAMID_REORDER_SOURCE from the public Pyramid parameter API, making reorder_source an internal-only setting that is exclusively controlled by map_rabitq_split_param(). This is the right direction — it prevents users from setting conflicting reorder_source values alongside split parameters.
Status of previously flagged issues
| Issue | Severity | Status |
|---|---|---|
NSW null dereference in add_one_point |
P1 | Fixed — now uses has_precise_reorder() |
get_distance() data race on build_flatten_interface_ |
critical | Addressed — documented ordering guarantee at member declaration |
rabitq_error_rate internal key → public constant |
suggestion | Fixed — now uses RABITQ_ERROR_RATE |
Per-query rabitq_error_rate not consumed by search path |
P2 | Open — DistanceRecordVector still not threaded through search_node/reorder_ |
| SQ8 substitute activates for all RaBitQ, not just split | P2 | Open — prepare_build_flatten gates on quantizer name only |
| Redundant SQ8 per hierarchy in multi-hierarchy builds | P2 | Open — each ODescent instance independently creates SQ8 store |
NaN sentinel for rabitq_error_rate |
suggestion | Deferred by author |
Silent skip logging in prepare_build_flatten |
suggestion | Deferred by author |
New observation on this commit
The removal of PYRAMID_REORDER_SOURCE from the external mapping table (line 1042) means the reorder_source → REORDER_SOURCE_KEY mapping entry is deleted, but map_rabitq_split_param() still writes REORDER_SOURCE_KEY into inner_json unconditionally (line 104). This is correct — the internal JSON is consumed by PyramidParameters::FromJson() which reads reorder_source as an internal field. The pyramid_zparameters_test.cpp tests at lines 477 and 536 verify both "base" and "precise" values are correctly parsed from the internal JSON.
Summary
The refactoring is clean and correct. The three remaining P2 issues (error rate propagation, SQ8 gating, redundant SQ8 instances) are non-blocking for merge but should be tracked for follow-up. The P1 and critical issues from earlier reviews have all been resolved.
LHT129
left a comment
There was a problem hiding this comment.
Code Review — commit 3c3a50f9
This commit removes PYRAMID_REORDER_SOURCE from the public Pyramid parameter API, making reorder_source an internal-only setting controlled exclusively by map_rabitq_split_param(). This correctly prevents users from setting conflicting reorder_source values alongside split parameters.
Status of previously flagged issues
| Issue | Severity | Status |
|---|---|---|
NSW null dereference in add_one_point |
P1 | Fixed — now uses has_precise_reorder() |
get_distance() data race on build_flatten_interface_ |
critical | Addressed — documented ordering guarantee |
rabitq_error_rate internal key → public constant |
suggestion | Fixed — now uses RABITQ_ERROR_RATE |
Per-query rabitq_error_rate not consumed by search path |
P2 | Open — DistanceRecordVector not threaded through search_node/reorder_ |
| SQ8 substitute activates for all RaBitQ, not just split | P2 | Open — gates on quantizer name only |
| Redundant SQ8 per hierarchy in multi-hierarchy builds | P2 | Open — each ODescent independently creates SQ8 |
NaN sentinel for rabitq_error_rate |
suggestion | Deferred by author |
Silent skip logging in prepare_build_flatten |
suggestion | Deferred by author |
Summary
The refactoring is clean and correct. The three remaining P2 issues are non-blocking for merge but should be tracked for follow-up. All P1 and critical issues from earlier reviews have been resolved.
LHT129
left a comment
There was a problem hiding this comment.
Code Review — commit 3c3a50f9
This commit removes PYRAMID_REORDER_SOURCE from the public Pyramid parameter API, making reorder_source an internal-only setting controlled exclusively by map_rabitq_split_param(). This correctly prevents users from setting conflicting reorder_source values alongside split parameters.
Status of previously flagged issues
| Issue | Severity | Status |
|---|---|---|
NSW null dereference in add_one_point |
P1 | Fixed — now uses has_precise_reorder() |
get_distance() data race on build_flatten_interface_ |
critical | Addressed — documented ordering guarantee |
rabitq_error_rate internal key → public constant |
suggestion | Fixed — now uses RABITQ_ERROR_RATE |
Per-query rabitq_error_rate not consumed by search path |
P2 | Open — DistanceRecordVector not threaded through search_node/reorder_ |
| SQ8 substitute activates for all RaBitQ, not just split | P2 | Open — gates on quantizer name only |
| Redundant SQ8 per hierarchy in multi-hierarchy builds | P2 | Open — each ODescent independently creates SQ8 |
NaN sentinel for rabitq_error_rate |
suggestion | Deferred by author |
Silent skip logging in prepare_build_flatten |
suggestion | Deferred by author |
Summary
The refactoring is clean and correct. The three remaining P2 issues are non-blocking for merge but should be tracked for follow-up. All P1 and critical issues from earlier reviews have been resolved.
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (531 changed lines across 18 files).
Submitted 4 inline comments.
Reviewed commit 3c3a50f.
| search_param.topk = k; | ||
| search_param.search_mode = KNN_SEARCH; | ||
| search_param.parallel_search_thread_count = parsed_param.parallel_search_thread_count; | ||
| search_param.enable_rabitq_one_bit_search = parsed_param.has_rabitq_one_bit_search |
There was a problem hiding this comment.
[P2] Propagate split lower-bound candidates into reorder
This enables one-bit traversal by default for split indexes, but search_node() never gives BasicSearcher a DistanceRecordVector, and search_impl() calls reorder without one. Consequently the quantizer receives a null lower-bound output and returns before applying ctx.rabitq_error_rate; candidates rejected by their x-bit estimate are also never rescored with x+y codes. Thread and merge lower-bound candidates through Pyramid as HGraph does before enabling this default.
| ODescent::prepare_build_flatten() { | ||
| if (this->build_flatten_interface_ != nullptr or this->build_vectors_ == nullptr or | ||
| this->build_vector_count_ <= 0 or | ||
| this->flatten_interface_->GetQuantizerName() != QUANTIZATION_TYPE_VALUE_RABITQ) { |
There was a problem hiding this comment.
[P2] Restrict the SQ8 substitute to split RaBitQ storage
GetQuantizerName() returns rabitq for both standard and split cells. Because Pyramid now supplies raw vectors to every ODescent builder, an existing standard RaBitQ configuration such as use_reorder: false now builds its graph with a separately trained SQ8 store and incurs an additional O(N·D) allocation. Gate this substitution specifically on split storage so omitting rabitq_bits_per_dim_precise preserves the previous graph distances and memory behavior.
| auto* root_ptr = h_ptr->root.get(); | ||
| futures.push_back(thread_pool_->GeneralEnqueue([&, codes, root_ptr]() { | ||
| ODescent builder(odescent_param_, codes, allocator_, nullptr); | ||
| ODescent builder( |
There was a problem hiding this comment.
[P2] Share the temporary SQ8 store across hierarchies
Every concurrent hierarchy constructs its own ODescent with the complete raw matrix, so each calls prepare_build_flatten() and trains and stores another full N×D SQ8 copy. Peak memory and training work therefore scale with hierarchy count and can make otherwise valid large multi-hierarchy builds run out of memory. Build one read-only temporary store and share it across hierarchy builders.
| index->KnnSearch(query, 1, R"({"pyramid":{"ef_search":10}})", vsag::FilterPtr{}); | ||
| REQUIRE(result->GetIds()[0] == ids[i]); | ||
| REQUIRE(result->GetDim() == 1); | ||
| REQUIRE(std::find(ids.begin(), ids.end(), result->GetIds()[0]) != ids.end()); |
There was a problem hiding this comment.
[P3] Preserve a meaningful promotion search oracle
This assertion passes if every query always returns the same arbitrary dataset ID, so it no longer verifies the previous non-split invariant that each vector retrieves itself after FLAT-to-GRAPH promotion. Keep the exact ID assertion for the generated non-split case and use a meaningful distance or recall criterion for split RaBitQ.
Signed-off-by: LightWant <2510035537@qq.com> Assisted-by: Codex:gpt-5
Signed-off-by: LightWant <2510035537@qq.com> Assisted-by: Codex:gpt-5
| } | ||
|
|
||
| inner_json[REORDER_SOURCE_KEY].SetString(HGRAPH_REORDER_SOURCE_BASE); | ||
| inner_json[BASE_CODES_KEY][CODES_TYPE_KEY].SetString(RABITQ_SPLIT_CODES); |
There was a problem hiding this comment.
[suggestion] map_rabitq_split_param silently overwrites user-specified reorder_source
mapping_external_param_to_inner runs first and may set REORDER_SOURCE_KEY from a user-provided reorder_source value. Then map_rabitq_split_param unconditionally overwrites it with HGRAPH_REORDER_SOURCE_BASE. If a user explicitly sets reorder_source: "precise" alongside rabitq_bits_per_dim_precise, the conflicting value is silently discarded.
Consider adding a validation check before the overwrite:
if (inner_json.Contains(REORDER_SOURCE_KEY)) {
CHECK_ARGUMENT(inner_json[REORDER_SOURCE_KEY].GetString() == HGRAPH_REORDER_SOURCE_BASE,
fmt::format("split mode requires reorder_source={}, got {}",
HGRAPH_REORDER_SOURCE_BASE,
inner_json[REORDER_SOURCE_KEY].GetString()));
}This would give users a clear error message instead of silently ignoring their explicit setting.
| @@ -113,6 +129,7 @@ Search-time parameters live under the `pyramid` sub-object: | |||
| | `subindex_ef_search` | int | `50` | Candidate list size used when traversing intermediate sub-graphs on the path. | | |||
There was a problem hiding this comment.
[suggestion] rabitq_one_bit_search search parameter is undocumented in the pyramid parameter table
PyramidSearchParameters::FromJson parses pyramid.rabitq_one_bit_search, and the pyramid.h constructor computes default_rabitq_one_bit_search_ to enable one-bit split search by default for split indexes. The rabitq_split.md docs mention this parameter, but the pyramid search parameter tables in both docs/docs/en/src/indexes/pyramid.md and docs/docs/zh/src/indexes/pyramid.md do not list it.
Consider adding a row like:
| rabitq_one_bit_search | bool | auto (true for split indexes) | Force the one-bit lower-bound search path in RaBitQ split mode. |
| PYRAMID_RABITQ_BITS_PER_DIM_BASE, | ||
| PYRAMID_RABITQ_BITS_PER_DIM_PRECISE, | ||
| total_bits)); | ||
|
|
There was a problem hiding this comment.
[suggestion] map_rabitq_split_param should propagate user-specified rabitq_bits_per_dim_query to internal JSON when it equals 32
When the user explicitly sets rabitq_bits_per_dim_query to 32 (the only valid value for split mode), the function only validates it but does not propagate it to inner_json. Instead, line 108 unconditionally sets RABITQ_QUANTIZATION_BITS_PER_DIM_QUERY_KEY to 32 in the internal JSON.
This works correctly because 32 is the only allowed value, but it means any user-specified value of 32 is silently accepted while the internal JSON always gets the hardcoded 32. If the validation is changed in the future (e.g., to also allow 4), this would need updating in two places. Consider propagating the validated value:
if (external_json.Contains(PYRAMID_RABITQ_BITS_PER_DIM_QUERY)) {
const int64_t query_bits = external_json[PYRAMID_RABITQ_BITS_PER_DIM_QUERY].GetInt();
CHECK_ARGUMENT(query_bits == 32, ...);
inner_json[...][RABITQ_QUANTIZATION_BITS_PER_DIM_QUERY_KEY].SetInt(query_bits);
} else {
inner_json[...][RABITQ_QUANTIZATION_BITS_PER_DIM_QUERY_KEY].SetInt(32);
}| ODescent::prepare_build_flatten() { | ||
| if (this->build_flatten_interface_ != nullptr or this->build_vectors_ == nullptr or | ||
| this->build_vector_count_ <= 0 or | ||
| this->flatten_interface_->GetQuantizerName() != QUANTIZATION_TYPE_VALUE_RABITQ) { |
There was a problem hiding this comment.
[note] P2 follow-up: SQ8 build-time substitute affects non-split RaBitQ configurations
prepare_build_flatten() in odescent_graph_builder.cpp:76 gates on GetQuantizerName() == "rabitq", which matches both standard and split RaBitQ cells. Since Pyramid now passes raw build vectors to every ODescent constructor, a pre-existing standard RaBitQ configuration (e.g. use_reorder: false without rabitq_bits_per_dim_precise) will now silently build its graph using separately trained SQ8 distances and allocate an additional O(N·D) scalar-code store.
This changes graph quality and peak memory for existing standard RaBitQ Pyramid indexes. Consider gating this optimization on the split data cell type (e.g. checking flatten_interface_->GetCodesType() == RABITQ_SPLIT_CODES) so that standard RaBitQ configurations retain their previous behavior.
| uint64_t subindex_ef_search{50}; | ||
| bool has_rabitq_one_bit_search{false}; | ||
| bool rabitq_one_bit_search{false}; | ||
| float rabitq_error_rate{std::numeric_limits<float>::quiet_NaN()}; |
There was a problem hiding this comment.
[note] rabitq_error_rate uses quiet_NaN() as sentinel — consider matching the has_rabitq_one_bit_search pattern
rabitq_error_rate defaults to std::numeric_limits<float>::quiet_NaN(). NaN has unusual comparison behavior (NaN != NaN is true, NaN < 0 is false), which can lead to subtle bugs if someone writes if (params.rabitq_error_rate > 0) without first checking whether it was set.
The has_rabitq_one_bit_search / rabitq_one_bit_search pair in the same struct already demonstrates a cleaner pattern. Consider using std::optional<float> or adding a has_rabitq_error_rate boolean to match the existing convention. This is non-blocking since the downstream quantizer treats non-finite values as "use the index default", but worth addressing for API consistency.
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (559 changed lines across 18 files).
Submitted 4 inline comments.
Reviewed commit 27ceb32.
| search_param.topk = k; | ||
| search_param.search_mode = KNN_SEARCH; | ||
| search_param.parallel_search_thread_count = parsed_param.parallel_search_thread_count; | ||
| search_param.enable_rabitq_one_bit_search = parsed_param.has_rabitq_one_bit_search |
There was a problem hiding this comment.
[P2] Propagate split lower-bound candidates before enabling this default
For a split index this enables x-bit traversal by default, but search_node() calls BasicSearcher without a DistanceRecordVector and search_impl() calls reorder without it. Consequently QueryWithDistanceLowerBound receives lower_bounds == nullptr, returns before using ctx.rabitq_error_rate, and candidates rejected by the x-bit estimate are never rescored with x+y codes (including range candidates). Thread and merge lower-bound records through the node searches and pass them to FlattenReorder, as HGraph does.
| ODescent::prepare_build_flatten() { | ||
| if (this->build_flatten_interface_ != nullptr or this->build_vectors_ == nullptr or | ||
| this->build_vector_count_ <= 0 or | ||
| this->flatten_interface_->GetQuantizerName() != QUANTIZATION_TYPE_VALUE_RABITQ) { |
There was a problem hiding this comment.
[P2] Restrict the SQ8 substitute to split RaBitQ storage
GetQuantizerName() is rabitq for both standard and split cells. Because Pyramid now passes raw vectors to every ODescent builder, a standard RaBitQ configuration without rabitq_bits_per_dim_precise—for example use_reorder: false—silently builds its graph with separately trained SQ8 distances and allocates an extra O(N·D) store. Gate this substitution on the split data cell so the promised unchanged standard path retains its prior graph and memory behavior.
| auto* root_ptr = h_ptr->root.get(); | ||
| futures.push_back(thread_pool_->GeneralEnqueue([&, codes, root_ptr]() { | ||
| ODescent builder(odescent_param_, codes, allocator_, nullptr); | ||
| ODescent builder( |
There was a problem hiding this comment.
[P2] Share the temporary SQ8 store across concurrent hierarchies
Every concurrent hierarchy receives the same complete raw matrix but creates its own ODescent, so each instance independently trains and stores a full N×D SQ8 copy. Peak memory and training work therefore scale with hierarchy count and can make otherwise valid large multi-hierarchy split builds run out of memory. Construct one read-only temporary flatten store and share it across the hierarchy builders.
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
Overall Assessment
This PR exposes split RaBitQ parameters for the Pyramid index, allowing x+y split storage mode (previously only available in HGraph). The implementation is well-structured and follows established patterns from the HGraph split RaBitQ integration.
Strengths
has_precise_reorder()/get_reorder_codes()helpers inpyramid.hcorrectly distinguish split mode (reorder from base codes) from traditional reorder (separate precise codes), avoiding confusion between the two paths.- Parameter validation in
map_rabitq_split_param()is thorough: filter_bits ∈ [1,8], supplement_bits ∈ [1,8], total ≤ 8, query_bits == 32. prepare_build_flatten()inodescent_graph_builder.cppproperly handles the build-time distance computation gap when quantizer is RaBitQ.- Test coverage added for split RaBitQ builds with ODescent in both integration and unit tests.
- Documentation updated across 6 files (EN/ZH) with parameter tables and configuration examples.
Issues Identified (7 inline comments posted)
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | [suggestion] | pyramid.cpp:105 |
map_rabitq_split_param silently overwrites user-specified reorder_source |
| 2 | [suggestion] | pyramid.md:129 |
rabitq_one_bit_search search parameter undocumented in parameter tables |
| 3 | [suggestion] | pyramid.cpp:95 |
map_rabitq_split_param should propagate user-specified rabitq_bits_per_dim_query |
| 4 | [note] P2 | pyramid.cpp:353 |
rabitq_error_rate stored in QueryContext but not consumed by search path |
| 5 | [note] P2 | odescent_graph_builder.cpp:76 |
SQ8 build-time substitute affects non-split RaBitQ configurations |
| 6 | [note] P2 | pyramid.cpp:319 |
Multi-hierarchy builds create redundant SQ8 stores per hierarchy |
| 7 | [note] | pyramid_zparameters.h:105 |
rabitq_error_rate uses quiet_NaN() as sentinel — consider matching has_rabitq_one_bit_search pattern |
Recommendation
The core implementation is solid. Items 1-3 are minor suggestions that could be addressed in this PR. Items 4-6 are P2 follow-ups suitable for post-merge. Overall the PR is in good shape for merging.
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
This PR adds RaBitQ x+y split parameter support to the Pyramid index, enabling split storage and reordering where the base codes serve as both filter codes (x bits) and reorder source (x+y bits), eliminating the need for a separate precise codes store.
Overall Assessment
The implementation is well-structured and follows existing patterns from the HGraph split RaBitQ integration. The parameter mapping, validation, and build-time SQ8 flatten interface are correctly implemented. Test coverage is thorough with unit tests for parameter parsing, functional tests for build and search with split mode, and integration tests for the full pipeline.
Key Observations
-
map_rabitq_split_paramordering (pyramid.cpp:1089): Called aftermapping_external_param_to_inner, correctly overwriting the previously-mappedrabitq_bits_per_dim_basewithtotal_bits(x+y). A comment noting this intentional overwrite would help future readers. -
RABITQ_ERROR_RATEdual mapping (pyramid.cpp:1048-1051): Mapped to both base and precise codes quantization params. In non-split mode where precise_codes uses fp32, the key is harmlessly ignored, but it may confuse debugging. -
Most previously-raised concerns have been addressed: String type checks, query bits validation, split NSW promotion fix, and thread-safety documentation have all been incorporated by the author.
Recommendation
Approve. The remaining notes are non-blocking clarity improvements. The core logic is correct and well-tested.
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
This PR adds RaBitQ x+y split parameter support to the Pyramid index, enabling split storage and reordering where the base codes serve as both filter codes (x bits) and reorder source (x+y bits), eliminating the need for a separate precise codes store.
Overall Assessment
The implementation is well-structured and follows existing patterns from the HGraph split RaBitQ integration. The parameter mapping, validation, and build-time SQ8 flatten interface are correctly implemented. Test coverage is thorough with unit tests for parameter parsing, functional tests for build and search with split mode, and integration tests for the full pipeline.
Key Observations
-
map_rabitq_split_param ordering (pyramid.cpp:1089): Called after mapping_external_param_to_inner, correctly overwriting the previously-mapped rabitq_bits_per_dim_base with total_bits (x+y). A comment noting this intentional overwrite would help future readers.
-
RABITQ_ERROR_RATE dual mapping (pyramid.cpp:1048-1051): Mapped to both base and precise codes quantization params. In non-split mode where precise_codes uses fp32, the key is harmlessly ignored, but it may confuse debugging.
-
Most previously-raised concerns have been addressed: String type checks, query bits validation, split NSW promotion fix, and thread-safety documentation have all been incorporated by the author.
Recommendation
Approve. The remaining notes are non-blocking clarity improvements. The core logic is correct and well-tested.
LHT129
left a comment
There was a problem hiding this comment.
Code review complete. Two inline comments posted. Summary: approve with minor notes on parameter mapping ordering and error rate dual mapping.
LHT129
left a comment
There was a problem hiding this comment.
Approve with minor notes.
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
This PR adds RaBitQ x+y split parameter support to the Pyramid index, enabling split storage and reordering where the base codes serve as both filter codes (x bits) and reorder source (x+y bits), eliminating the need for a separate precise codes store.
Overall Assessment
The implementation is well-structured and follows existing patterns from the HGraph split RaBitQ integration. The parameter mapping, validation, and build-time SQ8 flatten interface are correctly implemented. Test coverage is thorough with unit tests for parameter parsing, functional tests for build and search with split mode, and integration tests for the full pipeline.
Key Observations
-
map_rabitq_split_paramordering (pyramid.cpp): Called aftermapping_external_param_to_inner, correctly overwriting the previously-mappedrabitq_bits_per_dim_basewithtotal_bits(x+y). A comment noting this intentional overwrite would help future readers. -
RABITQ_ERROR_RATEdual mapping (pyramid.cpp): Mapped to both base and precise codes quantization params. In non-split mode where precise_codes uses fp32, the key is harmlessly ignored, but it may confuse debugging. -
Most previously-raised concerns have been addressed: String type checks, query bits validation, split NSW promotion fix, and thread-safety documentation have all been incorporated by the author.
Recommendation
Approve. The remaining notes are non-blocking clarity improvements. The core logic is correct and well-tested.
|
/retest |
Summary
rabitq_one_bit_searchoverridesrabitq_bits_per_dim_preciseis not providedFixes: #2454
Test
make debug./build/tests/unittests "[PyramidParameters]"make testpartially: full unit tests passed; functional tests were manually interrupted during(Daily) HGraph Build & ContinueAdd Testafter long runtime