Reintroduce shortcut total hit count when collecting hits #94170
Conversation
javanna
added
>enhancement
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
javanna
changed the title
|
Hi @javanna, I've created a changelog YAML for you. |
| @@ -503,14 +503,11 @@ Looking at the previous example: | |||
| // TESTRESPONSE[s/(?<=[" ])\d+(\.\d+)?/$body.$_path/] | |||
|
|
|||
|
|
|||
| We see a single collector named `SimpleTopScoreDocCollector` wrapped into | |||
| `CancellableCollector`. `SimpleTopScoreDocCollector` is the default "scoring and | |||
There was a problem hiding this comment.
I rephrased it to no longer mention CancellableCollector as that does not show for a while as far as I can see. We could extend profile further to expand this collector to its children too, but I kept things simple for now.
| } else { | ||
| throw new CollectionTerminatedException(); | ||
| } | ||
| } |
There was a problem hiding this comment.
minor adjustment, I found it easier to parse knowing that the two code blocks do exactly the same.
| assertThat(context.queryResult().topDocs().topDocs.totalHits.value, equalTo(1L)); | ||
| assertThat(context.queryResult().topDocs().topDocs.totalHits.relation, equalTo(TotalHits.Relation.EQUAL_TO)); | ||
| assertThat(context.queryResult().topDocs().topDocs.scoreDocs.length, equalTo(0)); | ||
| } |
There was a problem hiding this comment.
I moved this block above and used a different search context instance, because it is effectively not possible to update the parsed query (as the rewritten query is write once internally). This made us the match_all multiple times, instead of the bool query. Fixed and verified that the behaviour is not changing, as the total hit count cannot be shortcut for this query.
There was a problem hiding this comment.
The change looks correct, with two things we might need to check:
- MultiCollector#scoreMode() might not always return the appropriate score mode with this change, it would be good to check that this change doesn't disable e.g. dynamic pruning on disjunctions.
- Intuitively this change should improve performance with queries that support short-circuiting and degrade performance otherwise as queries will need to collect into two collectors. Hopefully the overhead is small.
|
I decided to go for #94858 instead, as this approach had some side effects caused by using |
With #89047 we removed the shortcut total hit count functionality that was performed by Elasticsearch in favour of relying on
TotalHitCountCollectorin Lucene to do the same, and apply that more broadly as well as such mechanism supports shortcutting the total hit count for more queries than just match_all and query.With that change we lost the ability to shortcut the total hit count when collecting hits, as
TopScoreDocsCollectorand TopFieldDocsCollectordon't currently rely onWeight#countlikeTotalHitCountCollector` does to shortcut the total hit count.This commit reintroduces the ability to shortcut total hit count when collecting hits, by relying on a separate total hit count collector (potentially early terminated).
This should reintroduce the previous performance characteristics of match_all and term queries, for which we observed a performance regression when size was greater than zero. We can expect a broader impact of the optimization than before though, thanks to the broader support for the shortcut in Lucene.