Skip to content

Commit 641111f

Browse files
committed
[RRF] Read rank_constant from the combination clause, not combination.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>
1 parent 74f0a90 commit 641111f

4 files changed

Lines changed: 121 additions & 29 deletions

File tree

src/main/java/org/opensearch/neuralsearch/query/FusionSpec.java

Lines changed: 53 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import org.opensearch.neuralsearch.processor.combination.ArithmeticMeanScoreCombinationTechnique;
1818
import org.opensearch.neuralsearch.processor.combination.RRFScoreCombinationTechnique;
1919
import org.opensearch.neuralsearch.processor.normalization.MinMaxScoreNormalizationTechnique;
20+
import org.opensearch.neuralsearch.processor.normalization.RRFScoreNormalizer;
2021

2122
/**
2223
* Immutable, resolved fusion configuration for the {@code hybrid} query resolver (fused) mode — the normalization +
@@ -33,8 +34,9 @@
3334
* <ul>
3435
* <li>{@code normalization-processor}: {@code normalization.technique} (min_max|l2|z_score) +
3536
* {@code combination.technique} (arithmetic_mean) + optional {@code combination.parameters.weights}.</li>
36-
* <li>{@code score-ranker-processor}: {@code combination.technique = rrf} +
37-
* {@code combination.parameters.rank_constant}. RRF is rank-based (no normalization clause).</li>
37+
* <li>{@code score-ranker-processor}: {@code combination.technique = rrf} + {@code combination.rank_constant} (on the
38+
* combination clause itself, NOT under {@code parameters} — that is where {@code RRFProcessorFactory} reads it) +
39+
* optional {@code combination.parameters.weights}. RRF is rank-based (no normalization clause).</li>
3840
* </ul>
3941
*/
4042
@Getter(AccessLevel.PACKAGE)
@@ -48,7 +50,8 @@ public final class FusionSpec {
4850
static final String NORMALIZATION_NONE = "none";
4951
static final String NORMALIZATION_MIN_MAX = MinMaxScoreNormalizationTechnique.TECHNIQUE_NAME;
5052

51-
static final int DEFAULT_RANK_CONSTANT = 60;
53+
// Sourced from the shared normalizer rather than redeclared, so fused mode and classic cannot drift apart.
54+
static final int DEFAULT_RANK_CONSTANT = RRFScoreNormalizer.DEFAULT_RANK_CONSTANT;
5255

5356
// Config-map keys (shared by the normalization-processor and score-ranker-processor definitions)
5457
private static final String PHASE_RESULTS_PROCESSORS_KEY = "phase_results_processors";
@@ -57,7 +60,7 @@ public final class FusionSpec {
5760
private static final String TECHNIQUE_KEY = "technique";
5861
private static final String PARAMETERS_KEY = "parameters";
5962
private static final String WEIGHTS_KEY = "weights";
60-
private static final String RANK_CONSTANT_KEY = "rank_constant";
63+
private static final String RANK_CONSTANT_KEY = RRFScoreNormalizer.PARAM_NAME_RANK_CONSTANT;
6164

6265
private final String combinationTechnique; // rrf | arithmetic_mean
6366
private final String normalizationTechnique; // none | min_max | z_score | l2
@@ -108,7 +111,7 @@ static FusionSpec fromPipelineConfig(Map<String, Object> pipelineConfig) {
108111
/**
109112
* Read a {@link FusionSpec} from an inline {@code fusion} block on the query body (precedence step 1: inline wins
110113
* over the attached pipeline). The block mirrors the processor JSON verbatim —
111-
* {@code {normalization: {technique}, combination: {technique, parameters: {weights | rank_constant}}}} — so this
114+
* {@code {normalization: {technique}, combination: {technique, rank_constant, parameters: {weights}}}} — so this
112115
* reuses the pipeline-config parsing. {@code combination.technique: rrf} routes to the rank-constant shape.
113116
*
114117
* @param fusionConfig the parsed inline fusion map (nullable)
@@ -130,13 +133,7 @@ static FusionSpec fromInlineFusion(Map<String, Object> fusionConfig) {
130133

131134
@SuppressWarnings("unchecked")
132135
private static FusionSpec fromNormalizationProcessor(Map<String, Object> config) {
133-
String normalization = NORMALIZATION_MIN_MAX;
134-
if (config.get(NORMALIZATION_CLAUSE) instanceof Map) {
135-
Object technique = ((Map<String, Object>) config.get(NORMALIZATION_CLAUSE)).get(TECHNIQUE_KEY);
136-
if (Objects.nonNull(technique)) {
137-
normalization = technique.toString().toLowerCase(Locale.ROOT);
138-
}
139-
}
136+
String normalization = readNormalizationTechnique(config, NORMALIZATION_MIN_MAX);
140137
String combination = TECHNIQUE_ARITHMETIC_MEAN;
141138
float[] weights = new float[0];
142139
if (config.get(COMBINATION_CLAUSE) instanceof Map) {
@@ -156,15 +153,52 @@ private static FusionSpec fromScoreRankerProcessor(Map<String, Object> config) {
156153
float[] weights = new float[0];
157154
if (config.get(COMBINATION_CLAUSE) instanceof Map) {
158155
Map<String, Object> combinationClause = (Map<String, Object>) config.get(COMBINATION_CLAUSE);
159-
if (combinationClause.get(PARAMETERS_KEY) instanceof Map) {
160-
Map<String, Object> parameters = (Map<String, Object>) combinationClause.get(PARAMETERS_KEY);
161-
if (parameters.get(RANK_CONSTANT_KEY) instanceof Number) {
162-
rankConstant = ((Number) parameters.get(RANK_CONSTANT_KEY)).intValue();
163-
}
164-
}
156+
rejectRankConstantUnderParameters(combinationClause);
157+
// rank_constant sits on the combination clause itself, which is where RRFProcessorFactory reads it — NOT
158+
// under `parameters`, whose only supported key is `weights`. Delegating to the shared resolver makes fused
159+
// mode accept, reject and report exactly what classic does: absent -> default, non-integer -> "must be an
160+
// integer", outside [1, 10000] -> the range error. Reading the value directly here would silently accept a
161+
// negative, oversized or fractional rank constant that the score-ranker-processor rejects.
162+
rankConstant = RRFScoreNormalizer.resolveRankConstant(combinationClause);
165163
weights = readWeights(combinationClause);
166164
}
167-
return new FusionSpec(TECHNIQUE_RRF, NORMALIZATION_NONE, rankConstant, weights);
165+
// RRF is rank based, so the score-ranker-processor has no normalization clause and this resolves to "none".
166+
// An inline fusion block can still carry one, and it is reported rather than dropped so the caller's technique
167+
// check rejects the contradictory pairing instead of silently ignoring what the user asked for.
168+
return new FusionSpec(TECHNIQUE_RRF, readNormalizationTechnique(config, NORMALIZATION_NONE), rankConstant, weights);
169+
}
170+
171+
/**
172+
* {@code rank_constant} under {@code combination.parameters} is a config error, not a place we also look: the
173+
* score-ranker-processor rejects it there ("supported parameters are [weights]"). Fused mode rejects it too, rather
174+
* than silently falling back to the default 60 and mis-ranking every query for a user who put it in the wrong place.
175+
*/
176+
@SuppressWarnings("unchecked")
177+
private static void rejectRankConstantUnderParameters(Map<String, Object> combinationClause) {
178+
if ((combinationClause.get(PARAMETERS_KEY) instanceof Map) == false) {
179+
return;
180+
}
181+
if (((Map<String, Object>) combinationClause.get(PARAMETERS_KEY)).containsKey(RANK_CONSTANT_KEY)) {
182+
throw new IllegalArgumentException(
183+
String.format(
184+
Locale.ROOT,
185+
"[%s] must be set on the [%s] clause, not under [%s]; supported parameters are [%s]",
186+
RANK_CONSTANT_KEY,
187+
COMBINATION_CLAUSE,
188+
PARAMETERS_KEY,
189+
WEIGHTS_KEY
190+
)
191+
);
192+
}
193+
}
194+
195+
@SuppressWarnings("unchecked")
196+
private static String readNormalizationTechnique(Map<String, Object> config, String defaultTechnique) {
197+
if ((config.get(NORMALIZATION_CLAUSE) instanceof Map) == false) {
198+
return defaultTechnique;
199+
}
200+
Object technique = ((Map<String, Object>) config.get(NORMALIZATION_CLAUSE)).get(TECHNIQUE_KEY);
201+
return Objects.isNull(technique) ? defaultTechnique : technique.toString().toLowerCase(Locale.ROOT);
168202
}
169203

170204
@SuppressWarnings("unchecked")

src/test/java/org/opensearch/neuralsearch/query/FusionSpecTests.java

Lines changed: 46 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,58 @@ public void testFromInlineFusion_whenNormalizationCombination_thenParsed() {
2626
}
2727

2828
public void testFromInlineFusion_whenRrf_thenRankConstantAndNoNormalization() {
29-
Map<String, Object> inline = Map.of("combination", Map.of("technique", "rrf", "parameters", Map.of("rank_constant", 42)));
29+
Map<String, Object> inline = Map.of("combination", Map.of("technique", "rrf", "rank_constant", 42));
3030
FusionSpec spec = FusionSpec.fromInlineFusion(inline);
3131
assertNotNull(spec);
3232
assertEquals(FusionSpec.TECHNIQUE_RRF, spec.combinationTechnique());
3333
assertEquals(FusionSpec.NORMALIZATION_NONE, spec.normalizationTechnique());
3434
assertEquals(42, spec.rankConstant());
3535
}
3636

37+
public void testFromInlineFusion_whenRrfWithNormalizationClause_thenNormalizationReported() {
38+
// RRF takes no normalization technique, but an inline block can still carry one. It is reported rather than
39+
// dropped so the caller's technique check can reject the contradictory pairing.
40+
Map<String, Object> inline = Map.of("normalization", Map.of("technique", "min_max"), "combination", Map.of("technique", "rrf"));
41+
FusionSpec spec = FusionSpec.fromInlineFusion(inline);
42+
assertNotNull(spec);
43+
assertEquals(FusionSpec.TECHNIQUE_RRF, spec.combinationTechnique());
44+
assertEquals(FusionSpec.NORMALIZATION_MIN_MAX, spec.normalizationTechnique());
45+
}
46+
47+
public void testFromInlineFusion_whenRrfRankConstantInvalid_thenRejected() {
48+
// Resolved through the shared validator, so fused mode rejects exactly what the score-ranker-processor rejects.
49+
assertThrows(
50+
IllegalArgumentException.class,
51+
() -> FusionSpec.fromInlineFusion(Map.of("combination", Map.of("technique", "rrf", "rank_constant", 0)))
52+
);
53+
assertThrows(
54+
IllegalArgumentException.class,
55+
() -> FusionSpec.fromInlineFusion(Map.of("combination", Map.of("technique", "rrf", "rank_constant", 10001)))
56+
);
57+
assertThrows(
58+
IllegalArgumentException.class,
59+
() -> FusionSpec.fromInlineFusion(Map.of("combination", Map.of("technique", "rrf", "rank_constant", "not-a-number")))
60+
);
61+
}
62+
63+
public void testFromInlineFusion_whenRankConstantUnderParameters_thenRejected() {
64+
// The score-ranker-processor reads rank_constant off the combination clause and rejects it under `parameters`
65+
// ("supported parameters are [weights]"). Fused mode must reject it the same way rather than silently defaulting
66+
// to 60 and mis-ranking every query.
67+
IllegalArgumentException e = assertThrows(
68+
IllegalArgumentException.class,
69+
() -> FusionSpec.fromInlineFusion(Map.of("combination", Map.of("technique", "rrf", "parameters", Map.of("rank_constant", 42))))
70+
);
71+
assertTrue(e.getMessage().contains("must be set on the [combination] clause"));
72+
}
73+
74+
public void testFromInlineFusion_whenRrfWithoutRankConstant_thenDefault() {
75+
FusionSpec spec = FusionSpec.fromInlineFusion(Map.of("combination", Map.of("technique", "rrf")));
76+
assertNotNull(spec);
77+
assertEquals(FusionSpec.TECHNIQUE_RRF, spec.combinationTechnique());
78+
assertEquals(FusionSpec.DEFAULT_RANK_CONSTANT, spec.rankConstant());
79+
}
80+
3781
public void testFromInlineFusion_whenDefaults_thenMinMaxArithmeticMean() {
3882
// An empty inline block resolves to the min_max + arithmetic_mean defaults.
3983
FusionSpec spec = FusionSpec.fromInlineFusion(Map.of());
@@ -67,12 +111,7 @@ public void testFromPipelineConfig_whenNormalizationProcessor_thenParsed() {
67111
public void testFromPipelineConfig_whenScoreRankerProcessor_thenRrf() {
68112
Map<String, Object> pipelineConfig = Map.of(
69113
"phase_results_processors",
70-
List.of(
71-
Map.of(
72-
"score-ranker-processor",
73-
Map.of("combination", Map.of("technique", "rrf", "parameters", Map.of("rank_constant", 10)))
74-
)
75-
)
114+
List.of(Map.of("score-ranker-processor", Map.of("combination", Map.of("technique", "rrf", "rank_constant", 10))))
76115
);
77116
FusionSpec spec = FusionSpec.fromPipelineConfig(pipelineConfig);
78117
assertNotNull(spec);

src/test/java/org/opensearch/neuralsearch/query/HybridQueryBuilderTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -659,7 +659,7 @@ public void testDoRewriteFused_whenUnsupportedTechnique_thenFailsFast() {
659659
public void testDoRewriteFused_whenRrf_thenFailsFast() {
660660
setUpClusterService();
661661
HybridQueryBuilder builder = fusedBuilder(
662-
new HashMap<>(Map.of("combination", Map.of("technique", "rrf", "parameters", Map.of("rank_constant", 60))))
662+
new HashMap<>(Map.of("combination", Map.of("technique", "rrf", "rank_constant", 60)))
663663
);
664664
QueryCoordinatorContext ctx = coordinatorContextFor(builder);
665665
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder.doRewrite(ctx));

src/testFixtures/java/org/opensearch/neuralsearch/BaseNeuralSearchIT.java

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2766,15 +2766,34 @@ protected void createDefaultRRFSearchPipeline() {
27662766

27672767
@SneakyThrows
27682768
protected void createRRFSearchPipeline(final String pipelineName, final List<Double> weights, boolean addExplainResponseProcessor) {
2769+
createRRFSearchPipeline(pipelineName, weights, null, addExplainResponseProcessor);
2770+
}
2771+
2772+
/**
2773+
* @param rankConstant written as {@code combination.rank_constant} when non-null — the location
2774+
* {@code RRFProcessorFactory} reads, which is the combination clause itself and NOT
2775+
* {@code parameters} (whose only supported key is {@code weights}). Null omits the key so the
2776+
* processor applies its own default.
2777+
*/
2778+
@SneakyThrows
2779+
protected void createRRFSearchPipeline(
2780+
final String pipelineName,
2781+
final List<Double> weights,
2782+
final Integer rankConstant,
2783+
boolean addExplainResponseProcessor
2784+
) {
27692785
XContentBuilder builder = XContentFactory.jsonBuilder()
27702786
.startObject()
27712787
.field("description", "Post processor for hybrid search")
27722788
.startArray("phase_results_processors")
27732789
.startObject()
27742790
.startObject("score-ranker-processor")
27752791
.startObject("combination")
2776-
.field("technique", "rrf")
2777-
.startObject("parameters");
2792+
.field("technique", "rrf");
2793+
if (rankConstant != null) {
2794+
builder.field("rank_constant", rankConstant);
2795+
}
2796+
builder.startObject("parameters");
27782797
if (weights.size() > 0) {
27792798
builder.startArray("weights");
27802799
for (Double weight : weights) {

0 commit comments

Comments
 (0)