refactor(autotune): make candidate choices explicit

[jac0626]

Change Type

• Bug fix
• New feature
• Improvement/Refactor
• Documentation
• CI/Build/Infra

Linked Issue

• Fixes: [improve](eval_performance): distinguish native arrays from candidate choices #2727

What Changed

• Treat bare JSON arrays as exact native index parameter values.
• Add explicit {"$choices": [...]} candidate expansion with validation, atomic alternatives,
  and duplicate removal while preserving existing $range behavior.
• Migrate built-in proposals, runnable examples, regression tests, and the English and Chinese
  AutoTune documentation to the unambiguous syntax.

Test Evidence

• make fmt
• make lint
• make test
• make cov, run tests, and collect coverage
• Other (describe below)

Test details:

clang-format-15 --dry-run --Werror <changed C++ files>
git diff --check
jq empty tools/autotune/examples/sift_hgraph_autotune_request.json
cmake --build build-release --target autotune_test --parallel 96
./build-release/tools/autotune/autotune_test --reporter compact
  All tests passed (265 assertions in 26 test cases)
./build-release/examples/cpp/326_feature_create_index_with_constraints
./build-release/examples/cpp/327_feature_autotune_existing_index
./build-release/examples/cpp/328_feature_autotune_existing_pyramid
  All three examples completed successfully with 2, 3, and 5 evaluated trials.

Compatibility Impact

• API/ABI compatibility: No C++ API or ABI change; AutoTune remains an experimental build-tree API.
• Behavior changes: Bare arrays are now literal parameter values. Discrete candidates must use
  $choices. This intentionally replaces the ambiguous experimental grammar before adoption.

Performance and Concurrency Impact

• Performance impact: None; candidate expansion complexity and trial planning are unchanged.
• Concurrency/thread-safety impact: None.

Documentation Impact

• No docs update needed
• Updated docs:
  • README.md
  • DEVELOPMENT.md
  • CONTRIBUTING.md
  • Other: English/Chinese AutoTune guide and API contract, tool example README

Risk and Rollback

• Risk level: Low.
• Rollback plan: Revert the commit to restore the previous experimental array expansion behavior.

Checklist

• I have linked the relevant issue (required for kind/bug and kind/feature; see "Linked Issue" above)
• I have added/updated tests for new behavior or bug fixes
• I have considered API compatibility impact
• I have updated docs if behavior/workflow changed
• My commit messages follow project conventions (Conventional Commits, optional [skip ci] prefix)

[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 experimental AutoTune JSON candidate-expression grammar to remove ambiguity: bare JSON arrays are now treated as literal native parameter values, while discrete candidate expansion requires an explicit {"$choices":[...]} expression (with validation and deduplication), preserving existing $range behavior. It migrates built-in proposals, examples, tests, and the English/Chinese AutoTune docs to the new explicit syntax.

Changes:

• Update candidate expansion to treat arrays as concrete values and add explicit $choices expansion with validation + duplicate removal.
• Migrate built-in proposal generation, examples, and C++ regression tests to the $choices syntax.
• Update English/Chinese AutoTune documentation and API contract to match the unambiguous grammar.

Reviewed changes

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

Show a summary per file

File	Description
tools/autotune/autotune_candidate.cpp	Implements explicit $choices expansion/validation and changes array handling to literal pass-through.
tools/autotune/autotune_test.cpp	Migrates tests to $choices and adds regression coverage for literal arrays vs choices + validation rules.
tools/autotune/autotune.h	Updates public header comments to describe the updated parameter-space grammar.
tools/autotune/examples/sift_hgraph_autotune_request.json	Migrates example request from bare arrays to explicit $choices.
tools/autotune/examples/README.md	Updates example documentation to reference $choices and clarify bare-array behavior.
examples/cpp/326_feature_create_index_with_constraints.cpp	Updates sample code to use $choices for discrete search candidates.
examples/cpp/327_feature_autotune_existing_index.cpp	Updates sample code to use $choices for discrete search candidates.
examples/cpp/328_feature_autotune_existing_pyramid.cpp	Updates sample code to use $choices for discrete search candidates.
docs/docs/en/src/resources/autotune.md	Syncs user guide with explicit $choices and literal array semantics.
docs/docs/en/src/resources/autotune_api_v1.md	Updates API contract to define $choices and literal arrays unambiguously.
docs/docs/zh/src/resources/autotune.md	Syncs Chinese user guide with explicit $choices and literal array semantics.
docs/docs/zh/src/resources/autotune_api_v1.md	Updates Chinese API contract to define $choices and literal arrays unambiguously.

---

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

[LHT129]

Overall this is a clean refactor that makes the AutoTune candidate grammar unambiguous. The code changes are well-tested with comprehensive validation coverage. I have one suggestion for the SearchRequest::parameter_space comment to align it with the IndexSpace documentation.

[suggestion] tools/autotune/autotune.h:106 — The SearchRequest::parameter_space comment mentions only $choices and $range, but the new grammar also allows concrete JSON values (including bare arrays) to pass through unchanged. The IndexSpace comments in the same header correctly describe this. Consider aligning this comment to avoid confusing API consumers.

Current:

/// JSON search space with explicit $choices or $range candidate expressions; missing supported
/// fields receive built-in proposals.

Suggested:

/// JSON search space with concrete values, explicit $choices, or $range candidate expressions;
/// missing supported fields receive built-in proposals. Bare arrays are concrete values.

[LHT129]

[suggestion] The SearchRequest::parameter_space comment mentions only $choices and $range, but the new grammar also allows concrete JSON values (including bare arrays) to pass through unchanged. The IndexSpace comments in the same header correctly describe this. Consider aligning this comment with the IndexSpace documentation to avoid confusing API consumers.

[LHT129]

[suggestion] The SearchRequest::parameter_space comment mentions only $choices and $range, but the new grammar also allows concrete JSON values (including bare arrays) to pass through unchanged. The IndexSpace comments in the same header correctly describe this. Consider aligning this comment with the IndexSpace documentation to avoid confusing API consumers.

Suggested:

/// JSON search space with concrete values, explicit $choices, or $range candidate expressions;
/// missing supported fields receive built-in proposals.

[LHT129]

[note] The expand_choices function uses choice.dump() for deduplication, which serializes each candidate value to a string on every iteration. This is correct and handles arbitrary JSON types uniformly, but it means every candidate — even simple integers — incurs a full JSON serialization. Since candidate counts are typically small (bounded by max_trials), this is unlikely to be a practical concern. Just noting for awareness.

[LHT129]

[note] When first == second (i.e., base_count <= 1024), buckets_count is set to a plain scalar instead of {"$choices": [...]}. This is correct — a single value doesn't need candidate expansion — but it means the field's JSON type differs depending on the dataset size. This is fine since the expand() function handles scalars, objects, and arrays uniformly. Just noting that callers inspecting the raw JSON shouldn't assume buckets_count is always wrapped in $choices.

[LHT129]

Thanks for this PR. The $choices syntax is a clean improvement over the implicit bare-array-as-candidates behavior — it makes the grammar unambiguous and properly separates "literal native parameter value" from "discrete candidate expansion."

Summary of changes:

• expand() now treats bare arrays as literal values (pass-through), and only $choices / $range trigger candidate expansion.
• New expand_choices() function with validation (non-empty array, no mixed keys) and duplicate removal via choice.dump().
• Built-in proposals (fill_* functions) now emit $choices wrappers instead of bare arrays.
• Tests, examples, and docs updated consistently.

Review comments posted:

1. autotune.h:106 — SearchRequest::parameter_space comment should mention concrete values (not just $choices/$range), matching IndexSpace.
2. autotune_candidate.cpp:167 — choice.dump() for dedup is correct but worth noting as a serialization cost per element.
3. autotune_candidate.cpp:238 — fill_ivf_create scalar fallback when first == second is correct but callers shouldn't assume buckets_count is always wrapped.

All three are [note]/[suggestion] level — no blocking issues found. The tests are thorough (validation, dedup, native array pass-through, array-of-arrays choices).

[LHT129]

[suggestion] The choice.dump() deduplication relies on nlohmann::json::dump() preserving key insertion order. Two semantically identical JSON objects with different key insertion orders would produce different serialized strings and thus NOT be deduplicated, potentially yielding duplicate candidates.

For example:

{"$choices": [{"a": 1, "b": 2}, {"b": 2, "a": 1}]}

would produce two candidates with identical semantics.

In practice, most $choices items are scalars (integers/strings) where this is not an issue, and object-valued choices typically come from a single construction path with deterministic ordering. However, if users ever construct $choices with object items from different sources, duplicates could leak through.

Consider sorting keys before serialization, or using a canonicalization approach for object-valued choices.

[LHT129]

Reviewed the full diff and all core source files. This is a clean refactor that makes the AutoTune candidate grammar unambiguous.

Summary of changes:

• expand() now treats bare arrays as literal pass-through values; only $choices / $range trigger candidate expansion.
• New expand_choices() with validation (non-empty array, no mixed keys) and duplicate removal via choice.dump().
• Built-in proposals (fill_* functions) emit $choices wrappers via the candidate_choices() helper.
• Tests, examples, and English/Chinese docs updated consistently with thorough coverage (validation, dedup, native array pass-through, array-of-arrays choices).

Review comments already posted by other reviewers cover:

1. autotune.h:106 — SearchRequest::parameter_space comment should mention concrete values alongside $choices/$range, matching IndexSpace documentation.
2. autotune_candidate.cpp:167 — choice.dump() for dedup incurs per-element serialization cost (acceptable given small candidate counts).
3. autotune_candidate.cpp:167 — choice.dump() may not deduplicate semantically identical objects with different key insertion orders.
4. autotune_candidate.cpp:238 — fill_ivf_create scalar fallback when first == second produces a type-inconsistent buckets_count field.

No additional blocking issues found. The test coverage is thorough (265 assertions in 26 test cases), and the migration is complete across all built-in proposals, examples, and documentation.

[LHT129]

I have completed a thorough review of this PR. The existing 6 inline comments from LHT129 and Copilot already cover the substantive findings:

1. SearchRequest::parameter_space comment alignment (LHT129): The comment should mention concrete values alongside $choices/$range, matching the IndexSpace comments. A suggested fix was provided.

2. choice.dump() deduplication performance (LHT129): Serializing every candidate for dedup is correct but noted for awareness — not a practical concern given small candidate counts.

3. choice.dump() key-ordering edge case (LHT129): Object-valued $choices items with different key insertion orders would not be deduplicated. This is a corner case unlikely in practice but worth addressing for robustness.

4. buckets_count scalar vs $choices inconsistency (LHT129): When first == second, the field is a plain scalar instead of {"$choices": [...]}. This is correct behavior but noted for callers inspecting raw JSON.

My assessment:

• Core logic (expand(), expand_choices(), candidate_choices()): Clean and correct. The separation of bare arrays (concrete values) from $choices (candidate expansion) is well-implemented.
• Validation: Good coverage — empty $choices, non-array $choices, mixed keys, and duplicate removal are all validated with clear error messages.
• Tests: Two new test cases (AutoTune distinguishes native arrays from explicit choices, AutoTune validates explicit choices) plus updates to existing tests. The tests cover bare array passthrough, empty array passthrough, $choices with array-valued items, validation error paths, and deduplication.
• Documentation: All 4 doc files (EN/ZH autotune.md and autotune_api_v1.md) are consistently updated with the new $choices syntax and bare-array semantics.
• Examples: All 3 C++ examples and the JSON example are updated to use the new syntax.
• Backward compatibility: This is a breaking change for the experimental AutoTune API, but the PR description acknowledges this and the change is intentional before broader adoption.

No new blocking issues found beyond what is already covered by the existing comments. The PR is well-structured and the test evidence in the PR description is thorough.

[LHT129]

I have reviewed this PR thoroughly. The refactoring is well-designed and the implementation is clean.

Summary of changes:

• Bare JSON arrays are now treated as native index parameter values (passed through unchanged).
• Explicit {"$choices": [...]} syntax is introduced for discrete candidate expansion.
• All built-in proposals, examples, tests, and documentation are migrated to the new syntax.
• Validation is added: $choices must be an array, non-empty, and not mixed with other keys.
• Duplicate removal is handled via choice.dump() serialization.

What I checked:

• The expand() function correctly distinguishes bare arrays (emit directly) from $choices/$range objects (expand).
• expand_object() recursion still works correctly for nested objects containing $choices/$range.
• The candidate_choices() helper consistently wraps all built-in proposals.
• Test coverage is thorough: native array passthrough, $choices validation, duplicate removal, and migration of existing tests.
• Documentation (EN/ZH) accurately reflects the new grammar.

No blocking issues found. The existing inline comments from other reviewers about the SearchRequest::parameter_space docstring and choice.dump() deduplication edge cases are reasonable suggestions but non-blocking.

This is a clean, well-tested refactor that makes the AutoTune grammar unambiguous. Nice work.