Implement base classes and enable fusion for hybrid query (limited to min_max and arithmetic mean) - #1933
Conversation
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
PR Reviewer Guide 🔍(Review updated until commit c95d889)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to c95d889 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 4f82009
Suggestions up to commit b9cbd23
Suggestions up to commit b9cbd23
Suggestions up to commit 457d8f6
Suggestions up to commit 457d8f6
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/fusion-hybrid-query #1933 +/- ##
=================================================================
+ Coverage 83.45% 83.52% +0.07%
- Complexity 3884 4025 +141
=================================================================
Files 291 297 +6
Lines 13819 14255 +436
Branches 2294 2387 +93
=================================================================
+ Hits 11532 11906 +374
- Misses 1454 1480 +26
- Partials 833 869 +36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fee3689 to
5659fe8
Compare
|
Persistent review updated to latest commit 5659fe8 |
5659fe8 to
d0c467b
Compare
|
Persistent review updated to latest commit d0c467b |
d0c467b to
f288716
Compare
|
Persistent review updated to latest commit f288716 |
c89ce8c to
99c7fe7
Compare
99c7fe7 to
eb1dbb8
Compare
|
Persistent review updated to latest commit eb1dbb8 |
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
eb1dbb8 to
e16cfa3
Compare
|
Persistent review updated to latest commit e16cfa3 |
|
Persistent review updated to latest commit 457d8f6 |
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
a74ff45 to
b9cbd23
Compare
|
Persistent review updated to latest commit a74ff45 |
|
Persistent review updated to latest commit b9cbd23 |
| * | ||
| * <p>This query is created internally by the coordinator self-erase and is never parseable from a search request. | ||
| */ | ||
| public class HybridFusionQuery extends AbstractQueryBuilder<HybridFusionQuery> { |
There was a problem hiding this comment.
I think this is a QueryBuilder rather than a Query. Can we rename it?
There was a problem hiding this comment.
makes sense, will rename it
| * this copy. The scoring {@link Weight}/iterator implementation is deferred to PR5, where the PIT/docId fast path is | ||
| * introduced — {@link #createWeight} intentionally throws until then. | ||
| */ | ||
| public final class FusedDocsScorerQuery extends Query { |
There was a problem hiding this comment.
Since we don't plan to use it in this PR maybe it's better to exclude it? And for the retriever framework we may also have a different implementation.
| * coordinator rewrite wiring lands with the execution path. | ||
| */ | ||
| @NoArgsConstructor(access = AccessLevel.PRIVATE) | ||
| public final class CoordinatorScoreFusion { |
There was a problem hiding this comment.
Design suggestion: separation of concerns in CoordinatorScoreFusion
Currently fuseMinMax() computes per-leg min/max bounds, normalizes scores, and combines them — all in one method. This works for min_max alone, but once z_score, l2, and RRF land on this path, we'd end up with separate fuseMinMax / fuseZScore / fuseL2 / fuseRRF methods each with their own inline stats-gathering or rank-computation loop.
What if we extract a per-leg transformation step? Something like:
interface ScoreTransformer {
List<Float> transform(List<Float> rawScores);
}Each implementation computes what it needs internally and returns values ready for combination:
| Technique | transform() does |
|---|---|
| min_max | compute min/max, apply (s-min)/(max-min) |
| z_score | compute mean/stddev, apply (s-mean)/stddev |
| l2 | compute L2 norm, apply s/norm |
| RRF | ignore score values, return 1/(k+rank) by position |
Then CoordinatorScoreFusion becomes technique-agnostic:
public static Map<String, Float> fuse(
List<Map<String, Float>> legRawScores,
ScoreTransformer transformer,
ScoreCombinationTechnique combiner
) { ... }This pairs ScoreTransformer with the existing ScoreCombinationTechnique — one transforms per-leg scores, the other combines across legs. Adding a technique means implementing the interface, no changes to the coordinator. It also mirrors how the classic pipeline already models RRF (normalization step replaces scores with reciprocals, combination step sums them) — just with a name that doesn't imply "normalization" for rank-based techniques.
Not blocking — the current code is correct. Just thinking about extensibility for the upcoming techniques.
There was a problem hiding this comment.
it's the same per-leg normalization seam we discussed for @vibrantvarun two-phase computeLegStats/normalize(score,stats) idea — let's converge on one abstraction rather than land two. Your single transform(List)→List is the better target: it generalizes to rank-based RRF (sort the leg, emit 1/(k+rank) by position), whereas the stats-split forces a per-score phase-2 that RRF can't satisfy.
This PR only supports min_max + arithmetic_mean (requireSupportedTechniques rejects the rest), so there's one technique today, I'd abstract in PR that lands the second scalar technique, keeping ScoreCombinationTechnique as the combine half.
| } catch (Exception e) { | ||
| listener.onFailure(e); | ||
| } | ||
| }, listener::onFailure) |
There was a problem hiding this comment.
Should we provide a better error message here since the error from multiSearch can be confusing since end users think they are doing hybrid query?
| for (QueryBuilder leg : legs) { | ||
| SearchSourceBuilder legSource = new SearchSourceBuilder().query(leg) | ||
| .size(windowSize) | ||
| .from(0) |
There was a problem hiding this comment.
Why from 0? Isn't the default behavior?
There was a problem hiding this comment.
Agree, legSource is a fresh SearchSourceBuilder whose from defaults to 0 at execution, so .from(0) is a no-op;
| * the shared core consumes; a leg that matched nothing contributes an empty map (groupLegHits fails fast on failures, | ||
| * so every slot is non-null). | ||
| */ | ||
| private static RankedDocs computeRankedDocs(SearchHit[][] legHits, FusionSpec fusion, int windowSize) { |
There was a problem hiding this comment.
Potential issue: _id collision in multi-index searches
In computeRankedDocs, leg hits are keyed purely by hit.getId():
Map<String, Float> byId = new LinkedHashMap<>();
for (SearchHit hit : hits) {
byId.put(hit.getId(), hit.getScore());
}For a single-index search this is fine — each _id is unique. But for multi-index searches (e.g., POST /index-a,index-b/_search), two different documents in different indices can share the same _id. In that case:
- Within a single leg's map, the second doc would overwrite the first (last-write-wins).
- Across legs, a doc from
index-ain leg 0 and a different doc fromindex-bin leg 1 would be treated as the same document during fusion — their scores would be combined as if they're one entity.
The fused HybridFusionQuery then uses IdsQueryBuilder with those ids, which would match both documents, and both would get the same fused score.
Should the key be _index + _id (or use hit.getIndex() + "#" + hit.getId()) to avoid conflation? The downstream IdsQueryBuilder in the Top would also need to become index-aware (or use a bool with terms on _id + filter on _index).
This may be an acceptable limitation for the initial scope if multi-index hybrid queries are uncommon, but worth documenting or validating against.
There was a problem hiding this comment.
agree that's a real bug
There was a problem hiding this comment.
Added validation that we are running query against a single index: https://github.com/martin-gaievski/neural-search/blob/feature/fusion-hybrid-query/src/main/java/org/opensearch/neuralsearch/query/HybridQueryBuilder.java#L586
| /** Legs whose Lucene match set is their own top-k (re-running them in the Tail = a redundant ANN pass). */ | ||
| private static boolean isMaterializableLeg(QueryBuilder leg) { | ||
| String name = leg.getWriteableName(); | ||
| return "knn".equals(name) || "neural".equals(name) || "neural_knn".equals(name); |
There was a problem hiding this comment.
why not use QueryBuilder name? what about neural_sparse?
There was a problem hiding this comment.
agree on naming, it should be safe to switch to QueryBuilder constant as we do have dependency on all these plugins.
neural_sparse should stay excluded. Materialization is only safe when a leg's full Lucene match set equals its returned top-k (true for HNSW kNN/neural). A neural_sparse leg's match set is every doc containing a query token — far larger than the window — so materializing to the window ids would drop the rest from the Tail and undercount total_hits/aggregations. Plus re-running it is cheap (no HNSW to re-walk); the check keys on getWriteableName() so it also correctly covers the seismic sparse-ANN variant (which keeps a lexical fallback). I'll expand the existing rationale comment at line 242 to make the "match-set == top-k" basis explicit.
There was a problem hiding this comment.
discussed offline, with latest code we are breaking default behavior, which is allow partial results (true setting, https://docs.opensearch.org/latest/install-and-configure/configuring-opensearch/search-settings/). If false is hardcoded that will fail the whole top query independently of actual setting, including default. Thus I'm reverting to initial version so we keep the default behavior. @vibrantvarun that something we discussed in #1933 (comment)
| return "knn".equals(name) || "neural".equals(name) || "neural_knn".equals(name); | ||
| } | ||
|
|
||
| private static boolean needsExecutionTail(SearchSourceBuilder source) { |
There was a problem hiding this comment.
Question: does the Tail actually help explain and profile?
needsExecutionTail includes explain and profile as reasons to include the Tail, but I'm not sure the Tail provides useful output for either:
explain: The user wants to understand how the fused score was computed (per-leg raw scores → normalization → combination). But the Tail puts the original legs inside a filter clause (non-scoring), so the explanation only shows "constant_score, boost=0.716" from the Top — not the per-leg breakdown. The actual fusion happened at the coordinator before the self-erased query was built, so it's not in the Lucene explain tree regardless of the Tail.
profile: The user wants to see where time was spent. With the Tail, profiling shows the shard re-executing the original legs inside the filter — which is redundant work (they already ran during the MultiSearch fan-out). It profiles the wrong thing: the second execution in the Tail, not the actual leg latency from the MultiSearch.
In both cases the Tail adds query-phase cost (re-executing legs) without providing the information the user is looking for. These might need a fusion-aware solution (e.g., storing the per-leg breakdown in the response) rather than the Tail.
Should explain and profile be removed from needsExecutionTail? Or is there a plan for fusion-aware explain/profile that would use the Tail differently?
There was a problem hiding this comment.
correct, Top is constant_score(ids)^fusedScore with min_max/arithmetic_mean already applied on the coordinator, so the Lucene tree has no fusion math to explain and the Tail is a non-scoring filter; for profile it re-executes legs that already ran in the fan-out. I'll drop explain and profile from needsExecutionTail. Safe: hits and fused scores come from the Top, and total_hits is still covered by wantsTotalsBeyondWindow; aggregations and highlight keep the Tail. Proper fusion-aware explain/profile is scoped to later PR.
| boolean topOnly; | ||
| if (topLevel == false) { | ||
| topOnly = true; // nested: enclosing filter intersects at the query phase | ||
| } else if (needsExecutionTail(source) || legsHaveInnerHits(legs)) { |
There was a problem hiding this comment.
Two issues:
1. needsExecutionTail doesn't cover all cases that need the Tail.
It checks aggregations, explain, profile, highlighter — but total_hits is handled separately by wantsTotalsBeyondWindow. If the method is called "needsExecutionTail" you'd expect it to be the single source of truth for "do we need the Tail?". Instead, the decision is split across three conditions. It would be clearer as one method that returns the full decision.
2. legsHaveInnerHits doesn't actually require the Tail.
extractInnerHitBuilders reads from sourceQueries (the field on HybridFusionQuery), not from the Tail's filter clause. The fetch phase registers inner_hits contexts from extractInnerHitBuilders and does its own per-doc lookup. The Tail re-executing the nested query during query phase is wasted work — the fetch phase doesn't use that result.
So legsHaveInnerHits being a reason for the Tail seems incorrect. The sourceQueries list just needs to be populated (which it always is when we pass the legs to HybridFusionQuery), but it doesn't need to be in the executed filter.
There was a problem hiding this comment.
(1) Agreed, I'll consolidate needsExecutionTail / legsHaveInnerHits / wantsTotalsBeyondWindow into one needsTail(...); they're already OR'd so it's a readability win.
(2) You're right on the mechanics: inner_hits are computed in the fetch phase per parent doc from the registered InnerHitContextBuilder's own query, so re-running the nested leg in the query-phase Tail is wasted. That catch is: sourceQueries isn't always populated — it's the same list that backs the Tail filter and is empty when we go Top-only, so I can't just drop legsHaveInnerHits or extractInnerHitBuilders would register nothing. Clean fix decouples the inner-hit registration source (always carry the leg builders) from the executed Tail (built only for aggs/totals/highlight), with a regression test — and should also cover the nested-fused case, where inner_hits are actually dropped today. Since inner_hits are correct now and this is an optimization, I'd land it as a focused follow-up.
| * </ol> | ||
| */ | ||
| @NoArgsConstructor(access = AccessLevel.PRIVATE) | ||
| final class FusionConfigResolver { |
There was a problem hiding this comment.
Questions on FusionConfigResolver
1. Inline body pipeline is silently broken
The comment in Step 2 acknowledges that core's resolvePipeline drains searchPipelineSource() before query rewrite runs, so fromPipelineConfig({}) returns null and the request fails with "no fusion config found." The user sees an error but it's confusing — they DID specify a pipeline with a normalization processor in the body. The error doesn't explain that inline body pipelines are unsupported for fused mode.
Could we detect this case earlier (e.g., at parse time if fusion: "pipeline" coexists with an inline search_pipeline block) and throw a clear error like "inline body search pipelines are not supported with fused mode; use a named pipeline or specify the config directly in the fusion block"?
2. Named pipeline doesn't exist — misleading error
If ?search_pipeline=my-pipeline references a pipeline that doesn't exist in cluster state, pipelineConfigById returns null → the error says "requires a normalization or score-ranker processor in the attached search pipeline." The actual problem is the pipeline doesn't exist, not that it's missing a processor. A more specific error ("pipeline 'my-pipeline' not found") would help debugging.
There was a problem hiding this comment.
On (1) you're right: core drains our inline search_pipeline map before coordinator rewrite, so we only see {} and emit the misleading "none found." Since searchPipelineSource() is still non-null when a block was attached, I'll turn that into a targeted rewrite-time error; genuinely supporting an inline body needs the companion core fix (deep-copy the config in resolvePipeline), which I'll track as a follow-up.
On (2), though, a genuinely missing named or index-default pipeline id doesn't reach this code: core's resolvePipeline throws "Pipeline [x] is not defined" (before rewriteAndFetch), so the user already gets a clear error. Our generic message only fires when the pipeline exists but has no fusion processor (or the drained-inline case), so a "not found" branch would be unreachable. I'll instead sharpen the wording to say the resolved pipeline has no normalization/score-ranker processor.
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
|
Persistent review updated to latest commit 4f82009 |
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
|
Persistent review updated to latest commit c95d889 |
| public static final Version MINIMAL_SUPPORTED_VERSION_METRICS_STATS = Version.V_3_3_0; | ||
| private static final Version MINIMAL_SUPPORTED_VERSION_NEURAL_KNN_QUERY_BUILDER = Version.V_3_0_0; | ||
| private static final Version MINIMAL_SUPPORTED_VERSION_AGENTIC_EMBEDDING_MODEL_ID = Version.V_3_6_0; | ||
| public static final Version MINIMAL_SUPPORTED_VERSION_FUSED_MODE_IN_HYBRID_QUERY = Version.V_3_8_0; |
There was a problem hiding this comment.
V_3_9_0 is available now
There was a problem hiding this comment.
3.8 is feature branch baseline, I keep it before all dependencies/plugins up-vert to 3.9 to avoid build failures and instabilities with not yet released changes in main. Control gate is the final merge from feature branch to main after all development is completed, then we must update the flag to latest OS version.
a71331b
into
opensearch-project:feature/fusion-hybrid-query
….parameters BEHAVIOR FIX on top of opensearch-project#1933. FusionSpec.fromScoreRankerProcessor read `combination.parameters.rank_constant`, but RRFProcessorFactory — the ground truth for the score-ranker-processor shape — reads `rank_constant` directly off the `combination` clause and rejects anything but `weights` under `parameters`. Two consequences, both caught by an integration test: - Every user-supplied rank_constant was silently ignored, so fused mode always ranked with the default 60. - The shape FusionSpec documented and the IT fixture emitted is rejected by the real processor with HTTP 400 "provided parameter for combination technique is not supported. supported parameters are [weights]", so a fused query could not even reuse an existing RRF pipeline. Changes: - Read rank_constant off the combination clause via the shared RRFScoreNormalizer.resolveRankConstant, so fused mode accepts, rejects and reports exactly what classic does (absent -> 60, non-integer -> "must be an integer", outside [1, 10000] -> the range error). Reading it directly here would silently accept a negative, oversized or fractional value. - Reject rank_constant under `parameters` with a clear message rather than falling back to 60 and mis-ranking every query for a user who put it in the wrong place. - Report the normalization technique for the rrf shape instead of pinning it to "none". The score-ranker-processor has no normalization clause so this still resolves to "none", but an inline fusion block can carry one, and reporting it lets the caller reject the contradictory pairing instead of silently dropping what the user asked for. - Correct the two javadocs that documented the wrong location, and emit rank_constant in the right place in BaseNeuralSearchIT.createRRFSearchPipeline (new optional overload; the 3-arg form delegates with null).
….parameters BEHAVIOR FIX on top of opensearch-project#1933. FusionSpec.fromScoreRankerProcessor read `combination.parameters.rank_constant`, but RRFProcessorFactory — the ground truth for the score-ranker-processor shape — reads `rank_constant` directly off the `combination` clause and rejects anything but `weights` under `parameters`. Two consequences, both caught by an integration test: - Every user-supplied rank_constant was silently ignored, so fused mode always ranked with the default 60. - The shape FusionSpec documented and the IT fixture emitted is rejected by the real processor with HTTP 400 "provided parameter for combination technique is not supported. supported parameters are [weights]", so a fused query could not even reuse an existing RRF pipeline. Changes: - Read rank_constant off the combination clause via the shared RRFScoreNormalizer.resolveRankConstant, so fused mode accepts, rejects and reports exactly what classic does (absent -> 60, non-integer -> "must be an integer", outside [1, 10000] -> the range error). Reading it directly here would silently accept a negative, oversized or fractional value. - Reject rank_constant under `parameters` with a clear message rather than falling back to 60 and mis-ranking every query for a user who put it in the wrong place. - Report the normalization technique for the rrf shape instead of pinning it to "none". The score-ranker-processor has no normalization clause so this still resolves to "none", but an inline fusion block can carry one, and reporting it lets the caller reject the contradictory pairing instead of silently dropping what the user asked for. - Correct the two javadocs that documented the wrong location, and emit rank_constant in the right place in BaseNeuralSearchIT.createRRFSearchPipeline (new optional overload; the 3-arg form delegates with null).
….parameters BEHAVIOR FIX on top of opensearch-project#1933. FusionSpec.fromScoreRankerProcessor read `combination.parameters.rank_constant`, but RRFProcessorFactory — the ground truth for the score-ranker-processor shape — reads `rank_constant` directly off the `combination` clause and rejects anything but `weights` under `parameters`. Two consequences, both caught by an integration test: - Every user-supplied rank_constant was silently ignored, so fused mode always ranked with the default 60. - The shape FusionSpec documented and the IT fixture emitted is rejected by the real processor with HTTP 400 "provided parameter for combination technique is not supported. supported parameters are [weights]", so a fused query could not even reuse an existing RRF pipeline. Changes: - Read rank_constant off the combination clause via the shared RRFScoreNormalizer.resolveRankConstant, so fused mode accepts, rejects and reports exactly what classic does (absent -> 60, non-integer -> "must be an integer", outside [1, 10000] -> the range error). Reading it directly here would silently accept a negative, oversized or fractional value. - Reject rank_constant under `parameters` with a clear message rather than falling back to 60 and mis-ranking every query for a user who put it in the wrong place. - Report the normalization technique for the rrf shape instead of pinning it to "none". The score-ranker-processor has no normalization clause so this still resolves to "none", but an inline fusion block can carry one, and reporting it lets the caller reject the contradictory pairing instead of silently dropping what the user asked for. - Correct the two javadocs that documented the wrong location, and emit rank_constant in the right place in BaseNeuralSearchIT.createRRFSearchPipeline (new optional overload; the 3-arg form delegates with null).
….parameters BEHAVIOR FIX on top of opensearch-project#1933. FusionSpec.fromScoreRankerProcessor read `combination.parameters.rank_constant`, but RRFProcessorFactory — the ground truth for the score-ranker-processor shape — reads `rank_constant` directly off the `combination` clause and rejects anything but `weights` under `parameters`. Two consequences, both caught by an integration test: - Every user-supplied rank_constant was silently ignored, so fused mode always ranked with the default 60. - The shape FusionSpec documented and the IT fixture emitted is rejected by the real processor with HTTP 400 "provided parameter for combination technique is not supported. supported parameters are [weights]", so a fused query could not even reuse an existing RRF pipeline. Changes: - Read rank_constant off the combination clause via the shared RRFScoreNormalizer.resolveRankConstant, so fused mode accepts, rejects and reports exactly what classic does (absent -> 60, non-integer -> "must be an integer", outside [1, 10000] -> the range error). Reading it directly here would silently accept a negative, oversized or fractional value. - Reject rank_constant under `parameters` with a clear message rather than falling back to 60 and mis-ranking every query for a user who put it in the wrong place. - Report the normalization technique for the rrf shape instead of pinning it to "none". The score-ranker-processor has no normalization clause so this still resolves to "none", but an inline fusion block can carry one, and reporting it lets the caller reject the contradictory pairing instead of silently dropping what the user asked for. - Correct the two javadocs that documented the wrong location, and emit rank_constant in the right place in BaseNeuralSearchIT.createRRFSearchPipeline (new optional overload; the 3-arg form delegates with null). Signed-off-by: Daniel Widdis <widdis@gmail.com>
….parameters BEHAVIOR FIX on top of opensearch-project#1933. FusionSpec.fromScoreRankerProcessor read `combination.parameters.rank_constant`, but RRFProcessorFactory — the ground truth for the score-ranker-processor shape — reads `rank_constant` directly off the `combination` clause and rejects anything but `weights` under `parameters`. Two consequences, both caught by an integration test: - Every user-supplied rank_constant was silently ignored, so fused mode always ranked with the default 60. - The shape FusionSpec documented and the IT fixture emitted is rejected by the real processor with HTTP 400 "provided parameter for combination technique is not supported. supported parameters are [weights]", so a fused query could not even reuse an existing RRF pipeline. Changes: - Read rank_constant off the combination clause via the shared RRFScoreNormalizer.resolveRankConstant, so fused mode accepts, rejects and reports exactly what classic does (absent -> 60, non-integer -> "must be an integer", outside [1, 10000] -> the range error). Reading it directly here would silently accept a negative, oversized or fractional value. - Reject rank_constant under `parameters` with a clear message rather than falling back to 60 and mis-ranking every query for a user who put it in the wrong place. - Report the normalization technique for the rrf shape instead of pinning it to "none". The score-ranker-processor has no normalization clause so this still resolves to "none", but an inline fusion block can carry one, and reporting it lets the caller reject the contradictory pairing instead of silently dropping what the user asked for. - Correct the two javadocs that documented the wrong location, and emit rank_constant in the right place in BaseNeuralSearchIT.createRRFSearchPipeline (new optional overload; the 3-arg form delegates with null). Signed-off-by: Daniel Widdis <widdis@gmail.com>
….parameters BEHAVIOR FIX on top of opensearch-project#1933. FusionSpec.fromScoreRankerProcessor read `combination.parameters.rank_constant`, but RRFProcessorFactory — the ground truth for the score-ranker-processor shape — reads `rank_constant` directly off the `combination` clause and rejects anything but `weights` under `parameters`. Two consequences, both caught by an integration test: - Every user-supplied rank_constant was silently ignored, so fused mode always ranked with the default 60. - The shape FusionSpec documented and the IT fixture emitted is rejected by the real processor with HTTP 400 "provided parameter for combination technique is not supported. supported parameters are [weights]", so a fused query could not even reuse an existing RRF pipeline. Changes: - Read rank_constant off the combination clause via the shared RRFScoreNormalizer.resolveRankConstant, so fused mode accepts, rejects and reports exactly what classic does (absent -> 60, non-integer -> "must be an integer", outside [1, 10000] -> the range error). Reading it directly here would silently accept a negative, oversized or fractional value. - Reject rank_constant under `parameters` with a clear message rather than falling back to 60 and mis-ranking every query for a user who put it in the wrong place. - Report the normalization technique for the rrf shape instead of pinning it to "none". The score-ranker-processor has no normalization clause so this still resolves to "none", but an inline fusion block can carry one, and reporting it lets the caller reject the contradictory pairing instead of silently dropping what the user asked for. - Correct the two javadocs that documented the wrong location, and emit rank_constant in the right place in BaseNeuralSearchIT.createRRFSearchPipeline (new optional overload; the 3-arg form delegates with null). Signed-off-by: Daniel Widdis <widdis@gmail.com>
* [RRF] Read rank_constant from the combination clause, not combination.parameters BEHAVIOR FIX on top of #1933. FusionSpec.fromScoreRankerProcessor read `combination.parameters.rank_constant`, but RRFProcessorFactory — the ground truth for the score-ranker-processor shape — reads `rank_constant` directly off the `combination` clause and rejects anything but `weights` under `parameters`. Two consequences, both caught by an integration test: - Every user-supplied rank_constant was silently ignored, so fused mode always ranked with the default 60. - The shape FusionSpec documented and the IT fixture emitted is rejected by the real processor with HTTP 400 "provided parameter for combination technique is not supported. supported parameters are [weights]", so a fused query could not even reuse an existing RRF pipeline. Changes: - Read rank_constant off the combination clause via the shared RRFScoreNormalizer.resolveRankConstant, so fused mode accepts, rejects and reports exactly what classic does (absent -> 60, non-integer -> "must be an integer", outside [1, 10000] -> the range error). Reading it directly here would silently accept a negative, oversized or fractional value. - Reject rank_constant under `parameters` with a clear message rather than falling back to 60 and mis-ranking every query for a user who put it in the wrong place. - Report the normalization technique for the rrf shape instead of pinning it to "none". The score-ranker-processor has no normalization clause so this still resolves to "none", but an inline fusion block can carry one, and reporting it lets the caller reject the contradictory pairing instead of silently dropping what the user asked for. - Correct the two javadocs that documented the wrong location, and emit rank_constant in the right place in BaseNeuralSearchIT.createRRFSearchPipeline (new optional overload; the 3-arg form delegates with null). Signed-off-by: Daniel Widdis <widdis@gmail.com> * [RRF] Wire RRF into the coordinator-side fusion path Enables `rrf` for the resolver (fused) mode of the hybrid query, reusing the shared helpers extracted in #1944 so coordinator-side RRF and classic shard-side RRF compute rank scores through the same code. RRF enters through the ScalarNormalizers registry rather than as a second fusion routine, because reciprocal rank fusion *is* a whole-leg normalization: rank is derived from the leg's full value set, exactly as min_max derives min/max from it. That is also how classic models it — RRFProcessorFactory builds an RRFNormalizationTechnique for a processor that has no normalization clause. So CoordinatorScoreFusion.fuse needs no rrf-specific branch, and neither does the orchestrator: both keep normalizing then combining without knowing which technique they hold. - RrfScalarNormalizer: ranks the leg via RRFScoreNormalizer.assignRanksByScoreDescending, then replaces each score with the shared scoreForRank(rank, rank_constant). A leg a doc did not match keeps CoordinatorScoreFusion's 0.0 slot, which RRFScoreCombinationTechnique's participation rule (score >= 0) treats as inert — mirroring how classic leaves an unmatched sub-query out of the sum. - ScalarNormalizers maps names to factories instead of singletons, mirroring classic's ScoreNormalizationFactory, because rrf is the one technique carrying a parameter. The three score-based techniques are stateless, so their factories hand back a shared instance and ignore the parameters. - FusionSpec resolves the score-ranker shape to normalization "rrf" rather than "none", naming the normalization classic already applies for it. An inline block may still carry a normalization clause; it is reported rather than dropped so the caller rejects the contradictory pairing. - HybridQueryBuilder.requireSupportedTechniques admits rrf and short circuits on rrf + rrf, the one pairing classic's compatibility matrix cannot speak to: that matrix describes the normalization-processor, whose combination options are the three means, while RRFProcessorFactory pins both halves itself. Any other normalization alongside rrf still falls through to the matrix and is rejected there by name. Tie contract: classic ranks ties by Lucene docId then shardId, both physical layout artifacts that do not exist coordinator-side, where only merged results are available. Coordinator RRF therefore ranks ties by ascending fusion key — layout- and insertion-order-independent, and the closest stable analogue. This is a deliberate divergence, documented on RrfScalarNormalizer and pinned by a test that inserts tied docs in two different orders and asserts an identical result. Tests: - CoordinatorScoreFusionRrfDifferentialTests: parity against RRFNormalizationTechnique + RRFScoreCombinationTechnique over both classic ranking paths (positional single-shard and the cross-shard priority queue), for single/multiple sub-queries, partial overlap, weights, rank_constant bounds, raw-score magnitude irrelevance, and the tie order above. - ScalarNormalizerTests: rrf resolves through the registry, the no-parameter overload falls back to classic's default rank_constant, two rank constants build two differently-scored normalizers, and the per-leg rank arithmetic is order-only (magnitude-independent). - HybridFusionOrchestratorTests: exact fused scores read off the self-erased query's should-clause boosts, plus rank_constant honoring and magnitude independence. - HybridQueryFusedModeIT: rrf end to end from both config sources (an attached score-ranker-processor pipeline and an inline block), and rejection of rrf paired with a score-normalization technique. Score assertions pin the set of possible rank-score sums rather than exact per-doc scores, since individual ranks depend on how the 3 shards split the corpus. Signed-off-by: Daniel Widdis <widdis@gmail.com> * [RRF] Add an integration test comparing fused RRF against the classic pipeline directly The existing RRF integration tests assert that fused mode produces rank-score sums; none of them compares fused mode against classic shard-side RRF. That comparison is the actual contract of this work — the shared RRFScoreNormalizer exists so the two paths cannot drift — so assert it end to end. For one index and one pair of sub-queries, run classic (a score-ranker-processor pipeline), fused with inline config, and fused reading that same pipeline, then require identical documents and identical scores. Covers rank_constant at 1, 60 and 10000, unweighted and weighted [0.3, 0.7]. Design notes: - Single shard, and a window well above the match count, so the two paths' candidate pools are the same set. They can legitimately differ once a truncating window meets more than one shard: classic ranks the union of each shard's top-pagination_depth per leg, fused the merged global top-window_size. - Scores are compared per document at exact equality, and the score sequence is compared separately, so the assertion is agnostic about which document wins a tie on the fused score. - The corpus gives every document a distinct (term frequency, length) pair per leg so neither leg ties two documents on score, and the test asserts that up front. A within-leg tie is the one place the paths deliberately disagree (classic orders by Lucene docId, fused by ascending _id), so asserting tie-freeness makes a future scoring change surface as "the corpus developed a tie" rather than as an unexplained parity failure. Verified non-vacuous by mutation: passing rankConstant + 1 to the inline fused config fails the per-document assertion with a full score diff. Signed-off-by: Daniel Widdis <widdis@gmail.com> * Add changelog entry Signed-off-by: Daniel Widdis <widdis@gmail.com> * [RRF] Delete the now-dead NORMALIZATION_NONE constant Modelling rrf as normalization=rrf removed the only place fused mode resolved a config to "none", so the constant no longer names anything the coordinator understands. Its last reference was the constructor's null fallback, which now has nothing to fall back to: both factories always resolve a technique name, each shape defaulting its own way, so require non-null instead of defaulting to a value the technique gate would reject anyway. Signed-off-by: Daniel Widdis <widdis@gmail.com> * [RRF] Read rank_constant per config shape, and key the matrix exemption on shape Two ways fused mode diverged from classic for rrf, both reported in review. 1. A `normalization-processor` may name rrf as its *normalization* technique, and there the rank constant lives under `normalization.parameters` — that is the map NormalizationProcessorFactory hands to RRFNormalizationTechnique. FusionSpec only ever looked on the combination clause (the score-ranker-processor's location), so every such pipeline fused at the default 60 no matter what the user configured, silently. Each shape now reads the rank constant where its own classic factory reads it, through the shared resolver, so an out-of-range or non-integer value is the same 400 classic gives rather than a silent fallback. 2. The rrf + rrf pairing was exempted from classic's compatibility matrix by technique name. But two different configs resolve to those same two names: the score-ranker-processor, where the exemption belongs because the matrix describes the normalization-processor and cannot speak to it, and a normalization-processor asked to combine rrf-normalized scores by rrf — which classic rejects through that very matrix. Matching on names admitted the second, leaving fused mode looser than classic. FusionSpec now carries the Shape it was read as, and the exemption keys on that, so the matrix stays authoritative for the normalization-processor shape. Both were reachable only in fused mode, which is unreleased. Signed-off-by: Daniel Widdis <widdis@gmail.com> --------- Signed-off-by: Daniel Widdis <widdis@gmail.com>
4 participants
Description
Initial scope for resolver in hybrid query, targeting feature branch:
After this PR following query will work:
windowandweightsparamsthese require an attached pipeline resolvable via inline body / ?search_pipeline= / index.search.default_pipeline
Intentional limitation:
There will be follow up PRs with:
Related Issues
Resolves #1930
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.