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.

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

Correct boost in script_score query and error on negative scores #52478

Merged
merged 5 commits into from
Feb 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion docs/reference/query-dsl/script-score-query.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,12 @@ scores be positive or `0`.
--

`min_score`::
(Optional, float) Documents with a <<relevance-scores,relevance score>> lower
(Optional, float) Documents with a score lower
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you remove the reference to the score docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before we were referencing relevance scores, while I think the goal of script_score query is to calculate custom scores through some scripts, not a traditional textual relevance score.

than this floating point number are excluded from the search results.

`boost`::
(Optional, float) Documents' scores produced by `script` are
multiplied by `boost` to produce final documents' scores. Defaults to `1.0`.

[[script-score-query-notes]]
==== Notes
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
# Integration tests for ScriptScoreQuery using Painless
setup:
- skip:
version: " - 7.9.99"
reason: "boost was corrected in script_score query from 8.0"
- do:
indices.create:
index: test_index
body:
settings:
index:
number_of_shards: 1
number_of_replicas: 0
mappings:
properties:
k:
type: keyword
i:
type: integer

- do:
bulk:
index: test_index
refresh: true
body:
- '{"index": {"_id": "1"}}'
- '{"k": "k", "i" : 1}'
- '{"index": {"_id": "2"}}'
- '{"k": "kk", "i" : 2}'
- '{"index": {"_id": "3"}}'
- '{"k": "kkk", "i" : 3}'
---
"Boost script_score":
- do:
search:
index: test_index
body:
query:
script_score:
query: {match_all: {}}
script:
source: "doc['i'].value * _score"
boost: 10

- match: { hits.total.value: 3 }
- match: { hits.hits.0._score: 30 }
- match: { hits.hits.1._score: 20 }
- match: { hits.hits.2._score: 10 }

---
"Boost script_score and boost internal query":
- do:
search:
index: test_index
body:
query:
script_score:
query: {match_all: {boost: 5}}
script:
source: "doc['i'].value * _score"
boost: 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use a different score here, e.g.: 5 to make it more clear.


- match: { hits.total.value: 3 }
- match: { hits.hits.0._score: 150 }
- match: { hits.hits.1._score: 100 }
- match: { hits.hits.2._score: 50 }

---
"Boost script_score with explain":
- do:
search:
index: test_index
body:
explain: true
query:
script_score:
query: {term: {"k": "kkk"}}
script:
source: "doc['i'].value"
boost: 10

- match: { hits.total.value: 1 }
- match: { hits.hits.0._score: 30 }
- match: { hits.hits.0._explanation.value: 30 }
- match: { hits.hits.0._explanation.details.0.description: "boost" }
- match: { hits.hits.0._explanation.details.0.value: 10}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import org.apache.lucene.search.Scorer;
import org.apache.lucene.search.BulkScorer;
import org.apache.lucene.util.Bits;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.Version;
import org.elasticsearch.script.ScoreScript;
import org.elasticsearch.script.ScoreScript.ExplanationHolder;
Expand Down Expand Up @@ -85,7 +84,7 @@ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float bo
}
boolean needsScore = scriptBuilder.needs_score();
ScoreMode subQueryScoreMode = needsScore ? ScoreMode.COMPLETE : ScoreMode.COMPLETE_NO_SCORES;
Weight subQueryWeight = subQuery.createWeight(searcher, subQueryScoreMode, boost);
Weight subQueryWeight = subQuery.createWeight(searcher, subQueryScoreMode, 1.0f);

return new Weight(this){
@Override
Expand All @@ -95,7 +94,7 @@ public BulkScorer bulkScorer(LeafReaderContext context) throws IOException {
if (subQueryBulkScorer == null) {
return null;
}
return new ScriptScoreBulkScorer(subQueryBulkScorer, subQueryScoreMode, makeScoreScript(context));
return new ScriptScoreBulkScorer(subQueryBulkScorer, subQueryScoreMode, makeScoreScript(context), boost);
} else {
return super.bulkScorer(context);
}
Expand All @@ -112,7 +111,7 @@ public Scorer scorer(LeafReaderContext context) throws IOException {
if (subQueryScorer == null) {
return null;
}
Scorer scriptScorer = new ScriptScorer(this, makeScoreScript(context), subQueryScorer, subQueryScoreMode, null);
Scorer scriptScorer = new ScriptScorer(this, makeScoreScript(context), subQueryScorer, subQueryScoreMode, boost, null);
if (minScore != null) {
scriptScorer = new MinScoreScorer(this, scriptScorer, minScore);
}
Expand All @@ -127,11 +126,11 @@ public Explanation explain(LeafReaderContext context, int doc) throws IOExceptio
}
ExplanationHolder explanationHolder = new ExplanationHolder();
Scorer scorer = new ScriptScorer(this, makeScoreScript(context),
subQueryWeight.scorer(context), subQueryScoreMode, explanationHolder);
subQueryWeight.scorer(context), subQueryScoreMode, 1f, explanationHolder);
int newDoc = scorer.iterator().advance(doc);
assert doc == newDoc; // subquery should have already matched above
float score = scorer.score();
float score = scorer.score(); // score without boost

Explanation explanation = explanationHolder.get(score, needsScore ? subQueryExplanation : null);
if (explanation == null) {
// no explanation provided by user; give a simple one
Expand All @@ -143,7 +142,10 @@ public Explanation explain(LeafReaderContext context, int doc) throws IOExceptio
explanation = Explanation.match(score, desc);
}
}

if (boost != 1f) {
explanation = Explanation.match(boost * explanation.getValue().floatValue(), "Boosted score, product of:",
Explanation.match(boost, "boost"), explanation);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest wrapping the explanation instead of modifying it in-place: create the scorer with boost=1f a couple lines above, and then here:

if (boost != 1f) {
    explanation = Explanation.match(boost * explanation.getValue().floatValue(), "Boosted score, product of:",
        Explanation.match(boost, "boost"),
        explanation);
}

if (minScore != null && minScore > explanation.getValue().floatValue()) {
explanation = Explanation.noMatch("Score value is too low, expected at least " + minScore +
" but got " + explanation.getValue(), explanation);
Expand Down Expand Up @@ -203,16 +205,18 @@ public int hashCode() {
private static class ScriptScorer extends Scorer {
private final ScoreScript scoreScript;
private final Scorer subQueryScorer;
private final float boost;
private final ExplanationHolder explanation;

ScriptScorer(Weight weight, ScoreScript scoreScript, Scorer subQueryScorer,
ScoreMode subQueryScoreMode, ExplanationHolder explanation) {
ScoreMode subQueryScoreMode, float boost, ExplanationHolder explanation) {
super(weight);
this.scoreScript = scoreScript;
if (subQueryScoreMode == ScoreMode.COMPLETE) {
scoreScript.setScorer(subQueryScorer);
}
this.subQueryScorer = subQueryScorer;
this.boost = boost;
this.explanation = explanation;
}

Expand All @@ -221,12 +225,13 @@ public float score() throws IOException {
int docId = docID();
scoreScript.setDocument(docId);
float score = (float) scoreScript.execute(explanation);
if (score == Float.NEGATIVE_INFINITY || Float.isNaN(score)) {
throw new ElasticsearchException(
"script_score query returned an invalid score [" + score + "] for doc [" + docId + "].");
if (score < 0f || Float.isNaN(score)) {
Copy link
Contributor

@jtibshirani jtibshirani Mar 3, 2020

Choose a reason for hiding this comment

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

@mayya-sharipova it looks like this PR also fixed a bug in script_score queries where we allowed negative scores. I think we should add a note to the breaking changes docs and also update the PR description to make it clear we included this change.

Copy link
Contributor Author

@mayya-sharipova mayya-sharipova Mar 3, 2020

Choose a reason for hiding this comment

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

@jtibshirani Thanks. Even before without this change a user would get an error if their script_score query produced a negative score. They would just get it from a different place, one of them from the Lucene here

So the only thing changed from a user perspective is an error message and error status code (before was 500, not 400x). Do you think it warrants a breaking change notice?
+1 for include this in the PR description

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried this out using an Elasticsearch 7.6 build, and didn't receive an error:

PUT my_index/_doc/1?refresh
{
  "field": "value"
}

GET my_index/_search
{
  "query": {
    "script_score": {
      "query": {
        "match_all": {}
      },
      "script": {
        "source": "-1000"
      }
    }
  }
}

The line you linked to is an assert, so perhaps these Lucene checks didn't always catch the issue in non-test environments.

Copy link
Contributor Author

@mayya-sharipova mayya-sharipova Mar 4, 2020

Choose a reason for hiding this comment

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

@jtibshirani Thanks for uncovering this. I understood what happened:

  • Before 7.5, script_score query was using ScriptScoreFunction that was returning 400 error with a negative score.
  • From 7.5, we have changed it to not use ScriptScoreFunction but forgot to add a condition for a negative score. But TopScoreDocCollector assertion is tripped, causing fatal error in the dev mode. But I guess we silence these assertions in a production mode as we don't see any visible errors or error log messages.

So, negative scores were wrongly allowed only in 7.5-7.6 versions, so to me it doesn't look like a really breaking change. But I think it is still worth to add a note with explanation in release notes. I will do that. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding an explanation to the release notes makes sense to me. I agree it shouldn't be presented as a typical 'breaking change', it is more like a regression that we fixed. Perhaps we could add a unit test along with the release notes update, to prevent a future regression?

throw new IllegalArgumentException("script_score script returned an invalid score [" + score + "] " +
"for doc [" + docId + "]. Must be a non-negative score!");
}
return score;
return score * boost;
}

@Override
public int docID() {
return subQueryScorer.docID();
Expand All @@ -247,15 +252,17 @@ public float getMaxScore(int upTo) {
private static class ScriptScorable extends Scorable {
private final ScoreScript scoreScript;
private final Scorable subQueryScorer;
private final float boost;
private final ExplanationHolder explanation;

ScriptScorable(ScoreScript scoreScript, Scorable subQueryScorer,
ScoreMode subQueryScoreMode, ExplanationHolder explanation) {
ScoreMode subQueryScoreMode, float boost, ExplanationHolder explanation) {
this.scoreScript = scoreScript;
if (subQueryScoreMode == ScoreMode.COMPLETE) {
scoreScript.setScorer(subQueryScorer);
}
this.subQueryScorer = subQueryScorer;
this.boost = boost;
this.explanation = explanation;
}

Expand All @@ -264,11 +271,11 @@ public float score() throws IOException {
int docId = docID();
scoreScript.setDocument(docId);
float score = (float) scoreScript.execute(explanation);
if (score == Float.NEGATIVE_INFINITY || Float.isNaN(score)) {
throw new ElasticsearchException(
"script_score query returned an invalid score [" + score + "] for doc [" + docId + "].");
if (score < 0f || Float.isNaN(score)) {
throw new IllegalArgumentException("script_score script returned an invalid score [" + score + "] " +
"for doc [" + docId + "]. Must be a non-negative score!");
}
return score;
return score * boost;
}
@Override
public int docID() {
Expand All @@ -284,11 +291,13 @@ private static class ScriptScoreBulkScorer extends BulkScorer {
private final BulkScorer subQueryBulkScorer;
private final ScoreMode subQueryScoreMode;
private final ScoreScript scoreScript;
private final float boost;

ScriptScoreBulkScorer(BulkScorer subQueryBulkScorer, ScoreMode subQueryScoreMode, ScoreScript scoreScript) {
ScriptScoreBulkScorer(BulkScorer subQueryBulkScorer, ScoreMode subQueryScoreMode, ScoreScript scoreScript, float boost) {
this.subQueryBulkScorer = subQueryBulkScorer;
this.subQueryScoreMode = subQueryScoreMode;
this.scoreScript = scoreScript;
this.boost = boost;
}

@Override
Expand All @@ -300,7 +309,7 @@ private LeafCollector wrapCollector(LeafCollector collector) {
return new FilterLeafCollector(collector) {
@Override
public void setScorer(Scorable scorer) throws IOException {
in.setScorer(new ScriptScorable(scoreScript, scorer, subQueryScoreMode, null));
in.setScorer(new ScriptScorable(scoreScript, scorer, subQueryScoreMode, boost, null));
}
};
}
Expand Down