Rewrite range queries with open bounds to exists query #26160
Conversation
This change rewrites range query with open bounds to an exists query that should be faster to execute. Fixes elastic#22640
jimczi
added
:Search/Search
| return new MatchNoDocsQuery("No mappings yet"); | ||
| } | ||
| if (fieldNamesFieldType.isEnabled()) { | ||
| return ExistsQueryBuilder.newFilter(context, fieldName); |
There was a problem hiding this comment.
maybe leave a comment that we need to check whether the fieldNames mapper is enabled because otherwise we fail exists queries
* Rewrite range queries with open bounds to exists query This change rewrites range query with open bounds to an exists query that should be faster to execute. Fixes #22640
* master: (30 commits) Rewrite range queries with open bounds to exists query (elastic#26160) Fix eclipse compilation problem (elastic#26170) Epoch millis and second formats parse float implicitly (Closes elastic#14641) (elastic#26119) fix SplitProcessor targetField test (elastic#26178) Fixed typo in README.textile (elastic#26168) Fix incorrect class name in deleteByQuery docs (elastic#26151) Move more token filters to analysis-common module reindex: automatically choose the number of slices (elastic#26030) Fix serialization of the `_all` field. (elastic#26143) percolator: Hint what clauses are important in a conjunction query based on fields Remove unused Netty-related settings (elastic#26161) Remove SimpleQueryStringIT#testPhraseQueryOnFieldWithNoPositions. Tests: reenable ShardReduceIT#testIpRange. Allow `ClusterState.Custom` to be created on initial cluster states (elastic#26144) Teach the build about betas and rcs (elastic#26066) Fix wrong header level inner hits: Unfiltered nested source should keep its full path Document how to import Lucene Snapshot libs when elasticsearch clients (elastic#26113) Use `global_ordinals_hash` execution mode when sorting by sub aggregations. (elastic#26014) Make the README use a single type in examples. (elastic#26098) ...
|
@jimczi when we first wrote the range query rewrite stuff I initially had this optimisation to rewrite to an exists query in the case where the query entirely overlaps the range of values on the shard, but when I did this I ran into issues with highlighting (tests failed). This was because an exists query will not highlight the value that matched the range. I assume all the tests passed this time but I am curious what has changed, will highlighting still work on these range queries? I know there is a argument to say that doing highlighting on a numeric field should not be supported but I think it would be weird if highlighting on a numeric field sometimes worked (when not rewritten to exists) and sometimes didn't work (when rewritten to exists). If we don't want to support highlighting on numeric/date fields maybe we should explicitly forbid it? |
|
I don't know when it was removed but we don't support range query in the highlighters. These queries are not extracted nor rewritten by the three remaining highlighters ( |
|
@jimczi ok, it could well have been on the old numeric types, but as we don't support numeric fields int eh remaining highlighters my fear has been resolved. Thanks for looking into it. ++ on this change. |
|
It's not explicit though, we accept the query if it targets a numeric field but we ignore the numeric queries so nothing is highlighted. Maybe we should fail the query if the field is a numeric field and its using one of the es highlighters (but should work if you use a custom highlighter) ? |
|
I like the idea but we might have to be careful with wildcards in case they match both string and numeric fields? |
|
We should definitely add something to the documentation to say what field types can be highlighted I don't see anything about it at the moment (although maybe I'm missing it): https://www.elastic.co/guide/en/elasticsearch/reference/6.0/search-request-highlighting.html |
This change rewrites range query with open bounds to an exists query that should be faster to execute.
Fixes #22640