-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Shortcut counts on exists queries #39570
Shortcut counts on exists queries #39570
Conversation
Pinging @elastic/es-search |
server/src/test/java/org/elasticsearch/search/query/QueryPhaseTests.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/query/TopDocsCollectorContext.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/search/query/QueryPhaseTests.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/query/TopDocsCollectorContext.java
Show resolved
Hide resolved
068b113
to
efd7457
Compare
@jpountz @jtibshirani I added an update and left a question about also handling |
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.
It looks good to me overall. I left one comment about a null check.
server/src/main/java/org/elasticsearch/search/query/TopDocsCollectorContext.java
Outdated
Show resolved
Hide resolved
@jpountz I added the null checks, didn't go into adding specific tests for this though because these cases seem to be triggered by the randomization frequently enough. Let me know if there is anything else to add. |
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.
One comment, other than that LGTM. Feel free to push without further review.
server/src/main/java/org/elasticsearch/search/query/TopDocsCollectorContext.java
Outdated
Show resolved
Hide resolved
`TopDocsCollectorContext` can already shortcut hit counts on `match_all` and `term` queries when there are no deletions. This change adds this ability for `exists` queries if the index doesn't have deletions and fields are indexed. Closes elastic#37475
`TopDocsCollectorContext` can already shortcut hit counts on `match_all` and `term` queries when there are no deletions. This change adds this ability for `exists` queries if the index doesn't have deletions and fields are indexed. Closes #37475
@jpountz I ported this back to 7.1, do you think it should also go into 7.0? Maybe not is my feeling atm. |
I wouldn't push it to 7.0. |
TopDocsCollectorContext
can already shortcut hit counts onmatch_all
andterm
queries when there are no deletions.This change adds this ability for
exists
queries if the index doesn't have deletions and fields are indexed.Closes #37475