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 @wxyucs
/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>

[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:1252

• STORE_RAW_VECTOR is currently written to inner_json[QUANTIZATION_PARAMS_KEY][HOLD_MOLDS], but the BruteForce defaults (and BruteForceParameter::FromJson) only look under base_codes.quantization_params (and precise_codes.quantization_params). As written, the external flag will not affect the quantizer params and instead creates an unused top-level quantization_params object.

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

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/quantization/transform_quantization/transform_quantizer_parameter.cpp:28

• The tq_chain parsing doesn’t appear to trim whitespace or reject empty segments. Configurations like \"mrle, rabitq\" (note the space) or \"mrle,\" can yield a bottom_type with leading whitespace or an empty string, which will break default creation/dispatch in hard-to-diagnose ways. Consider trimming each split item and validating that all segments are non-empty before using chain_items.back().

    auto chain_items = SplitString(chain);
    CHECK_ARGUMENT(chain_items.size() > 1,
                   "tq_chain must contain at least one transformer and one quantizer");
    const auto bottom_type = chain_items.back();

src/quantization/fp32_quantizer_parameter_test.cpp:63

• This test assumes a stable ordering of report.issues, but the traversal order depends on JSON object iteration semantics (which may differ by build options or upstream changes). To avoid flaky behavior, compare the collected paths/messages order-independently (e.g., sort by path, or assert presence via a set).

    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 27 out of 27 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[LHT129]

[note] This is a well-executed refactoring PR that replaces template-based parameter mapping with structured builders across all index entry points. The diff is large (1110 additions, 1484 deletions) but the changes are mechanical and consistent.

Summary of review:

The refactoring is sound. Key improvements observed:

• JSON string templates replaced with CreateDefault() / BuildDefault*() factory functions that construct canonical inner configs programmatically.
• ConstParamMap / format_map / mapping_external_param_to_inner removed entirely, along with the DEFAULT_MAP in inner_string_params.h.
• RaBitQSplitConfig parsing separated from application (ParseRaBitQSplitConfig + ApplyRaBitQSplitConfig), making the split-config flow explicit and testable.
• SINDI and SINDIV2 now validate external parameters against a whitelist of supported flat keys, rejecting unknown fields early.
• CompatibilityReport / CollectCompatibilityIssues added as a structured alternative to CheckCompatibility, with test coverage.
• FP32QuantizerParameter::ToJson() now includes hold_molds, making the field round-trippable.
• Regression tests added for split config parsing/application and multi-difference compatibility reporting.

Previously reported issues that have been addressed:

• STORE_RAW_VECTOR now correctly writes hold_molds to both base_codes.quantization_params and precise_codes.quantization_params in BruteForce mapping.
• size_t replaced with uint64_t in CollectCompatibilityIssues array traversal.
• Unused fp32_quantizer_parameter.h and rabitq_quantizer_parameter.h includes removed from hgraph_param_mapping.cpp.

One remaining item (previously flagged by Copilot, not yet addressed):

• FlattenDataCellParameter::CreateDefault accepts hold_molds but only applies it when the quantizer is FP32. INT8 quantizers also parse hold_molds, so calling CreateDefault("int8", ..., true) would silently drop the flag. Current callers all use FP32, so this is not a live bug, but worth addressing for interface correctness.

Behavior change note (intentional, documented in PR description):

• codes_type=rabitq_split now requires the nested RaBitQ quantizer to already have rabitq_version=split. Inconsistent configurations are rejected with CHECK_ARGUMENT instead of being silently mutated. This is a deliberate cleanup that removes downstream mutation from FlattenDataCellParameter::FromJson.

No blocking issues found. The refactoring is clean and well-tested (713 unit tests passing, clang-format/clang-tidy clean).

[LHT129]

经过全面审查（commit 3035744），这个 PR 质量很高。重构将 JSON 字符串模板替换为结构化 builder 函数，提升了可读性和类型安全。

审查维度：

• 正确性 ✅：参数映射逻辑与原有模板行为一致。supported_keys 白名单校验（SINDI/SINDI v2）增强了健壮性。CollectCompatibilityIssues 实现正确，JSON 递归比较逻辑完整。
• 风格 ✅：CreateDefault 工厂方法模式统一了各参数类型的构造方式。显式 if-else 链比 ConstParamMap 更直观。
• 测试 ✅：已有测试覆盖参数映射路径。
• 性能 ✅：build_default_* 函数在初始化时调用一次，非热路径。CollectCompatibilityIssues 中的 std::function 递归 lambda 开销可接受（非热路径）。
• 安全性 ✅：RaBitQSplitConfig 的 ParseRaBitQSplitConfig 正确验证了 bit 范围 [1,8] 和 total <= 8。ValidateMRLEDim 验证了 mrle_dim 在 [0, dim] 范围内。
• 可维护性 ✅：移除 ConstParamMap 和 DEFAULT_MAP（116 行）消除了隐式依赖。结构化 builder 使得添加新参数类型更清晰。

之前 Copilot 发现的 STORE_RAW_VECTOR 映射错误、size_t 使用和未使用的 includes 都已在最新 commit 中修复。未发现新的阻塞性问题。