Skip to content

[MOD-11888] Return descriptive errors from VecSimIndex_ResolveParams - #1051

Open
alonre24 wants to merge 2 commits into
mainfrom
MOD-11888-vecsim-resolve-params-error-messages
Open

alonre24 wants to merge 2 commits into
mainfrom
MOD-11888-vecsim-resolve-params-error-messages

Conversation

@alonre24

@alonre24 alonre24 commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Query-time parameter rejections (e.g. EPSILON, EF_RUNTIME, RERANK, hybrid policy) only ever surfaced a bare VecSimResolveCode, giving callers no way to explain why a specific parameter or value was rejected (e.g. the classic "Error parsing vector similarity parameters: Invalid option" with no further detail).
  • VecSimIndex_ResolveParams gains an optional const char **err_msg out-param; each _ResolveParams_* helper now returns through a new SetResolveErr() helper that formats a specific message (e.g. "EPSILON is only valid for HNSW or SVS indexes") into a thread-local buffer instead of a bare error code.
  • Adds a .claude/skills/vecsim-error-messages/SKILL.md documenting the pattern for future additions.

Index-creation-time error reporting (VecSimIndex_New returning NULL with no message) was considered under the same ticket but is descoped from this PR.

Test plan

  • Existing unit tests (test_common, test_bruteforce, test_hnsw, test_svs) updated for the new VecSimIndex_ResolveParams signature and pass on CI
  • check-format.sh / clang-format clean
  • Follow-up (tracked separately under MOD-11888): wire RediSearch's VecSim_ResolveQueryParams to forward these messages into the client-facing error, and update RediSearch's tests accordingly

🤖 Generated with Claude Code


Note

Medium Risk
Breaking C API signature change for VecSimIndex_ResolveParams requires all callers to pass the new err_msg argument; query-param validation behavior is unchanged aside from richer optional messages.

Overview
Query-time VecSim parameter resolution now exposes why a runtime param failed, not only a VecSimResolveCode.

VecSimIndex_ResolveParams gains an optional const char **err_msg out-parameter (documented in vec_sim.h). Rejection paths in vec_sim.cpp go through a new SetResolveErr() helper that formats into a thread-local 256-byte buffer; passing nullptr for err_msg remains safe. Messages name params via VecSimCommonStrings::*_STRING and describe constraints (wrong index type, query type, duplicate param, bad value, unknown param, hybrid policy conflicts). HYBRID_POLICY invalid values now get a message listing expected batches / adhoc_bf instead of a bare code.

A .claude/skills/vecsim-error-messages/SKILL.md skill documents the pattern for future rejections. Unit tests (test_common, test_bruteforce, test_hnsw, test_svs) pass the new argument and assert specific err_msg strings where relevant.

Not in this PR: RediSearch still maps resolve codes to canned client errors and does not forward err_msg (noted as MOD-11888 follow-up).

Reviewed by Cursor Bugbot for commit d1c3eaa. Bugbot is set up for automated code reviews on this repo. Configure here.

Query-time parameter rejections (EPSILON, EF_RUNTIME, RERANK, hybrid
policy, etc.) only ever returned a bare VecSimResolveCode, so callers
had no way to explain to users why a specific parameter/value was
rejected. Add an optional err_msg out-param that each resolver helper
fills with a message describing the actual constraint that failed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.40506% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.52%. Comparing base (5d9c4fd) to head (d1c3eaa).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/VecSim/vec_sim.cpp 92.40% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1051      +/-   ##
==========================================
+ Coverage   97.40%   97.52%   +0.11%     
==========================================
  Files         141      141              
  Lines        8686     9050     +364     
==========================================
+ Hits         8461     8826     +365     
+ Misses        225      224       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The previous commit threaded the new err_msg out-param through every
existing test call site but always passed nullptr, so nothing
actually exercised SetResolveErr()'s message content. Capture it for
a representative case per resolver (unknown param, already-set, bad
value, invalid policy, hybrid-adhoc interactions) and assert on the
exact text.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant