refactor(parameters): replace template-based index mappings

[LHT129]

Summary

Replace template-based parameter mapping with structured defaults and explicit flat-field translation across index entry points.

Changes

• Replace JSON string templates in HGraph, Pyramid, IVF, BruteForce/WARP, and SIMQ with structural builders.
• Remove ConstParamMap usage while preserving one-to-many mappings and unknown-field rejection.
• Introduce RaBitQSplitConfig parsing/application and remove the downstream split-version mutation.
• Add SINDI and SINDI v2 flat-key boundary validation.
• Add CompatibilityReport issue collection while preserving CheckCompatibility.
• Add regression coverage for split configuration and multi-difference compatibility reporting.

Behavior change

• codes_type=rabitq_split now requires the nested RaBitQ quantizer to already use rabitq_version=split. Inconsistent configurations are rejected instead of silently mutating rabitq_version during FlattenDataCellParameter::FromJson. Public index mappings construct the canonical split configuration before parsing.

Testing

• Release build passed using the remote fixed-version dependency cache after a GitHub download timeout.
• 713 non-daily unit tests passed with 85,349,000 assertions.
• SINDIV2Parameter focused suite passed: 15 test cases, 68 assertions.
• HGraphParameter focused suite passed: 19 test cases, 128 assertions.
• clang-format 15 passed.
• clang-tidy 15 passed.

Related to #2729

[vsag-bot]

/label status/waiting-for-review
/waiting-on reviewer
/request-review @jiaweizone
/request-review @inabao

[mergify]

Merge Protections

🟢 All 2 merge protections satisfied — ready to merge.

Show 2 satisfied protections

🟢 Require kind label

• label~=^kind/

🟢 Require version label

• label~=^version/

[Copilot]

Pull request overview

This PR refactors the index-parameter mapping layer to replace string-template JSON defaults and mapping tables with structured default builders plus explicit flat-key translation, while also centralizing RaBitQ split handling and adding a compatibility-report API to surface all JSON differences.

Changes:

• Replaces template-based default parameter JSON generation with structured builders and explicit per-key mapping for multiple index entry points (HGraph, Pyramid, IVF, BruteForce/WARP, SIMQ).
• Introduces CompatibilityReport / CollectCompatibilityIssues() to collect all JSON differences while preserving the existing boolean compatibility check.
• Centralizes RaBitQ split parsing/application and removes downstream “split-version” mutation; adds boundary validation for flat external keys (e.g., SINDI, SIMQ).

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/quantization/fp32_quantizer_parameter_test.cpp	Adds regression test ensuring compatibility reporting collects all JSON differences.
src/parameter.h	Adds CompatibilityReport/CompatibilityIssue and implements JSON-diff collection on Parameter.
src/datacell/flatten_datacell_parameter.cpp	Removes RaBitQ split-version mutation; enforces canonical split-quantizer requirement when codes_type=rabitq_split.
src/algorithm/sindi/sindi.cpp	Adds flat-key boundary validation (unknown-field rejection) for SINDI external params.
src/algorithm/simq/simq.cpp	Replaces template mapping with structured defaults + explicit key validation/mapping for SIMQ.
src/algorithm/pyramid/pyramid.cpp	Replaces template mapping with structured defaults + explicit flat-key translation; applies centralized RaBitQ split config.
src/algorithm/ivf/ivf.cpp	Replaces template mapping with structured defaults + explicit flat-key translation for IVF.
src/algorithm/inner_index_parameter.h	Introduces RaBitQSplitConfig API and replaces mutation-based helper with parse/apply split config functions.
src/algorithm/inner_index_parameter.cpp	Implements RaBitQ split parsing/validation and application into inner JSON.
src/algorithm/inner_index_parameter_test.cpp	Adds regression coverage for split configuration parse/apply behavior.
src/algorithm/hgraph/hgraph_param_mapping.cpp	Replaces template mapping with structured defaults + explicit mapping; applies centralized RaBitQ split config.
src/algorithm/bruteforce/bruteforce.cpp	Replaces template mapping with structured defaults + explicit flat-key translation for BruteForce/WARP.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

src/algorithm/sindi/sindi.cpp:266

• This uses std::unordered_set but the file’s includes (in the shown hunk) don’t include <unordered_set>. Relying on transitive includes is fragile and may fail to compile on some toolchains; add an explicit #include <unordered_set>.

    static const std::unordered_set<std::string> supported_keys = {
        SPARSE_TERM_ID_LIMIT,
        SPARSE_DOC_PRUNE_RATIO,
        USE_REORDER_KEY,
        USE_QUANTIZATION,
        SPARSE_WINDOW_SIZE,
        SPARSE_AVG_DOC_TERM_LENGTH,
        SPARSE_DESERIALIZE_WITHOUT_FOOTER,
        SPARSE_DESERIALIZE_WITHOUT_BUFFER,
        SPARSE_REMAP_TERM_IDS,
        SPARSE_RERANK_TYPE,
        SPARSE_DMQ_SHARED_CODEBOOK_THRESHOLD,
        SPARSE_IMMUTABLE,
    };

src/algorithm/simq/simq.cpp:1105

• This introduces std::unordered_set usage without an explicit <unordered_set> include in the visible include list. Add #include <unordered_set> to avoid build breaks due to missing transitive includes.

    static const std::unordered_set<std::string> keys = {BRUTE_FORCE_BASE_IO_TYPE,
                                                         BRUTE_FORCE_BASE_FILE_PATH,
                                                         "init_cluster_ratio",
                                                         "max_cluster_size",
                                                         "split_start_idx",
                                                         "random_seed",
                                                         "coarse_k",
                                                         "rerank_k"};

src/parameter.cpp:45

• The collected issue messages don’t indicate which side is missing/unexpected (e.g., missing from other vs missing from this). Since these messages are surfaced as diagnostics (not just internal errors), making them directional (e.g., "missing in right-hand config" / "unexpected in right-hand config") would make compatibility reports more actionable.

                        report.issues.push_back({child_path, "field is missing"});

src/parameter.cpp:53

• The collected issue messages don’t indicate which side is missing/unexpected (e.g., missing from other vs missing from this). Since these messages are surfaced as diagnostics (not just internal errors), making them directional (e.g., "missing in right-hand config" / "unexpected in right-hand config") would make compatibility reports more actionable.

                        report.issues.push_back({path + "." + key, "unexpected field"});

src/quantization/fp32_quantizer_parameter_test.cpp:63

• This test assumes a specific ordering of report.issues. If JSON object iteration order changes (e.g., due to different nlohmann::json object type or wrapper behavior), this can become flaky. Consider asserting on an order-independent representation (e.g., collect paths into a set/vector and sort before comparison) so the test validates content rather than iteration order.

    REQUIRE(report.issues.size() == 3);
    REQUIRE(report.issues[0].path == "$.first");
    REQUIRE(report.issues[1].path == "$.nested.second");
    REQUIRE(report.issues[2].path == "$.extra");

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

src/algorithm/pyramid/pyramid.cpp:1204

• This parses a JSON string at runtime to create an empty array for defaults. Prefer constructing an empty array JsonType directly (or using an existing helper) to avoid unnecessary parsing overhead and potential parse-failure paths in a default builder.

    json[NO_BUILD_LEVELS].SetJson(JsonType::Parse("[]"));

src/parameter.h:17

• CompatibilityIssue introduces std::string in this header; consider explicitly including <string> here to avoid relying on transitive includes (include-what-you-use).

#include <vector>

src/parameter.h:31

• CompatibilityIssue introduces std::string in this header; consider explicitly including <string> here to avoid relying on transitive includes (include-what-you-use).

struct CompatibilityIssue {
    std::string path;
    std::string message;
};

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/algorithm/bruteforce/bruteforce.cpp:1253

• In BruteForce external param mapping, STORE_RAW_VECTOR is currently written to a top-level quantization_params.hold_molds field, but the BruteForce schema places quantization_params under base_codes (and precise_codes). As a result, the user-provided store_raw_vector value is ignored by CreateFlattenParam(base_codes_json) / the quantizer parameter parsing.

        } else if (key == STORE_RAW_VECTOR) {
            inner_json[QUANTIZATION_PARAMS_KEY][HOLD_MOLDS].SetJson(field);
        } else if (key == USE_ATTRIBUTE_FILTER) {

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (7)

src/algorithm/simq/simq.cpp:1096

• ValidateSIMQExternalKeys() is a file-local helper but currently has external linkage, which makes it an exported symbol from the shared library (vsag uses default visibility). Mark it static or move it into an anonymous namespace to avoid unintentionally expanding the ABI surface.

void

src/algorithm/bruteforce/bruteforce.cpp:1252

• STORE_RAW_VECTOR is currently mapped to a top-level "quantization_params.hold_molds" field, but BruteForceParameter only consumes BASE_CODES_KEY (via CreateFlattenParam(base_codes_json)). This means store_raw_vector will not affect the actual base_codes quantizer config.

        } else if (key == STORE_RAW_VECTOR) {
            inner_json[QUANTIZATION_PARAMS_KEY][HOLD_MOLDS].SetJson(field);

src/algorithm/pyramid/pyramid.cpp:1148

• BuildDefaultPyramidParam() is a file-local helper but currently has external linkage, which makes it an exported symbol from the shared library (vsag uses default visibility). Mark it static or move it into an anonymous namespace to avoid unintentionally expanding the ABI surface.

JsonType

src/algorithm/ivf/ivf.cpp:79

• BuildDefaultIVFParam() is a file-local helper but currently has external linkage, which makes it an exported symbol from the shared library (vsag uses default visibility). Mark it static or move it into an anonymous namespace to avoid unintentionally expanding the ABI surface.

JsonType

src/algorithm/simq/simq.cpp:1086

• BuildDefaultSIMQParam() is a file-local helper but currently has external linkage, which makes it an exported symbol from the shared library (vsag uses default visibility). Mark it static or move it into an anonymous namespace to avoid unintentionally expanding the ABI surface.

This issue also appears on line 1096 of the same file.

JsonType

src/algorithm/bruteforce/bruteforce.cpp:1143

• BuildDefaultBruteForceParam() is a file-local helper but currently has external linkage, which makes it an exported symbol from the shared library (vsag uses default visibility). Mark it static or move it into an anonymous namespace to avoid unintentionally expanding the ABI surface.

This issue also appears on line 1251 of the same file.

JsonType

src/algorithm/sindi/sindi.cpp:25

• This file uses std::unordered_set but does not include <unordered_set>. Relying on transitive includes is non-portable and can break builds across standard libraries/compilers.

#include <shared_mutex>
#include <unordered_map>
#include <vector>

[LHT129]

Code Review Summary

Commit reviewed: a11cf1c

All issues flagged in the previous Copilot review have been addressed:

1. STORE_RAW_VECTOR — ApplyHoldMoldsToQuantizer is now correctly applied to both base_codes and precise_codes quantizers in bruteforce.cpp and hgraph_param_mapping.cpp.

2. size_t → uint64_t — Changed in parameter.h to match VSAG coding standards.

3. Test ordering dependency — Fixed by using std::map for order-independent assertions in fp32_quantizer_parameter_test.cpp.

4. TQ TYPE_KEY — Now properly set in TransformQuantizerParameter::ToJson() at transform_quantizer_parameter.cpp:112.

5. TQ whitespace — SplitString trims whitespace; verified by test.

Additional observations (no action needed)

• JSON pointer escaping (escape_json_pointer_token / append_json_pointer_token) is RFC 6901 compliant. The test correctly verifies "/a~1b" for key "a/b".
• RaBitQSplitConfig struct with ParseRaBitQSplitConfig (validates external params) + ApplyRaBitQSplitConfig (mutates inner JSON) is a clean separation of concerns. The shared key "base_quantization_type" works correctly for both HGraph and Pyramid since HGRAPH_BASE_QUANTIZATION_TYPE and PYRAMID_BASE_QUANTIZATION_TYPE have the same string value.
• build_default_flatten_param double-application of hold_molds (via CreateDefault + ApplyHoldMoldsToQuantizer) is intentional: CreateDefault handles FP32/INT8 directly, while ApplyHoldMoldsToQuantizer unwraps TQ chains to reach the bottom quantizer.
• SINDI/SINDIV2 now validate external params against a supported_keys set, rejecting unknown keys with a clear error message.
• FlattenDataCellParameter::CreateDefault correctly sets hold_molds on FP32/INT8 quantizer parameter objects, and ToJson serializes it.
• Naming convention — new helper functions use snake_case (build_default_*), consistent with the existing codebase style.
• Test coverage — regression tests added for split config, compatibility reporting, and hold_molds behavior.

No new substantive issues found. The refactoring is clean and well-tested.

[LHT129]

This push addresses all previously raised review comments:

1. STORE_RAW_VECTOR path — Now correctly writes to base_codes.quantization_params and precise_codes.quantization_params via ApplyHoldMoldsToQuantizer, with a regression test added in bruteforce_parameter_test.cpp.

2. size_t → uint64_t — The CollectCompatibilityIssues loop in parameter.cpp now uses uint64_t for array traversal, consistent with project conventions.

3. Test ordering dependency — The CompatibilityReport test in fp32_quantizer_parameter_test.cpp now uses std::map for order-independent assertions, and also covers JSON pointer escaping (/a~1b).

4. Chain whitespace — SplitString already trims whitespace around tokens, so "mrle, rabitq" and "mrle,rabitq" are both handled correctly. A dedicated test in transform_quantizer_parameter_test.cpp verifies this.

No new issues found. The refactoring is clean, well-tested, and all prior concerns are resolved.

[Copilot]

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

src/datacell/bucket_datacell_parameter_test.cpp:55

• This line exceeds the 100-character limit for C++ sources (see AGENTS.md hard constraints). Please wrap the statement to keep lines <= 100 chars.

        auto parameter = MultiVectorDataCellParameter::CreateDefault(IO_TYPE_VALUE_BLOCK_MEMORY_IO);

[LHT129]

Review Summary

This is a large, well-executed refactoring that replaces template-based JSON parameter mapping (ConstParamMap + string templates) with explicit build_default_* functions and if-else chains across all index types. The PR also introduces CompatibilityReport/CompatibilityIssue for structured compatibility checking and RaBitQSplitConfig parsing/application.

What's been addressed from prior reviews

• STORE_RAW_VECTOR now correctly maps to base_codes.quantization_params.hold_molds and precise_codes.quantization_params.hold_molds via ApplyHoldMoldsToQuantizer
• parameter.h includes <string> for CompatibilityIssue/CompatibilityReport
• sindi.cpp includes <unordered_set>
• Test ordering issue fixed by using std::map for order-independent assertions
• FlattenDataCellParameter::CreateDefault now handles hold_molds for both FP32 and INT8
• SplitString trims whitespace, so chain parsing handles spaces correctly
• Unused build_default_quantization_param and stale includes removed

Items flagged in this review

1. Error message format (inner_index_parameter.cpp:105): The error message shows tq_chain="mrle, rabitq" (with space), but SplitString normalizes whitespace, so the no-space variant is also valid. Consider using the canonical normalized form in the error message.
2. Performance note (parameter.cpp:29): CollectCompatibilityIssues does a full JSON round-trip for comparison. Fine for a non-hot-path compatibility check, but worth noting.
3. Consistency note (hgraph_param_mapping.cpp:295): SINDI/SINDI v2 use a supported_keys whitelist for validation; the other index types use if-else chains with a catch-all throw. Both approaches are functionally correct, but consistency would improve maintainability.

Overall assessment

The refactoring is sound. The explicit mapping approach is more readable and maintainable than the template-based system it replaces. All previously flagged issues have been addressed. No correctness or security issues found.

[Copilot]

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated 1 comment.

[LHT129]

[note] In src/parameter.cpp:92, common_size is derived from nlohmann::json::size() which returns size_t, but the loop iterates with uint64_t i. This can produce a signed/unsigned mismatch warning on platforms where size_t is narrower than uint64_t (e.g., 32-bit) or trigger -Wsign-compare warnings.

Consider using size_t for the loop index to match the container size type, or casting common_size explicitly to uint64_t if the project convention requires uint64_t everywhere:

for (size_t i = 0; i < common_size; ++i) {

[Copilot]

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated 1 comment.

[LHT129]

Review Summary

This PR replaces template-based JSON parameter mapping with structural builders and explicit flat-field translation across all index types. The refactoring is thorough and well-structured, with several improvements over the previous approach.

Issues Fixed from Previous Reviews

• STORE_RAW_VECTOR now correctly applies ApplyHoldMoldsToQuantizer to both base and precise codes in BruteForce, HGraph, and Pyramid
• size_t replaced with uint64_t in CollectCompatibilityIssues
• Test ordering dependency fixed with std::map
• SplitString whitespace trimming already handled

New Additions

• CompatibilityReport / CompatibilityIssue with JSON pointer path escaping (~0 for ~, ~1 for /) provides structured compatibility checking
• RaBitQSplitConfig parsing and ApplyRaBitQSplitConfig centralize split quantizer configuration
• ApplyHoldMoldsToQuantizer propagates hold_molds flag through TQ chains to supported quantizers (fp32, int8)
• ValidateMRLEDim and RequiresRawVectorFor* helpers consolidate validation logic
• CreateDefault factory methods on parameter classes improve testability and code reuse
• Whitelist-based validation for SINDI/SINDIV2 parameters provides clear error messages

One Minor Note

• build_default_simq_param defaults to IO_TYPE_VALUE_ASYNC_IO while all other builders default to IO_TYPE_VALUE_BLOCK_MEMORY_IO. This is consistent with the old template, but worth confirming the asymmetry is intentional.

Overall

The refactoring eliminates the fragile string-template mutation chain and replaces it with structured, type-safe builders. The explicit if-else chains are verbose but clear and maintainable. No blocking issues found.

[Copilot]

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[LHT129]

Code Review: PR #2730

Commit: b00f317 — refactor(parameters): replace template-based index mappings

Summary

This is a well-executed refactoring that replaces template-string-based JSON parameter construction (format_map + ConstParamMap + DEFAULT_MAP) with explicit builder functions and structured if-else field mapping chains across BruteForce, HGraph, IVF, Pyramid, and SIMQ index types.

What was reviewed

• 36 changed files, 1327 additions, 1582 deletions
• All key source files read from commit b00f317
• Existing review comments from previous commits checked for resolution

Positive observations

1. Previous Copilot issues are all addressed:

   • STORE_RAW_VECTOR now correctly applies ApplyHoldMoldsToQuantizer to both base and precise codes (was previously only applied to base codes)
   • size_t usage replaced with int64_t where appropriate
   • Error messages use consistent formatting
   • CreateDefault() no longer trims user-specified quantization params
   • Test ordering corrected

2. Clean removal of dead code: DEFAULT_MAP (~100 entries) and all format_map/ConstParamMap usage removed. Verified no remaining references in the PR commit.

3. Good boundary validation added: SINDI and SINDIV2 now use supported_keys unordered_set to reject unknown flat keys, preventing silent parameter drift.

4. Comprehensive test coverage: New regression tests added for bruteforce, hgraph, pyramid, sindi_v2, bucket_datacell, fp32/int8 quantizer, transform_quantizer, memory_io, and simq parameter handling.

5. CompatibilityReport / CollectCompatibilityIssues: The recursive JSON diff approach in parameter.cpp is a clean, general-purpose mechanism for detecting parameter incompatibility.

6. RaBitQSplitConfig: Well-structured with proper validation in ParseRaBitQSplitConfig and clean separation of parsing vs. application.

Issues found

No substantive issues identified. The refactoring is consistent, the builder pattern is applied uniformly across all index types, and all edge cases (unknown keys, missing fields, type mismatches) are handled with appropriate error reporting.

Verdict

LGTM. Ready to merge pending CI pass.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.