Repository navigation
fix: preserve rewritten FunctionChain scores in reduction and fusion - #55232
junjiejiangjjj wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
mergify
Bot
added
dco-passed
|
[ci-v2-notice] To rerun ci-v2 checks, comment with:
If you have any questions or requests, please contact @zhikunyao. |
❌ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 9.4min | - |
| ❌ Code-Check | FAILURE | 4.6min | - |
| ✅ UT-Integration | SUCCESS | 42.1min | - |
| ❌ UT-GO | FAILURE | 34.4min | - |
| ✅ UT-CPP-Cov | SUCCESS | 27.2min | 23007 total, 23007 passed, 0 failed |
Total: 62min | Pipeline | Artifacts
Overall Coverage: 79.0%
Diff Coverage: Go 97.0% (65 hit, 2 miss, 67 measurable lines, 25 unmeasured)
Diff Coverage HTML: view changed lines
Total Patch Coverage: 97.0% (65/67 measurable lines, 25 unmeasured)
Failed Test Logs:
- Code-Check: download log archive (
code-check.loginside; index) - UT-GO: download log archive (
ut-go-test.loginside; index)
Skip metric-based distance restoration when L0/L1 chains write . Preserve rewritten scores during HybridSearch fusion and treat them as larger-is-better, while retaining existing behavior for raw metric scores. Add Go regression tests and Python e2e cases covering L2 score rewriting, group-by, pagination, and normalized and unnormalized hybrid fusion. issue: milvus-io#55212 Signed-off-by: junjie.jiang <junjie.jiang@zilliz.com>
9b2203a to
5d306b7
Compare
liliu-z
left a comment
liliu-z
left a comment
There was a problem hiding this comment.
The direction looks right to me. Once an L0/L1 chain writes $score, the value is no longer a distance, so Proxy shouldn't negate it back and Merge shouldn't treat it as one.
My main concern: the reduce ops' metrics output has three consumers (rerank, element_best_collapse, agg), and only rerank learns about rewritten scores. On the other two paths, results that used to have the right order and the wrong sign now come back in the wrong order. Details inline.
| offset = v | ||
| } | ||
| return &searchReduceOperator{ | ||
| scoreRewritten: chain.QueryNodeChainsWriteScore(t.request.GetFunctionChains()), |
There was a problem hiding this comment.
search_aggregation still takes the score direction from the metric. L0 is allowed with it (only L1 is rejected), and aggOp builds SearchAggregationComputer from metrics[0], so for L2, top_hits sorts _score ascending. With an L0 chain writing $score, the scores that reach it are now larger-is-better, so top_hits returns the worst hits first and size keeps the wrong ones. Before this change the order was right and only the sign was off.
| return nil, err | ||
| } | ||
| return &hybridSearchReduceOperator{ | ||
| scoresRewritten: t.subSearchScoresRewritten(), |
There was a problem hiding this comment.
Same problem in hybrid search. element_best_collapse runs on this output before rerank and picks each row's best element with metric.PositivelyRelated(metrics[i]). Take a hybrid search that mixes an element-level struct sub-search on L2 (whose L0/L1 chain writes $score) with a row-level one: collapse now keeps the lowest business score per row.
Instead of re-deriving the flag in rerankOperator and threading ScoresRewritten through SearchParams / SearchRuntimeInfo / MergeOption, could the reduce ops emit the effective direction together with metrics? Then all three consumers read the same thing.
| if normalize { | ||
| for i, m := range metricTypes { | ||
| normFuncs[i] = getNormalizeFunc(m) | ||
| if !isScoreRewritten(rewritten, i) { |
There was a problem hiding this comment.
This also changes norm_score=true for IP/COSINE/BM25 sub-searches that rewrite $score, which #55212 didn't affect. These scores used to be mapped into [0,1]. Now they keep their raw scale, so a business score like 6*ts swamps the normalized vector scores, even though the user explicitly asked for normalization.
Would it be more consistent to treat a rewritten input as a generic larger-is-better score and apply the IP-style 0.5 + atan(x)/π? That still fixes the L2 sign problem, and norm_score keeps meaning "all inputs in [0,1]".
|
|
||
| Proxy derives whether each sub-search wrote `$score` from that sub-search's | ||
| L0/L1 outputs and passes the state to Merge through runtime build context. | ||
| Rewritten scores are larger-is-better business values: Merge preserves their |
There was a problem hiding this comment.
One case worth spelling out here: for L2, L0/L1 read $score as the negated distance (segcore flips it in segment_c.cpp). So a chain that adjusts the score rather than replacing it, e.g. $score * 1.2, used to return 1.2d and now returns -1.2d. If that's the intended contract, could we document it? The API doc (20260624) only says $score starts from the search score/distance, which reads like a positive L2 distance.
| } | ||
|
|
||
| func reduceResults(ctx context.Context, toReduceResults []*internalpb.SearchResults, nq, topK, offset int64, metricType string, pkType schemapb.DataType, queryInfo *planpb.QueryInfo, isAdvance bool, isSearchAggregation bool, collectionID int64, partitionIDs []int64) (*milvuspb.SearchResults, error) { | ||
| func shouldRestoreDistance(metricType string, scoreRewritten []bool) bool { |
There was a problem hiding this comment.
nit: scoreRewritten ...bool is passed as an optional arg through four functions, and only [0] is ever read. A plain bool parameter, or a field on reduce.ResultInfo next to the metric type, would be easier to follow.
✅ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 10.0min | - |
| ✅ Code-Check | SUCCESS | 5.1min | - |
| ✅ UT-Integration | SUCCESS | 37.3min | - |
| ✅ UT-GO | SUCCESS | 34.2min | - |
| ✅ UT-CPP-Cov | SUCCESS | 27.0min | 23007 total, 23007 passed, 0 failed |
Total: 59min | Pipeline | Artifacts
Overall Coverage: 79.0%
Diff Coverage: Go 97.0% (65 hit, 2 miss, 67 measurable lines, 25 unmeasured)
Diff Coverage HTML: view changed lines
Total Patch Coverage: 97.0% (65/67 measurable lines, 25 unmeasured)
Skip metric-based distance restoration when L0/L1 chains write .
Preserve rewritten scores during HybridSearch fusion and treat them as
larger-is-better, while retaining existing behavior for raw metric scores.
Add Go regression tests and Python e2e cases covering L2 score rewriting,
group-by, pagination, and normalized and unnormalized hybrid fusion.
issue: #55212