Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not pass negative scores into function_score or script_score queries #13627

Closed
wants to merge 1 commit into from
Closed

Do not pass negative scores into function_score or script_score queries #13627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Remove handling of index.mapper.dynamic in AutoCreateIndex([#13067](https://github.com/opensearch-project/OpenSearch/pull/13067))

### Fixed
- Fix negative RequestStats metric issue ([#13553](https://github.com/opensearch-project/OpenSearch/pull/13553))
- Fix get field mapping API returns 404 error in mixed cluster with multiple versions ([#13624](https://github.com/opensearch-project/OpenSearch/pull/13624))
- Allow clearing `remote_store.compatibility_mode` setting ([#13646](https://github.com/opensearch-project/OpenSearch/pull/13646))
- Replace negative input scores to function/script score queries with zero to avoid downstream exception ([#13627](https://github.com/opensearch-project/OpenSearch/pull/13627))
- Fix ReplicaShardBatchAllocator to batch shards without duplicates ([#13710](https://github.com/opensearch-project/OpenSearch/pull/13710))

### Security
Expand Down
76 changes: 76 additions & 0 deletions modules/lang-painless/src/yamlRestTest/resources/rest-api-spec/test/painless/30_search.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,3 +482,79 @@
}]
- match: { error.root_cause.0.type: "illegal_argument_exception" }
- match: { error.root_cause.0.reason: "script score function must not produce negative scores, but got: [-9.0]"}

---
"Do not throw exception if input score is negative":
- do:
index:
index: test
id: 1
body: { "color" : "orange red yellow" }
- do:
index:
index: test
id: 2
body: { "color": "orange red purple", "shape": "red square" }
- do:
index:
index: test
id: 3
body: { "color" : "orange red yellow purple" }
- do:
indices.refresh: { }
- do:
search:
index: test
body:
query:
function_score:
query:
multi_match:
query: "red"
type: "cross_fields"
fields: [ "color", "shape^100"]
tie_breaker: 0.1
functions: [{
"script_score": {
"script": {
"lang": "painless",
"source": "_score"
}
}
}]
explain: true
- match: { hits.total.value: 3 }
- match: { hits.hits.2._score: 0.0 }
Copy link
Contributor

@mingshl mingshl May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

nice, so the functional score on a document will be at least zero, instead of negative. This will fix the score per document level.

Wondering if this also fix the per field level score is negative? can we run profile api on yaml test and check perFieldSimilarity score is negative or zero?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The logic focuses on the input score passed to a function or script, regardless of where that score came from.

Both before and after this change, we have checks that the score returned by a function or script is not negative (and throw an exception if it is). Unfortunately, this meant that a function/script that just passes a score through would throw an exception if the input score is negative.

So, for any case where a function or script gets an illegal negative value as input, this change defensively treats it as zero.

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome! thank you

- do:
search:
index: test
body:
query:
function_score:
query:
multi_match:
query: "red"
type: "cross_fields"
fields: [ "color", "shape^100"]
tie_breaker: 0.1
weight: 1
explain: true
- match: { hits.total.value: 3 }
- match: { hits.hits.2._score: 0.0 }
- do:
search:
index: test
body:
query:
script_score:
query:
multi_match:
query: "red"
type: "cross_fields"
fields: [ "color", "shape^100"]
tie_breaker: 0.1
script:
source: "_score"
explain: true
- match: { hits.total.value: 3 }
- match: { hits.hits.2._score: 0.0 }
6 changes: 4 additions & 2 deletions server/src/main/java/org/opensearch/common/lucene/search/function/FunctionScoreQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,8 +533,10 @@ public float score() throws IOException {
int docId = docID();
// Even if the weight is created with needsScores=false, it might
// be costly to call score(), so we explicitly check if scores
// are needed
float subQueryScore = needsScores ? super.score() : 0f;
// are needed.
// While the function scorer should never turn a score negative, we
// must guard against the input score being negative.
float subQueryScore = needsScores ? Math.max(0f, super.score()) : 0f;
if (leafFunctions.length == 0) {
return subQueryScore;
}
Expand Down
14 changes: 10 additions & 4 deletions server/src/main/java/org/opensearch/common/lucene/search/function/ScriptScoreFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,14 @@
public class ScriptScoreFunction extends ScoreFunction {

static final class CannedScorer extends Scorable {
protected int docid;
protected float score;
private int docid;
private float score;

public void score(float subScore) {
// We check to make sure the script score function never makes a score negative, but we need to make
// sure the script score function does not receive negative input.
this.score = Math.max(0.0f, subScore);
Copy link
Collaborator

@reta reta May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@msfroh don't we try to hide the problem instead of fixing the source of negative scoring? As you mentioned, we do catch that in some cases (and thrown an exception) but it slips in other, why cannot we fix that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was some discussion about this on #7860.

Essentially, the scoring logic in cross_fields match is pretty complicated. Actually, the Elasticsearch documentation for cross_fields even contains the following warning:

The cross_fields type blends field statistics in a complex way that can be hard to interpret. The score combination can even be incorrect, in particular when some documents contain some of the search fields, but not all of them.

As @ylwu-amzn called out on the issue, any change we make to the cross_fields scoring formula could impact people's "regular" search results. We'd need to be extremely careful.

Meanwhile, this change just turns a query that was throwing an exception (which is what people have complained about) into one that doesn't.

We could possibly wrap the query returned from MatchQueryBuilder in a custom scorer that clamps negative values to zero. Functionally that's what happens if tie_breaker is 0. I would still include the changes in this PR as an extra layer of protection.

Copy link
Collaborator Author

@msfroh msfroh May 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@reta -- I spent some more time today trying to prevent the negative score from bubbling up. So far as I can tell, this is the query that's returning the negative scores:

OpenSearch/server/src/main/java/org/apache/lucene/queries/BlendedTermQuery.java

Line 410 in 4a42150

return new DisjunctionMaxQuery(queries, tieBreakerMultiplier);

Unfortunately, that just returns a Lucene DisjunctionMaxQuery, which normally does the correct thing. The problem arises because of the adjustDF and/or the adjustTTF methods. Let's pretend that Lucene term stats are something other than what they really are... yay!

I believe it's specifically adjustTTF method (called from the blend method) that breaks us, because it lowers the sumTotalTermFreq to the minimum of sumTotalTermFreq across all of the fields, which may be lower than any individual field's term's max termFreq (from another field). In theory, the shared sumTotalTermFreq should never be lower than the maximum of termFreq for any doc across all fields, but we don't have a good way of getting that value without iterating through all documents matched by the terms. I'd love to add a line just before here saying minSumTTF = Math.max(minSumTTF, maxTermFreq).

tl;dr: Preventing the negative scores from getting into function/script scoring functions is hard. This cross_field scoring is a complicated mess. While it is the root cause, the immediate pain point is not the negative scores, but that the negative scores make perfectly valid function/script scoring throw an exception.

I'd like to commit this fix as-is to address the immediate problem. We can address the complicated cross_field logic down the road.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When we enter the blend() method for the integ test I wrote, we start off with color:red with df=3, sumTtf=3, while shape:red has df=1,sumTtf=1. Also, maxDoc is 3.

  1. We exit the first loop with minSumTTF=2 and max = 3. We set maxDoc to 2 (since it's bigger than minSumTTF).
  2. Later, we set actualDf to 2.

The relevant piece of the explain output is:

{
  "value" : -0.22314355,
  "description" : "idf, computed as log(1 + (N - n + 0.5) / (n + 0.5)) from:",
  "details" : [
    {
      "value" : 2,
      "description" : "n, number of documents containing term",
      "details" : [ ]
    },
    {
      "value" : 1,
      "description" : "N, total number of documents with field",
      "details" : [ ]
    }
  ]
},

That n, number of documents containing the term came from actualDf.

I think I can actually fix it by guaranteeing that actualDf is never greater than N, which I can get from reader.getDocCount(terms[i].field()).

So, after saying that fixing the underlying problem is too hard, I think I can fix the underlying problem instead. 😁

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here we go: #13829

It may still be worth merging this PR as an extra layer of protection, but I'd need to update / remove the YAML REST test, because we no longer get a negative score that we force to zero. Alternatively, we could just close this PR, since the underlying problem is (probably) fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for looking into this @msfroh (I learn quite a few things from your clear explanation)

}

@Override
public int docID() {
Expand Down Expand Up @@ -105,7 +111,7 @@ public LeafScoreFunction getLeafScoreFunction(LeafReaderContext ctx) throws IOEx
public double score(int docId, float subQueryScore) throws IOException {
leafScript.setDocument(docId);
scorer.docid = docId;
scorer.score = subQueryScore;
scorer.score(subQueryScore);
double result = leafScript.execute(null);
if (result < 0f) {
throw new IllegalArgumentException("script score function must not produce negative scores, but got: [" + result + "]");
Expand All @@ -119,7 +125,7 @@ public Explanation explainScore(int docId, Explanation subQueryScore) throws IOE
if (leafScript instanceof ExplainableScoreScript) {
leafScript.setDocument(docId);
scorer.docid = docId;
scorer.score = subQueryScore.getValue().floatValue();
scorer.score(subQueryScore.getValue().floatValue());
exp = ((ExplainableScoreScript) leafScript).explain(subQueryScore, functionName);
} else {
double score = score(docId, subQueryScore.getValue().floatValue());
Expand Down
4 changes: 3 additions & 1 deletion server/src/main/java/org/opensearch/script/ScoreScript.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,9 @@ public void setDocument(int docid) {
public void setScorer(Scorable scorer) {
this.scoreSupplier = () -> {
try {
return scorer.score();
// The ScoreScript is forbidden from returning a negative value.
// We should guard against receiving negative input.
return Math.max(0f, scorer.score());
} catch (IOException e) {
throw new UncheckedIOException(e);
}
Expand Down
114 changes: 114 additions & 0 deletions server/src/test/java/org/opensearch/index/query/NegativeBoostQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.index.query;

import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.search.DocIdSetIterator;
import org.apache.lucene.search.Explanation;
import org.apache.lucene.search.IndexSearcher;
import org.apache.lucene.search.Query;
import org.apache.lucene.search.QueryVisitor;
import org.apache.lucene.search.ScoreMode;
import org.apache.lucene.search.Scorer;
import org.apache.lucene.search.Weight;

import java.io.IOException;

/**
* Similar to Lucene's BoostQuery, but will accept negative boost values (which is normally wrong, since scores
* should not be negative). Useful for testing that other query types guard against negative input scores.
*/
public class NegativeBoostQuery extends Query {
private final Query query;
private final float boost;

public NegativeBoostQuery(Query query, float boost) {
if (boost >= 0) {
throw new IllegalArgumentException("Expected negative boost. Use BoostQuery if boost is non-negative.");
}
this.boost = boost;
this.query = query;
}

@Override
public String toString(String field) {
StringBuilder builder = new StringBuilder();
builder.append("(");
builder.append(query.toString(field));
builder.append(")");
builder.append("^");
builder.append(boost);
return builder.toString();
}

@Override
public void visit(QueryVisitor visitor) {
query.visit(visitor);
}

@Override
public boolean equals(Object other) {
return sameClassAs(other) && equalsTo(getClass().cast(other));
}

private boolean equalsTo(NegativeBoostQuery other) {
return query.equals(other.query) && Float.floatToIntBits(boost) == Float.floatToIntBits(other.boost);
}

@Override
public int hashCode() {
int h = classHash();
h = 31 * h + query.hashCode();
h = 31 * h + Float.floatToIntBits(boost);
return h;
}

@Override
public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) throws IOException {
final float negativeBoost = this.boost;
Weight delegate = query.createWeight(searcher, scoreMode, boost);
return new Weight(this) {
@Override
public Explanation explain(LeafReaderContext context, int doc) throws IOException {
return delegate.explain(context, doc);
}

@Override
public Scorer scorer(LeafReaderContext context) throws IOException {
Scorer delegateScorer = delegate.scorer(context);
return new Scorer(this) {
@Override
public DocIdSetIterator iterator() {
return delegateScorer.iterator();
}

@Override
public float getMaxScore(int upTo) throws IOException {
return delegateScorer.getMaxScore(upTo);
}

@Override
public float score() throws IOException {
return delegateScorer.score() * negativeBoost;
}

@Override
public int docID() {
return delegateScorer.docID();
}
};
}

@Override
public boolean isCacheable(LeafReaderContext ctx) {
return delegate.isCacheable(ctx);
}
};
}
}
19 changes: 19 additions & 0 deletions server/src/test/java/org/opensearch/index/query/functionscore/FunctionScoreTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
import org.opensearch.index.fielddata.ScriptDocValues;
import org.opensearch.index.fielddata.SortedBinaryDocValues;
import org.opensearch.index.fielddata.SortedNumericDoubleValues;
import org.opensearch.index.query.NegativeBoostQuery;
import org.opensearch.search.DocValueFormat;
import org.opensearch.search.MultiValueMode;
import org.opensearch.search.aggregations.support.ValuesSourceType;
Expand Down Expand Up @@ -1095,6 +1096,24 @@ public void testExceptionOnNegativeScores() {
assertThat(exc.getMessage(), not(containsString("consider using log1p or log2p instead of log to avoid negative scores")));
}

public void testNoExceptionOnNegativeScoreInput() throws IOException {
IndexSearcher localSearcher = new IndexSearcher(reader);
TermQuery termQuery = new TermQuery(new Term(FIELD, "out"));

// test that field_value_factor function throws an exception on negative scores
FieldValueFactorFunction.Modifier modifier = FieldValueFactorFunction.Modifier.NONE;

final ScoreFunction fvfFunction = new FieldValueFactorFunction(FIELD, 1, modifier, 1.0, new IndexNumericFieldDataStub());
FunctionScoreQuery fsQuery1 = new FunctionScoreQuery(
new NegativeBoostQuery(termQuery, -10f),
fvfFunction,
CombineFunction.MULTIPLY,
null,
Float.POSITIVE_INFINITY
);
localSearcher.search(fsQuery1, 1);
}

public void testExceptionOnLnNegativeScores() {
IndexSearcher localSearcher = new IndexSearcher(reader);
TermQuery termQuery = new TermQuery(new Term(FIELD, "out"));
Expand Down
24 changes: 23 additions & 1 deletion server/src/test/java/org/opensearch/search/query/ScriptScoreQueryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,23 @@
import org.apache.lucene.index.DirectoryReader;
import org.apache.lucene.index.IndexWriter;
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.DocIdSetIterator;
import org.apache.lucene.search.Explanation;
import org.apache.lucene.search.IndexSearcher;
import org.apache.lucene.search.MatchAllDocsQuery;
import org.apache.lucene.search.Query;
import org.apache.lucene.search.ScoreMode;
import org.apache.lucene.search.Scorer;
import org.apache.lucene.search.TermQuery;
import org.apache.lucene.search.TopDocs;
import org.apache.lucene.search.TwoPhaseIterator;
import org.apache.lucene.search.Weight;
import org.apache.lucene.store.Directory;
import org.opensearch.Version;
import org.opensearch.common.lucene.search.Queries;
import org.opensearch.common.lucene.search.function.ScriptScoreQuery;
import org.opensearch.index.query.NegativeBoostQuery;
import org.opensearch.script.ScoreScript;
import org.opensearch.script.Script;
import org.opensearch.script.ScriptType;
Expand All @@ -64,6 +68,7 @@
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;
import java.util.function.BiFunction;
import java.util.function.Function;

import static org.hamcrest.CoreMatchers.containsString;
Expand Down Expand Up @@ -185,6 +190,15 @@ public void testScriptScoreErrorOnNegativeScore() {
assertTrue(e.getMessage().contains("Must be a non-negative score!"));
}

public void testNoExceptionOnNegativeInputScore() throws IOException {
Script script = new Script("script that returns _score");
ScoreScript.LeafFactory factory = newFactory(script, true, (s, e) -> s.get_score());
NegativeBoostQuery negativeBoostQuery = new NegativeBoostQuery(new TermQuery(new Term("field", "text")), -10.0f);
ScriptScoreQuery query = new ScriptScoreQuery(negativeBoostQuery, script, factory, -1f, "index", 0, Version.CURRENT);
TopDocs topDocs = searcher.search(query, 1);
assertEquals(0.0f, topDocs.scoreDocs[0].score, 0.0001);
}

public void testTwoPhaseIteratorDelegation() throws IOException {
Map<String, Object> params = new HashMap<>();
String scriptSource = "doc['field'].value != null ? 2.0 : 0.0"; // Adjust based on actual field and logic
Expand Down Expand Up @@ -220,6 +234,14 @@ private ScoreScript.LeafFactory newFactory(
Script script,
boolean needsScore,
Function<ScoreScript.ExplanationHolder, Double> function
) {
return newFactory(script, needsScore, (s, e) -> function.apply(e));
}

private ScoreScript.LeafFactory newFactory(
Script script,
boolean needsScore,
BiFunction<ScoreScript, ScoreScript.ExplanationHolder, Double> function
) {
SearchLookup lookup = mock(SearchLookup.class);
LeafSearchLookup leafLookup = mock(LeafSearchLookup.class);
Expand All @@ -236,7 +258,7 @@ public ScoreScript newInstance(LeafReaderContext ctx) throws IOException {
return new ScoreScript(script.getParams(), lookup, indexSearcher, leafReaderContext) {
@Override
public double execute(ExplanationHolder explanation) {
return function.apply(explanation);
return function.apply(this, explanation);
}
};
}
Expand Down
Loading