-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Add an option to early terminate document collection when searching/counting #6885
Conversation
TODO:
|
@@ -240,6 +259,10 @@ public void writeTo(StreamOutput out) throws IOException { | |||
out.writeOptionalString(preference); | |||
out.writeBytesReference(source); | |||
out.writeStringArray(types); | |||
|
|||
if (out.getVersion().onOrAfter(Version.V_1_4_0)) { | |||
out.writeVInt(threshold); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you use vint you can't write negative values :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just go with threshold + 1
on the write side and threshold - 1
on the read side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to just change the default to 0, it does not really make sense for the user to set it to 0. maybe even throw an exception when they want to set it.
left some comments - I like it... |
Updated PR:
TODO:
It seems like there is no equivalent of Another way would be to somehow add the count-specific param |
} | ||
|
||
@Override | ||
public void setScorer(Scorer scorer) throws IOException { | ||
this.exists = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on removing this
I have added a new The search request with the new param looks as follows: curl -XGET 'localhost:9200/_search' -d '
{
terminate_after_count: 1,
"query" : ...
}' or curl -XGET 'localhost:9200/_search?terminate_after_count=1' -d '
{
"query" : ...
}' and for curl -XGET 'localhost:9200/_count?terminate_after_count=1' -d '
{
"query" : ...
}' Both the |
@clintongormley any thoughts on the naming/api for this? |
Updated PR:
TODO:
|
@areek perhaps |
@clintongormley the problem with |
@areek i agree with you about the terminate_after count. True/false does make more sense. But if the user does specify |
Updated PR:
{
"took": 1,
"timed_out": false,
"terminated_early": true,
"_shards": {
...
},
"hits": {
...
}
} The |
@s1monw It would be awesome to have this reviewed. |
return this; | ||
} | ||
|
||
public int terminateAfterCount() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we usually call the getter also terminateAfter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops refactoring miss. will fix
I left a couple of comments but it looks great so far |
@@ -63,6 +63,9 @@ query. | |||
|
|||
|default_operator |The default operator to be used, can be `AND` or | |||
`OR`. Defaults to `OR`. | |||
|
|||
|terminate_after |The maximum count for each shard, upon reaching which the query |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add coming[1.4.0]
here I guess?
@@ -62,6 +62,10 @@ that point when expired. Defaults to no timeout. | |||
`query_and_fetch`. Defaults to `query_then_fetch`. See | |||
<<search-request-search-type,_Search Type_>> for | |||
more details on the different types of search that can be performed. | |||
|
|||
|`terminate_after` |The maximum number of documents to collect for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add coming[1.4.0]
here I guess?
@areek Docs look good, I'd just add a mention of the key in the response when the request has been terminated early |
did another review and this LGTM too |
Mentioned the added key |
Allow users to control document collection termination, if a specified terminate_after number is set. Upon setting the newly added parameter, the response will include a boolean terminated_early flag, indicating if the document collection for any shard terminated early. closes elastic#6876
added support for terminate_after elastic/elasticsearch#6885
The idea is to add an option which will let the user control the number of matched documents collected per shard before scoring (if applicable). This will be helpful for exists-type functionality or when not all matched document has to be scored.
Closes #6876