Skip to content

fix(milvus): only apply cosine-scale radius threshold when the collection metric is COSINE - #3980

Open
Shizoqua wants to merge 2 commits into
HKUDS:mainfrom
Shizoqua:fix/milvus-radius-threshold-metric-mismatch
Open

Shizoqua wants to merge 2 commits into
HKUDS:mainfrom
Shizoqua:fix/milvus-radius-threshold-metric-mismatch

Conversation

@Shizoqua

Copy link
Copy Markdown
Contributor

Summary

Fixes silently broken retrieval on Milvus collections configured with L2 or IP distance metric.

Problem

Query threshold filtering passed the raw cosine_better_than_threshold straight through as Milvus's radius search param regardless of the collection's own metric type.

Changes

Radius filtering is now only applied when the collection metric is COSINE, matching the documented meaning of the threshold.

Testing

Added regression tests covering COSINE, L2, and IP metric types. Ran the full milvus test suite, all passing.

Compatibility and Risk

No API changes. COSINE collections (the default) are unaffected; L2/IP collections stop applying a wrong-scale filter.

Related Issue

No related issue.

…tion metric is COSINE

Signed-off-by: Lanre Shittu <136805224+Shizoqua@users.noreply.github.com>
# scale instead of the intended similarity cutoff. Only apply it when
# the collection's own metric is COSINE, where the semantics match.
search_params_inner = dict(search_params_base.get("params", {}))
if self.index_config.metric_type == "COSINE":

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read this against milvus_impl.py's other query paths and the sibling backends: faiss_impl.py:522, nano_vector_db_impl.py:890 and qdrant_impl.py:823 all apply cosine_better_than_threshold unconditionally, regardless of the store's own distance metric (qdrant's collection is even hardcoded to Distance.COSINE).

__post_init__ here (lines 2430-2435) still requires cosine_better_than_threshold for every MILVUS_METRIC_TYPE, including L2 and IP: construction raises if it's missing. After this fix, a collection configured with MILVUS_METRIC_TYPE=L2 or IP is still forced to supply that threshold, and this code path now silently never applies it. Every other backend enforces the same threshold regardless of its own metric, so Milvus becomes the one backend where a required parameter can be dead configuration with zero signal to the operator.

A one-time logger.warning in __post_init__ when self.index_config.metric_type != "COSINE", right after self.cosine_better_than_threshold is set at line 2435, would at least tell the operator their threshold is being ignored for this collection instead of the current silent divergence from the rest of the retrieval stack.

I did not run this against a live Milvus instance, just traced the code paths and the new test file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the new commit: the warning at construction when metric_type is not COSINE covers the silent-mismatch case, and the two new tests exercise both branches.

…threshold unapplied

Signed-off-by: Lanre Shittu <136805224+Shizoqua@users.noreply.github.com>

This branch has not been deployed

No deployments
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.

2 participants