fix: [2.6] wire up IndexStaticFaced::ConfigCheck dispatch#1573
Merged
sre-ci-robot merged 2 commits intoApr 10, 2026
Conversation
…#1570) Cherry-pick of the main-branch fix for zilliztech#1570. The SFINAE probe used to register per-index StaticConfigCheck functions into staticConfigCheckMap was permanently false because the probe's function-type template parameter (decltype(InternalConfigCheck)) declared the second arg as IndexVersion while every real StaticConfigCheck impl takes PARAM_TYPE. int32_t does not implicitly convert to the unscoped PARAM_TYPE enum, so has_static_StaticConfigCheck was false for every index, the if constexpr branch in RegisterStaticFunc never instantiated, and IndexStaticFaced::ConfigCheck always fell through to Status::success. Fix: - Align InternalConfigCheck declaration/definition to take PARAM_TYPE. - Pass PARAM_TYPE::TRAIN at the dispatch site (ConfigCheck is a build-time pre-check; TRAIN is the only paramType the existing impls act on). - Change the float/binary classification in IvfIndexNode::StaticConfigCheck and HnswIndexNode::StaticConfigCheck from KnowhereFloatTypeCheck<DataType> to !std::is_same_v<DataType, knowhere::bin1>. KnowhereFloatTypeCheck excludes int8/fp16/bf16, but those types are mocked to fp32 at runtime via MockData<>::type and should accept float metrics — without this companion change, fixing the dispatch alone would regress IndexStaticFaced<int8>::ConfigCheck("IVF_FLAT", L2) which the existing test_config.cc::checkBuildConfig already exercises. Add tests/ut/test_config.cc cases that pin the dispatch path: fp32+IVF_FLAT+HAMMING -> invalid_metric_type, bin1+BIN_IVF_FLAT+HAMMING -> success, int8+IVF_FLAT+L2 -> success, int8+IVF_FLAT+JACCARD -> invalid_metric_type. issue: zilliztech#1570 Signed-off-by: xianliang.li <xianliang.li@zilliz.com>
|
@foxspy 🔍 Important: PR Classification Needed! For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:
For any PR outside the kind/improvement category, ensure you link to the associated issue using the format: “issue: #”. Thanks for your efforts and contribution to the community!. |
mergify
Bot
added
do-not-merge/missing-related-issue
and removed
do-not-merge/missing-related-issue
labels
mergify
Bot
added
do-not-merge/missing-related-issue
and removed
do-not-merge/missing-related-issue
labels
This was referenced Apr 9, 2026
…ticConfigCheck Cherry-pick of the main-branch follow-up to PR zilliztech#1571 addressing @SpadeA-Tang's review. With the dispatch wired up by the previous commit, IvfIndexNode/ HnswIndexNode/IndexNodeWithDataViewRefiner's StaticConfigCheck would now reject compound emb-list metrics like MAX_SIM_IP for indexes whose registration includes the EMB_LIST feature (IVFFLAT, IVF_FLAT, IVFFLATCC, IVF_FLAT_CC, SCANN_DVR, etc.). The metric whitelist {L2, IP, COSINE} / {HAMMING, JACCARD} only matches the base metric names, but production callers pass MAX_SIM_IP / MAX_SIM_HAMMING / MAX_SIM_COSINE (etc.) and AddEmbList decomposes them internally via get_sub_metric_type before handing the base metric to faiss. Apply the same decomposition in StaticConfigCheck: if the metric is a known compound (MAX_SIM_*, DTW_*), use its underlying base metric for the float/binary classification; otherwise fall back to the raw value. Add positive/negative tests in tests/ut/test_config.cc. issue: zilliztech#1570 Signed-off-by: xianliang.li <xianliang.li@zilliz.com>
Collaborator
|
wait for #1571 |
Collaborator
|
/lgtm |
marcelo-cjl
approved these changes
Apr 10, 2026
Collaborator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: foxspy, marcelo-cjl The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
issue: #1570
Cherry-pick of #1571 to 2.6.
Summary
InternalConfigChecksignature to takePARAM_TYPEso the SFINAE probe inRegisterStaticFuncactually matches every index's realStaticConfigCheckimpl. Previously the probe usedIndexVersion(int32_t), which does not implicitly convert to the unscopedPARAM_TYPEenum, sostaticConfigCheckMapwas permanently empty andIndexStaticFaced::ConfigCheckalways fell through toStatus::success.PARAM_TYPE::TRAINat the dispatch site (index_static.cc:60).IvfIndexNode::StaticConfigCheckandHnswIndexNode::StaticConfigCheck: classify the float vs binary metric branch with!std::is_same_v<DataType, knowhere::bin1>instead ofKnowhereFloatTypeCheck<DataType>. The latter excludesint8/fp16/bf16, but those are mocked tofp32at runtime viaMockData<>, so they should accept float metrics — without this companion change, fixing the dispatch alone would regressIndexStaticFaced<int8>::ConfigCheck("IVF_FLAT", L2), whichtests/ut/test_config.cc::checkBuildConfigalready exercises.tests/ut/test_config.ccthat pin the dispatch path:fp32+IVF_FLAT+HAMMING→invalid_metric_type,bin1+BIN_IVF_FLAT+HAMMING→success,int8+IVF_FLAT+L2→success,int8+IVF_FLAT+JACCARD→invalid_metric_type.Test Plan
pre-commit runclean on all 5 changed files