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

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

Merged
merged 5 commits into from
Feb 24, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7c9f5f8
Correct boost calculation in script_score query
mayya-sharipova Feb 18, 2020
1d8313e
Address feedback
mayya-sharipova Feb 20, 2020
96db563
Address Feedback
mayya-sharipova Feb 24, 2020
c16c444
Remove unused imports
mayya-sharipova Feb 24, 2020
8882f39
Merge remote-tracking branch 'upstream/master' into script_score_boost
mayya-sharipova Feb 24, 2020
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
View file
Open in desktop
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
86 changes: 86 additions & 0 deletions ...s/lang-painless/src/test/resources/rest-api-spec/test/painless/110_script_score_boost.yml
View file
Open in desktop
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: 10}}
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: 300 }
- match: { hits.hits.1._score: 200 }
- match: { hits.hits.2._score: 100 }

---
"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}
43 changes: 30 additions & 13 deletions server/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@
import org.elasticsearch.script.Script;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;
import java.util.Set;

Expand Down Expand Up @@ -85,7 +88,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 +98,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 +115,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,12 +130,14 @@ 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, boost, 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 already computed with boost

Explanation explanation = explanationHolder.get(score, needsScore ? subQueryExplanation : null);


if (explanation == null) {
// no explanation provided by user; give a simple one
String desc = "script score function, computed with script:\"" + script + "\"";
Expand All @@ -143,7 +148,12 @@ public Explanation explain(LeafReaderContext context, int doc) throws IOExceptio
explanation = Explanation.match(score, desc);
}
}

if (boost != 1.0f) {
Copy link
Contributor

@matriv matriv Feb 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This seems to be executed also when explanation == null? Or I'm missing something?
Maybe it worths checking for a test that when the boost is != 1 and explanation is false there is no explanation returned regarding the boost?

Also, why not return an explanation if the boost is 1, since it's asked by the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way we handle explanation object is different from other queries, which may be confusing. In script_score query a user can provide his/her custom explanation.

This seems to be executed also when explanation == null?

Not exactly. We start with explanation that is user provided explanation for the script, and if it null, it is substituted by our standard explanation (line 143), so on in line 151 explanation is never null.

Maybe it worths checking for a test that when the boost is != 1 and explanation is false there is no explanation returned regarding the boost?

When is explanation is not asked (explain = false in a search request), we will not even go to this method. This method is executed only when a user requests an explanation.

Also, why not return an explanation if the boost is 1, since it's asked by the user?

I was thinking since boost is an optional parameter, when it is not provided by a user, there is no need to provide an explanation about it.

List<Explanation> subs = new ArrayList<>();
subs.addAll(Arrays.asList(explanation.getDetails()));
subs.add(Explanation.match(boost, "boost"));
explanation = Explanation.match(explanation.getValue(), explanation.getDescription(), subs);
}
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,30 +213,33 @@ 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;
}

@Override
public float score() throws IOException {
int docId = docID();
scoreScript.setDocument(docId);
float score = (float) scoreScript.execute(explanation);
float score = (float) scoreScript.execute(explanation) * boost;
if (score == Float.NEGATIVE_INFINITY || Float.isNaN(score)) {
throw new ElasticsearchException(
"script_score query returned an invalid score [" + score + "] for doc [" + docId + "].");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better if this error message returned the value produced by the script without the boost.

}
return score;
}

@Override
public int docID() {
return subQueryScorer.docID();
Expand All @@ -247,23 +260,25 @@ 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;
}

@Override
public float score() throws IOException {
int docId = docID();
scoreScript.setDocument(docId);
float score = (float) scoreScript.execute(explanation);
float score = (float) scoreScript.execute(explanation) * boost;
if (score == Float.NEGATIVE_INFINITY || Float.isNaN(score)) {
throw new ElasticsearchException(
"script_score query returned an invalid score [" + score + "] for doc [" + docId + "].");
Expand All @@ -284,11 +299,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 +317,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