Add the ability to set the number of hits to track accurately #36357

jimczi · 2018-12-07T10:45:15Z

In Lucene 8 searches can skip non-competitive hits if the total hit count is not requested.
It is also possible to track the number of hits up to a certain threshold. This is a trade off to speed up searches while still being able to know a lower bound of the total hit count. This change adds the ability to set this threshold directly in the track_total_hits search option. A boolean value (true, false) indicates whether the total hit count should be tracked in the response. When set as an integer this option allows to compute a lower bound of the total hits while preserving the ability to skip non-competitive hits when enough matches have been collected.

Relates #33028

elasticmachine · 2018-12-07T10:45:17Z

Pinging @elastic/es-search

jpountz

I left some comments but like it in general.

jpountz · 2018-12-07T12:47:52Z

docs/reference/search/request/track-total-hits.asciidoc

+[[search-request-track-total-hits]]
+=== Track total hits
+
+The total hit count can't be computed accurately without visiting all matches,


maybe add "generally" since match_all and term queries without deletions can still get their hit count computed for free

jpountz · 2018-12-07T12:48:36Z

docs/reference/search/request/track-total-hits.asciidoc

+which is costly for queries that match lots of documents. The `track_total_hits`
+parameter allows you to control how the total number of hits should be tracked.
+When set to `true` the search response will track the number of hits that match
+the query accurately:


maybe be explicit and say that total.relation will always be equal to eq when track_total_hits is set to true?

jpountz · 2018-12-07T12:49:55Z

docs/reference/search/request/track-total-hits.asciidoc

+{
+    "track_total_hits": true,
+    "query" : {
+        "match_all" : {}


this example might be a bit confusing since the hit count of match_all can be computed easily. Use eg. a range query instead?

jpountz · 2018-12-07T12:51:33Z

docs/reference/search/request/track-total-hits.asciidoc

+<1> The total number of hits is unknown.
+
+Given that it is often enough to have a lower bound of the number of hits,
+such as "there are more than 1000 hits", it is also possible to set


maybe rephrase to "there are 1000 hits or more" since the lower bound is inclusive

or "at least"

jpountz · 2018-12-07T13:30:30Z

server/src/main/java/org/elasticsearch/action/search/SearchPhaseController.java

+                if (totalHits < trackTotalHitsUpTo) {
+                    return new TotalHits(totalHits, totalHitsRelation);
+                } else {
+                    return new TotalHits(trackTotalHitsUpTo, Relation.GREATER_THAN_OR_EQUAL_TO);


maybe add a comment: Lucene can give us more information but it seems that we never want to return accurate hit counts if the count is greater than the upTo so that users do not get used to accurate hit counts when they didn't actually ask for accuracy?

jpountz · 2018-12-07T13:36:50Z

server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java

+        }
+        if (restRequest.paramAsBoolean(TOTAL_HITS_AS_INT_PARAM, false)) {
+            throw new IllegalArgumentException("[" + TOTAL_HITS_AS_INT_PARAM + "] cannot be used " +
+                "if the tracking of total hits is not accurate (true) or disabled (false), got " + trackTotalHitsUpTo);


is the message right? "not accurate" happens when the parameter is an integer value rather than true?

The message is completely off, thanks. I pushed a fix

jpountz · 2018-12-07T14:08:39Z

server/src/main/java/org/elasticsearch/search/query/TopDocsCollectorContext.java

                                                 boolean hasFilterCollector) throws IOException {
            super(reader, query, sortAndFormats, scrollContext.lastEmittedDoc, numHits, trackMaxScore,
-                trackTotalHits, hasFilterCollector);
+                SearchContext.TRACK_TOTAL_HITS_ACCURATE, hasFilterCollector);


why accurate? we don't need hit counts since we already have it via numHits?

Yes the logic is wrong here, I reverted it to disable the tracking if the scroll context already contains the total hits:
331d8fe

jimczi

Thanks @jpountz, I pushed some commits to address your comments.

In Lucene 8 searches can skip non-competitive hits if the total hit count is not requested. It is also possible to track the number of hits up to a certain threshold. This is a trade off to speed up searches while still being able to know a lower bound of the total hit count. This change adds the ability to set this threshold directly in the track_total_hits search option. A boolean value (true, false) indicates whether the total hit count should be tracked in the response. When set as an integer this option allows to compute a lower bound of the total hits while preserving the ability to skip non-competitive hits when enough matches have been collected. Relates elastic#33028

The merge sort of sorted segments can produce an invalid sort if the sort field is an Integer/Long that uses reverse order and contains values equal to Integer/Long#MIN_VALUE. These values are always sorted first during a merge (instead of last because of the reverse order) due to this bug. Indices affected by the bug can be detected by running the CheckIndex command on a distribution that contains the fix (7.6+).

jimczi · 2018-12-26T08:45:08Z

run gradle build tests 1

jimczi · 2018-12-26T08:45:21Z

run gradle build tests 2

jpountz

LGTM

karmi · 2019-01-06T08:01:54Z

docs/reference/search/uri-request.asciidoc

 of the total number of hits that match the query.
-(see <<index-modules-index-sorting,_Index Sorting_>> for more details).
-Defaults to true.
+It also accepts an integer which in this case represents the number of


@jimczi , this is problematic in strongly-typed languages, since the track_total_hits parameter is defined as a boolean, and eg. the Go test suite fails with cannot use 4 (type int) as type bool in field value:

elasticsearch/rest-api-spec/src/main/resources/rest-api-spec/test/search/220_total_hits_object.yml

Line 117 in b34e7d4

track_total_hits: 4

I understand the motivation here, but we should at least revisit the parameter definitions, and support something like "type" : ["boolean","number]", so the code generators can make decisions here.

jimczi added >feature :Search/Search Search-related issues that do not fall into other categories v7.0.0 labels Dec 7, 2018

jpountz reviewed Dec 7, 2018

View reviewed changes

jimczi commented Dec 7, 2018

View reviewed changes

jimczi force-pushed the track_total_hits_lower_bound branch from 63c233e to c840019 Compare December 9, 2018 19:12

jimczi added 9 commits December 9, 2018 20:54

fix wrong assignement when track_total_hits is disabled

0ff9521

update docs

61c176d

fix trackTotalHitsUpTo propagation

d070f1a

address review

4582c6f

address review bis

36870f7

optimize the logic for track total hits on scroll queries

b775ddc

fix docs

6287563

jimczi force-pushed the track_total_hits_lower_bound branch from c840019 to 6287563 Compare December 9, 2018 19:55

Merge branch 'master' into track_total_hits_lower_bound

60eccb3

jimczi added 3 commits January 2, 2019 10:06

Merge branch 'master' into track_total_hits_lower_bound

f54276e

fix ut

67f1dea

Merge branch 'master' into track_total_hits_lower_bound

e865f8d

jpountz approved these changes Jan 4, 2019

View reviewed changes

Merge branch 'master' into track_total_hits_lower_bound

d1de0fa

jimczi merged commit e38cf1d into elastic:master Jan 4, 2019

jimczi deleted the track_total_hits_lower_bound branch January 4, 2019 19:36

karmi reviewed Jan 6, 2019

View reviewed changes

jimczi mentioned this pull request Jan 7, 2019

Update CompositeAggregationBuilder.java #36278

Closed

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to set the number of hits to track accurately #36357

Add the ability to set the number of hits to track accurately #36357

jimczi commented Dec 7, 2018

elasticmachine commented Dec 7, 2018

jpountz left a comment

jpountz Dec 7, 2018

jpountz Dec 7, 2018

jpountz Dec 7, 2018

jpountz Dec 7, 2018

jpountz Dec 7, 2018

jpountz Dec 7, 2018

jimczi Dec 7, 2018

jpountz Dec 7, 2018

jimczi Dec 7, 2018

jpountz Dec 7, 2018

jimczi Dec 7, 2018

jimczi left a comment

jimczi commented Dec 26, 2018

jimczi commented Dec 26, 2018

jpountz left a comment

karmi Jan 6, 2019

Add the ability to set the number of hits to track accurately #36357

Add the ability to set the number of hits to track accurately #36357

Conversation

jimczi commented Dec 7, 2018

elasticmachine commented Dec 7, 2018

jpountz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimczi left a comment

Choose a reason for hiding this comment

jimczi commented Dec 26, 2018

jimczi commented Dec 26, 2018

jpountz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment