Skip to content

Set maxScore for empty TopDocs to Nan rather than 0 #32938

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

Merged
merged 6 commits into from
Aug 22, 2018
Merged
Show file tree
Hide file tree
Changes from 2 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
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ public void testSearchWithTermsAgg() throws IOException {
assertNull(searchResponse.getSuggest());
assertEquals(Collections.emptyMap(), searchResponse.getProfileResults());
assertEquals(0, searchResponse.getHits().getHits().length);
assertEquals(0f, searchResponse.getHits().getMaxScore(), 0f);
assertEquals(Float.NaN, searchResponse.getHits().getMaxScore(), 0f);
Terms termsAgg = searchResponse.getAggregations().get("agg1");
assertEquals("agg1", termsAgg.getName());
assertEquals(2, termsAgg.getBuckets().size());
Expand Down Expand Up @@ -293,7 +293,7 @@ public void testSearchWithRangeAgg() throws IOException {
assertEquals(Collections.emptyMap(), searchResponse.getProfileResults());
assertEquals(5, searchResponse.getHits().totalHits);
assertEquals(0, searchResponse.getHits().getHits().length);
assertEquals(0f, searchResponse.getHits().getMaxScore(), 0f);
assertEquals(Float.NaN, searchResponse.getHits().getMaxScore(), 0f);
Range rangeAgg = searchResponse.getAggregations().get("agg1");
assertEquals("agg1", rangeAgg.getName());
assertEquals(2, rangeAgg.getBuckets().size());
Expand Down Expand Up @@ -323,7 +323,7 @@ public void testSearchWithTermsAndRangeAgg() throws IOException {
assertNull(searchResponse.getSuggest());
assertEquals(Collections.emptyMap(), searchResponse.getProfileResults());
assertEquals(0, searchResponse.getHits().getHits().length);
assertEquals(0f, searchResponse.getHits().getMaxScore(), 0f);
assertEquals(Float.NaN, searchResponse.getHits().getMaxScore(), 0f);
Terms termsAgg = searchResponse.getAggregations().get("agg1");
assertEquals("agg1", termsAgg.getName());
assertEquals(2, termsAgg.getBuckets().size());
Expand Down Expand Up @@ -375,7 +375,7 @@ public void testSearchWithMatrixStats() throws IOException {
assertEquals(Collections.emptyMap(), searchResponse.getProfileResults());
assertEquals(5, searchResponse.getHits().totalHits);
assertEquals(0, searchResponse.getHits().getHits().length);
assertEquals(0f, searchResponse.getHits().getMaxScore(), 0f);
assertEquals(Float.NaN, searchResponse.getHits().getMaxScore(), 0f);
assertEquals(1, searchResponse.getAggregations().asList().size());
MatrixStats matrixStats = searchResponse.getAggregations().get("agg1");
assertEquals(5, matrixStats.getFieldCount("num"));
Expand Down Expand Up @@ -474,7 +474,7 @@ public void testSearchWithParentJoin() throws IOException {
assertEquals(Collections.emptyMap(), searchResponse.getProfileResults());
assertEquals(3, searchResponse.getHits().totalHits);
assertEquals(0, searchResponse.getHits().getHits().length);
assertEquals(0f, searchResponse.getHits().getMaxScore(), 0f);
assertEquals(Float.NaN, searchResponse.getHits().getMaxScore(), 0f);
assertEquals(1, searchResponse.getAggregations().asList().size());
Terms terms = searchResponse.getAggregations().get("top-tags");
assertEquals(0, terms.getDocCountError());
Expand Down Expand Up @@ -513,7 +513,7 @@ public void testSearchWithSuggest() throws IOException {
assertNull(searchResponse.getAggregations());
assertEquals(Collections.emptyMap(), searchResponse.getProfileResults());
assertEquals(0, searchResponse.getHits().totalHits);
assertEquals(0f, searchResponse.getHits().getMaxScore(), 0f);
assertEquals(Float.NaN, searchResponse.getHits().getMaxScore(), 0f);
assertEquals(0, searchResponse.getHits().getHits().length);
assertEquals(1, searchResponse.getSuggest().size());
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not too happy that all the failures I got came from integration tests and docs tests. I added some assertions to QueryPhaseTests but maybe that is not enough. Suggestions are welcome on where to add tests for this change.


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ Possible response:
},
"hits": {
"total": 3,
"max_score": 0.0,
"max_score": null,
"hits": []
},
"aggregations": {
Expand Down
2 changes: 1 addition & 1 deletion docs/reference/getting-started.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -1141,7 +1141,7 @@ And the response (partially shown):
},
"hits" : {
"total" : 1000,
"max_score" : 0.0,
"max_score" : null,
"hits" : [ ]
},
"aggregations" : {
Expand Down
2 changes: 1 addition & 1 deletion docs/reference/mapping/params/normalizer.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ returns
},
"hits": {
"total": 3,
"max_score": 0.0,
"max_score": null,
"hits": []
},
"aggregations": {
Expand Down
5 changes: 5 additions & 0 deletions docs/reference/migration/migrate_7_0/search.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,8 @@ considerably.
`max_concurrent_shard_requests` used to limit the total number of concurrent shard
requests a single high level search request can execute. In 7.0 this changed to be the
max number of concurrent shard requests per node. The default is now `5`.

==== `max_score` set to `null` when scores are not tracked

`max_score` used to be set to `0` whenever scores are not tracked. `null` is now used
instead which is a more appropriate value for a scenario where scores are not available.
2 changes: 1 addition & 1 deletion docs/reference/search/request-body.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ be set to `true` in the response.
},
"hits": {
"total": 1,
"max_score": 0.0,
"max_score": null,
"hits": []
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ Which should look like:
},
"hits": {
"total" : 0,
"max_score" : 0.0,
"max_score" : null,
"hits" : []
},
"suggest": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ public TopDocs[] topDocs(SearchHit[] hits) throws IOException {
for (LeafReaderContext ctx : context.searcher().getIndexReader().leaves()) {
intersect(weight, innerHitQueryWeight, totalHitCountCollector, ctx);
}
result[i] = new TopDocs(totalHitCountCollector.getTotalHits(), Lucene.EMPTY_SCORE_DOCS, 0);
result[i] = new TopDocs(totalHitCountCollector.getTotalHits(), Lucene.EMPTY_SCORE_DOCS, Float.NaN);
} else {
int topN = Math.min(from() + size(), context.searcher().getIndexReader().maxDoc());
TopDocsCollector<?> topDocsCollector;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ public class Lucene {

public static final ScoreDoc[] EMPTY_SCORE_DOCS = new ScoreDoc[0];

public static final TopDocs EMPTY_TOP_DOCS = new TopDocs(0, EMPTY_SCORE_DOCS, 0.0f);
public static final TopDocs EMPTY_TOP_DOCS = new TopDocs(0, EMPTY_SCORE_DOCS, Float.NaN);

public static Version parseVersion(@Nullable String version, Version defaultVersion, Logger logger) {
if (version == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ public TopDocs[] topDocs(SearchHit[] hits) throws IOException {
if (size() == 0) {
TotalHitCountCollector totalHitCountCollector = new TotalHitCountCollector();
intersect(weight, innerHitQueryWeight, totalHitCountCollector, ctx);
result[i] = new TopDocs(totalHitCountCollector.getTotalHits(), Lucene.EMPTY_SCORE_DOCS, 0);
result[i] = new TopDocs(totalHitCountCollector.getTotalHits(), Lucene.EMPTY_SCORE_DOCS, Float.NaN);
} else {
int topN = Math.min(from() + size(), context.searcher().getIndexReader().maxDoc());
TopDocsCollector<?> topDocsCollector;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public void execute(SearchContext searchContext) throws QueryPhaseExecutionExcep
suggestPhase.execute(searchContext);
// TODO: fix this once we can fetch docs for suggestions
searchContext.queryResult().topDocs(
new TopDocs(0, Lucene.EMPTY_SCORE_DOCS, 0),
new TopDocs(0, Lucene.EMPTY_SCORE_DOCS, Float.NaN),
new DocValueFormat[0]);
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ Collector create(Collector in) {
@Override
void postProcess(QuerySearchResult result) {
final int totalHitCount = hitCountSupplier.getAsInt();
result.topDocs(new TopDocs(totalHitCount, Lucene.EMPTY_SCORE_DOCS, 0), null);
result.topDocs(new TopDocs(totalHitCount, Lucene.EMPTY_SCORE_DOCS, Float.NaN), null);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ public void testIssue11119() throws Exception {

assertThat(response.getHits().getTotalHits(), equalTo(8L));
assertThat(response.getHits().getHits().length, equalTo(0));
assertThat(response.getHits().getMaxScore(), equalTo(0f));
assertThat(response.getHits().getMaxScore(), equalTo(Float.NaN));
Terms terms = response.getAggregations().get("terms");
assertThat(terms, notNullValue());
assertThat(terms.getName(), equalTo("terms"));
Expand Down Expand Up @@ -356,7 +356,7 @@ public void testIssue11119() throws Exception {

assertThat(response.getHits().getTotalHits(), equalTo(8L));
assertThat(response.getHits().getHits().length, equalTo(0));
assertThat(response.getHits().getMaxScore(), equalTo(0f));
assertThat(response.getHits().getMaxScore(), equalTo(Float.NaN));
terms = response.getAggregations().get("terms");
assertThat(terms, notNullValue());
assertThat(terms.getName(), equalTo("terms"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@

import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
import static org.hamcrest.Matchers.instanceOf;

Expand Down Expand Up @@ -103,6 +104,7 @@ private void countTestCase(Query query, IndexReader reader, boolean shouldCollec
final boolean rescore = QueryPhase.execute(context, searcher, checkCancelled -> {});
assertFalse(rescore);
assertEquals(searcher.count(query), context.queryResult().topDocs().totalHits);
assertThat(context.queryResult().topDocs().getMaxScore(), equalTo(Float.NaN));
}

private void countTestCase(boolean withDeletions) throws Exception {
Expand Down Expand Up @@ -172,11 +174,14 @@ public void testPostFilterDisablesCountOptimization() throws Exception {

QueryPhase.execute(context, contextSearcher, checkCancelled -> {});
assertEquals(1, context.queryResult().topDocs().totalHits);
assertThat(context.queryResult().topDocs().scoreDocs.length, equalTo(0));
assertThat(context.queryResult().topDocs().getMaxScore(), equalTo(Float.NaN));

contextSearcher = new IndexSearcher(reader);
context.parsedPostFilter(new ParsedQuery(new MatchNoDocsQuery()));
QueryPhase.execute(context, contextSearcher, checkCancelled -> {});
assertEquals(0, context.queryResult().topDocs().totalHits);
assertThat(context.queryResult().topDocs().getMaxScore(), equalTo(Float.NaN));
reader.close();
dir.close();
}
Expand Down Expand Up @@ -205,13 +210,13 @@ public void testTerminateAfterWithFilter() throws Exception {
context.parsedPostFilter(new ParsedQuery(new TermQuery(new Term("foo", Integer.toString(i)))));
QueryPhase.execute(context, contextSearcher, checkCancelled -> {});
assertEquals(1, context.queryResult().topDocs().totalHits);
assertThat(context.queryResult().topDocs().getMaxScore(), equalTo(1F));
assertThat(context.queryResult().topDocs().scoreDocs.length, equalTo(1));
}
reader.close();
dir.close();
}


public void testMinScoreDisablesCountOptimization() throws Exception {
Directory dir = newDirectory();
final Sort sort = new Sort(new SortField("rank", SortField.Type.INT));
Expand All @@ -230,11 +235,13 @@ public void testMinScoreDisablesCountOptimization() throws Exception {
context.setTask(new SearchTask(123L, "", "", "", null, Collections.emptyMap()));
QueryPhase.execute(context, contextSearcher, checkCancelled -> {});
assertEquals(1, context.queryResult().topDocs().totalHits);
assertThat(context.queryResult().topDocs().getMaxScore(), equalTo(Float.NaN));

contextSearcher = new IndexSearcher(reader);
context.minimumScore(100);
QueryPhase.execute(context, contextSearcher, checkCancelled -> {});
assertEquals(0, context.queryResult().topDocs().totalHits);
assertThat(context.queryResult().topDocs().getMaxScore(), equalTo(Float.NaN));
reader.close();
dir.close();
}
Expand Down Expand Up @@ -289,16 +296,19 @@ public void testInOrderScrollOptimization() throws Exception {

QueryPhase.execute(context, contextSearcher, checkCancelled -> {});
assertThat(context.queryResult().topDocs().totalHits, equalTo((long) numDocs));
assertThat(context.queryResult().topDocs().getMaxScore(), equalTo(1F));
assertNull(context.queryResult().terminatedEarly());
assertThat(context.terminateAfter(), equalTo(0));
assertThat(context.queryResult().getTotalHits(), equalTo((long) numDocs));

contextSearcher = getAssertingEarlyTerminationSearcher(reader, size);
QueryPhase.execute(context, contextSearcher, checkCancelled -> {});
assertThat(context.queryResult().topDocs().totalHits, equalTo((long) numDocs));
assertThat(context.queryResult().topDocs().getMaxScore(), equalTo(1F));
assertTrue(context.queryResult().terminatedEarly());
assertThat(context.terminateAfter(), equalTo(size));
assertThat(context.queryResult().getTotalHits(), equalTo((long) numDocs));
assertThat(context.queryResult().topDocs().getMaxScore(), equalTo(1F));
assertThat(context.queryResult().topDocs().scoreDocs[0].doc, greaterThanOrEqualTo(size));
reader.close();
dir.close();
Expand Down Expand Up @@ -334,12 +344,14 @@ public void testTerminateAfterEarlyTermination() throws Exception {
QueryPhase.execute(context, contextSearcher, checkCancelled -> {});
assertTrue(context.queryResult().terminatedEarly());
assertThat(context.queryResult().topDocs().totalHits, equalTo(1L));
assertThat(context.queryResult().topDocs().getMaxScore(), equalTo(1F));
assertThat(context.queryResult().topDocs().scoreDocs.length, equalTo(1));

context.setSize(0);
QueryPhase.execute(context, contextSearcher, checkCancelled -> {});
assertTrue(context.queryResult().terminatedEarly());
assertThat(context.queryResult().topDocs().totalHits, equalTo(1L));
assertThat(context.queryResult().topDocs().getMaxScore(), equalTo(Float.NaN));
assertThat(context.queryResult().topDocs().scoreDocs.length, equalTo(0));
}

Expand All @@ -348,6 +360,7 @@ public void testTerminateAfterEarlyTermination() throws Exception {
QueryPhase.execute(context, contextSearcher, checkCancelled -> {});
assertTrue(context.queryResult().terminatedEarly());
assertThat(context.queryResult().topDocs().totalHits, equalTo(1L));
assertThat(context.queryResult().topDocs().getMaxScore(), equalTo(1F));
assertThat(context.queryResult().topDocs().scoreDocs.length, equalTo(1));
}
{
Expand All @@ -360,13 +373,15 @@ public void testTerminateAfterEarlyTermination() throws Exception {
QueryPhase.execute(context, contextSearcher, checkCancelled -> {});
assertTrue(context.queryResult().terminatedEarly());
assertThat(context.queryResult().topDocs().totalHits, equalTo(1L));
assertThat(context.queryResult().topDocs().getMaxScore(), greaterThan(0f));
assertThat(context.queryResult().topDocs().scoreDocs.length, equalTo(1));

context.setSize(0);
context.parsedQuery(new ParsedQuery(bq));
QueryPhase.execute(context, contextSearcher, checkCancelled -> {});
assertTrue(context.queryResult().terminatedEarly());
assertThat(context.queryResult().topDocs().totalHits, equalTo(1L));
assertThat(context.queryResult().topDocs().getMaxScore(), equalTo(Float.NaN));
assertThat(context.queryResult().topDocs().scoreDocs.length, equalTo(0));
}
{
Expand All @@ -376,6 +391,7 @@ public void testTerminateAfterEarlyTermination() throws Exception {
QueryPhase.execute(context, contextSearcher, checkCancelled -> {});
assertTrue(context.queryResult().terminatedEarly());
assertThat(context.queryResult().topDocs().totalHits, equalTo(1L));
assertThat(context.queryResult().topDocs().getMaxScore(), greaterThan(0f));
assertThat(context.queryResult().topDocs().scoreDocs.length, equalTo(1));
assertThat(collector.getTotalHits(), equalTo(1));
context.queryCollectors().clear();
Expand All @@ -387,6 +403,7 @@ public void testTerminateAfterEarlyTermination() throws Exception {
QueryPhase.execute(context, contextSearcher, checkCancelled -> {});
assertTrue(context.queryResult().terminatedEarly());
assertThat(context.queryResult().topDocs().totalHits, equalTo(1L));
assertThat(context.queryResult().topDocs().getMaxScore(), equalTo(Float.NaN));
assertThat(context.queryResult().topDocs().scoreDocs.length, equalTo(0));
assertThat(collector.getTotalHits(), equalTo(1));
}
Expand Down Expand Up @@ -539,19 +556,19 @@ public void testIndexSortScrollOptimization() throws Exception {
dir.close();
}

static IndexSearcher getAssertingEarlyTerminationSearcher(IndexReader reader, int size) {
private static IndexSearcher getAssertingEarlyTerminationSearcher(IndexReader reader, int size) {
return new IndexSearcher(reader) {
protected void search(List<LeafReaderContext> leaves, Weight weight, Collector collector) throws IOException {
final Collector in = new AssertingEalyTerminationFilterCollector(collector, size);
final Collector in = new AssertingEarlyTerminationFilterCollector(collector, size);
super.search(leaves, weight, in);
}
};
}

private static class AssertingEalyTerminationFilterCollector extends FilterCollector {
private static class AssertingEarlyTerminationFilterCollector extends FilterCollector {
private final int size;

AssertingEalyTerminationFilterCollector(Collector in, int size) {
AssertingEarlyTerminationFilterCollector(Collector in, int size) {
super(in);
this.size = size;
}
Expand Down