Add a shard filter search phase to pre-filter shards based on query rewriting #25658

s1monw · 2017-07-11T21:13:42Z

Today if we search across a large amount of shards we hit every shard. Yet, it's quite
common to search across an index pattern for time based indices but filtering will exclude
all results outside a certain time range ie. now-3d. While the search can potentially hit
hundreds of shards the majority of the shards might yield 0 results since there is not document
that is within this date range. Kibana for instance does this regularly but used _field_stats
to optimize the indexes they need to query. Now with the deprecation of _field_stats and it's upcoming
removal a single dashboard in kibana can potentially turn into searches hitting hundreds or thousands
of shards and that can easily cause search rejections even though the most of the requests are
very likely super cheap and only need a query rewriting to early terminate with 0 results.

This change adds a pre-filter phase for searches that can, if the number of shards are higher than
a the pre_filter_shard_size threshold (defaults to 128 shards), fan out to the shards
and check if the query can potentially match any documents at all. While false positives are possible,
a negative response means that no matches are possible. These requests are not subject to rejection
and can greatly reduce the number of shards a request needs to hit. The approach here is preferable
to the kibana approach with field stats since it correctly handles aliases and uses the correct
threadpools to execute these requests. Further it's completely transparent to the user and improves
scalability of elasticsearch in general on large clusters.

…ewriting Today if we search across a large amount of shards we hit every shard. Yet, it's quite common to search across an index pattern for time based indices but filtering will exclude all results outside a certain time range ie. `now-3d`. While the search can potentially hit hunderets of shards the majority of the shards might yield 0 results since there is not document that is within this date range. Kibana for instance does this regularly but used `_field_stats` to optimzie the indice they need to query. Now with the deprecation of `_field_stats` and it's upcoming removal a single dashboard in kibanan can potentially turn into searches hitting hunderets or thousands of shards and that can easily cause search rejections even though the most of the requests are very likely super cheap and only need a query rewriting to early terminate with 0 results. This change adds a pre-filter phase for searches that can, if the number of shards are higher than a the `pre_filter_shards_after` threshold (defaults to 128 shards), fan out to the shards and check if the query can potentially match any documents at all. While false positives are possible, a negative response means that no matches are possible. These requests are not subject to rejection and can greatly reduce the number of shards a request needs to hit. The approach here is preferable to the kibana approach with field stats since it correctly handles aliases and uses the correct threadpools to execute these requests. Further it's completely transparent to the user and improves scalability of elasticsearch in general on large clusters.

s1monw · 2017-07-11T21:14:39Z

@jpountz @jimczi I will need to do some work on the unittest end but I wanted to get it out here asap for first rounds and opinions. I also would like to have @clintongormley to look into naming of the parameter I am not a huge fan of it.

epixa · 2017-07-11T21:17:42Z

/cc @spalger

jimczi

I left some minor comments but I love it.
This is a nice solution for time based search but not only so a huge +1.

jimczi · 2017-07-12T07:42:34Z

core/src/main/java/org/elasticsearch/action/search/CanMatchPreFilterSearchPhase.java

+        } else if (results.numMatches == 0) {
+            // this is a special case where we have no hit but we need to get at least one search response in order
+            // to produce a valid search result with all the aggs etc. at least that is what I think is the case... and clint does so
+            // too :D


I agree too ;)
It's extra work since for instance global ords or fielddata could be loaded by this single search but we can optimize this later. It's already a huge win since this will avoid the loading on the other shards !

jimczi · 2017-07-12T07:43:38Z

core/src/main/java/org/elasticsearch/action/search/SearchRequest.java

@@ -58,6 +58,8 @@

    private static final ToXContent.Params FORMAT_PARAMS = new ToXContent.MapParams(Collections.singletonMap("pretty", "false"));

+    public static final int DEFAULT_PRE_FILTER_SHARDS_AFTER = 1;
+


What is the default ? 128 like below or 1 ?

haha yeah true I wanted to trigger this constantly so I changed this but didn't revert

jimczi · 2017-07-12T07:50:26Z

core/src/main/java/org/elasticsearch/search/SearchService.java

+            if (source != null) {
+                QueryBuilder queryBuilder = source.query();
+                AggregatorFactories.Builder aggregations = source.aggregations();
+                boolean hasGlobalAggs = aggregations != null && aggregations.hasGlobalAggregationBuilder();


This could be check on the coordinating node instead to save the round trip since if there is a global agg all shards must match ?

++ will do that

jimczi · 2017-07-12T08:02:20Z

rest-api-spec/src/main/resources/rest-api-spec/test/search/140_pre_filter_search_shards.yml

+        pre_filter_shards_after: 1
+        body: { "size" : 0, "query" : { "range" : { "created_at" : { "gte" : "2016-02-01", "lt": "2018-02-01"} } } }
+
+  - match: { _shards.total: 2 }


I understand why it's important for testing this feature but shouldn't we return the total number of shards pre-filtering ? I think it should be transparent and not modify the total here, otherwise it becomes hard to understand why some shards are in and some are not ?

Agreed I would like it better if it was transparent.

I can try but it might complicate things to be honest...

jakommo · 2017-07-12T08:06:41Z

/cc @n0othing @astefan @gingerwizard @inqueue as we talked about this a few days ago.

jpountz

I left some thoughts.

jpountz · 2017-07-12T08:01:40Z

core/src/main/java/org/elasticsearch/action/search/SearchRequest.java

@@ -58,6 +58,8 @@

    private static final ToXContent.Params FORMAT_PARAMS = new ToXContent.MapParams(Collections.singletonMap("pretty", "false"));

+    public static final int DEFAULT_PRE_FILTER_SHARDS_AFTER = 1;


docs claim the default is 128?

haha yeah true I wanted to trigger this constantly so I changed this but didn't revert

jpountz · 2017-07-12T08:03:44Z

core/src/main/java/org/elasticsearch/search/SearchService.java

+            if (source != null) {
+                QueryBuilder queryBuilder = source.query();
+                AggregatorFactories.Builder aggregations = source.aggregations();
+                boolean hasGlobalAggs = aggregations != null && aggregations.hasGlobalAggregationBuilder();


uh oh oh I would have forgotten about this guy. I guess testing found it?

I believe there is a similar case with minDocCount=0 on terms aggs which exposes all terms contained in the terms dict of doc values.

I believe there is a similar case with minDocCount=0 on terms aggs which exposes all terms contained in the terms dict of doc values.

can you elaborate on this. I am not sure how i can check that

Good catch. This means that we need to check all root aggregations and make sure that none of them can return buckets when the query is MatchNone.
I think we could/should make the aggregation rewriting aware of the query rewriting.
Currently we rewrite aggregations on the shards but they are not supposed to check the query. Instead we could just pass the rewritten query when we rewrite aggs and if the query cannot match document the agg could be rewritten in an MatchNoneAggregationBuilder. Then we could have special cases for aggs like a root terms aggregation with minDocCount set to 0 and canMatch could check after the aggs rewriting that all root aggregations are MatchNoneAggregationBuilder ?

As a first step, I'd just do instanceof checks for TermsAggregationBuilder and (Date)HistogramAggregationBuilder, and check the value of minDocCount.

colings86 · 2017-07-12T08:42:00Z

core/src/main/java/org/elasticsearch/action/search/FetchSearchPhase.java

@@ -105,7 +105,8 @@ private void innerRun() throws IOException {
            -> moveToNextPhase(searchPhaseController, scrollId, reducedQueryPhase, queryAndFetchOptimization ?
            queryResults : fetchResults);
        if (queryAndFetchOptimization) {
-            assert phaseResults.isEmpty() || phaseResults.get(0).fetchResult() != null;
+            assert phaseResults.isEmpty() || phaseResults.get(0).fetchResult() != null : "phaseResults emtpy [" + phaseResults.isEmpty()


nit: emtpy --> empty

colings86 · 2017-07-12T08:47:33Z

core/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java

+        List<SearchRequest> requests = multiRequest.requests();
+        preFilterShardsAfter = Math.max(1, preFilterShardsAfter / (requests.size()+1));
+        for (SearchRequest request : requests) {
+            request.setPreFilterSearchShardsAfter(preFilterShardsAfter);


Should we check if preFilterShardsAfter has been set explicitly on the search request and set it to the min of preFilterShardsAfter and request.getPreFilterSearchShardsAfter()? Not sure if this would actually matter in practice?

good catch I will do that

s1monw · 2017-07-12T09:20:39Z

I pushed new commit addressing all commetns except of the min_doc_count @jpountz @jimczi

colings86 · 2017-07-12T09:54:13Z

core/src/main/java/org/elasticsearch/search/SearchService.java

+                AggregatorFactories.Builder aggregations = source.aggregations();
+                boolean hasGlobalAggs = aggregations != null && aggregations.hasGlobalAggregationBuilder();
+                if (queryBuilder != null && hasGlobalAggs == false) { // we need to executed hasGlobalAggs is equivalent to match all
+                    return queryBuilder instanceof MatchNoneQueryBuilder == false;


As far as I can see the only time this will be hit is if the query is a simple range query which does not overlap with the data on the shard as we only check the root query type. This means that if you have a boolean query with a must/filter range clause and other clauses this won't be rewritten to a match none query and therefore will still cause the search request to hit that shard. To me this seems like a fairly common case for search. Maybe we should change the rewrite of the BoolQueryBuilder to rewrite to a match none query if any of the must/filter clauses are match_none to catch these cases too? (I can add this in a separate PR after this is merged)

this is irrelevent as I hadn't seen #25650

clintongormley · 2017-07-12T10:19:02Z

I also would like to have @clintongormley to look into naming of the parameter I am not a huge fan of it.

I don't have much in the way of suggestions, but we have batched_reduce_size, so perhaps prefilter_shards_size?

jimczi

Thanks for keeping the "total" _shards.total.
For the min_doc_count issue I agree with Adrien, just checking the root aggregations builders should be enough.

s1monw · 2017-07-12T13:36:29Z

@jpountz @jimczi it's ready for another round

* master: (181 commits) Use a non default port range in MockTransportService Add a shard filter search phase to pre-filter shards based on query rewriting (elastic#25658) Prevent excessive disk consumption by log files Migrate RestHttpResponseHeadersIT to ESRestTestCase (elastic#25675) Use config directory to find jvm.options Fix inadvertent rename of systemd tests Adding basic search request documentation for high level client (elastic#25651) Disallow lang to be used with Stored Scripts (elastic#25610) Fix typo in ScriptDocValues deprecation warnings (elastic#25672) Changes DocValueFieldsFetchSubPhase to reuse doc values iterators for multiple hits (elastic#25644) Query range fields by doc values when they are expected to be more efficient than points. Remove SearchHit#internalHits (elastic#25653) [DOCS] Reorganized the highlighting topic so it's less confusing. Add an underscore to flood stage setting Avoid failing install if system-sysctl is masked Add another parent value option to join documentation (elastic#25609) Ensure we rewrite common queries to `match_none` if possible (elastic#25650) Remove reference to field-stats docs. Optimize the order of bytes in uuids for better compression. (elastic#24615) Fix BytesReferenceStreamInput#skip with offset (elastic#25634) ...

Relates to #25658 Closes #25698

…tion in mixed version 6.0 applies some optimization to query rewriting if the number of shards is large. In oder to make use of this optimization this commit adds the internal endpoint to 5.6 such that a 6.0 coordinator node can make use of the feature even in a mixed cluster or via cross cluster search. Relates to elastic#25658

…ewriting (#25658) Today if we search across a large amount of shards we hit every shard. Yet, it's quite common to search across an index pattern for time based indices but filtering will exclude all results outside a certain time range ie. `now-3d`. While the search can potentially hit hundreds of shards the majority of the shards might yield 0 results since there is not document that is within this date range. Kibana for instance does this regularly but used `_field_stats` to optimize the indexes they need to query. Now with the deprecation of `_field_stats` and it's upcoming removal a single dashboard in kibana can potentially turn into searches hitting hundreds or thousands of shards and that can easily cause search rejections even though the most of the requests are very likely super cheap and only need a query rewriting to early terminate with 0 results. This change adds a pre-filter phase for searches that can, if the number of shards are higher than a the `pre_filter_shard_size` threshold (defaults to 128 shards), fan out to the shards and check if the query can potentially match any documents at all. While false positives are possible, a negative response means that no matches are possible. These requests are not subject to rejection and can greatly reduce the number of shards a request needs to hit. The approach here is preferable to the kibana approach with field stats since it correctly handles aliases and uses the correct threadpools to execute these requests. Further it's completely transparent to the user and improves scalability of elasticsearch in general on large clusters.

Even if the query part can rewrite to match none we can't skip the suggest execution since it might yield results. Relates to elastic#25658

Even if the query part can rewrite to match none we can't skip the suggest execution since it might yield results. Relates to #25658

jpountz · 2017-07-18T15:55:06Z

core/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java

@@ -282,10 +286,22 @@ public void writeTo(StreamOutput out) throws IOException {
            }
        }

-        public Builder addAggregators(AggregatorFactories factories) {
-            throw new UnsupportedOperationException("This needs to be removed");
+        public boolean mustVisiteAllDocs() {


therealnb · 2018-11-12T13:52:59Z

This is a great idea, but it looks like failIfOverShardCountLimit is called before the shards are skipped. Is there any reason it has to be like this?

Obviously, if I query index-* with a small time range query the pre filter would bring the shard count to < 1000, but it will still fail failIfOverShardCountLimit. So, naively, it looks like it would be better to do failIfOverShardCountLimit after.

therealnb · 2018-11-13T21:08:07Z

Ok, I spoke too soon. Looks like the failIfOverShardCountLimit defaults to disabled in the latest branch. This all makes sense now.

By default Elasticsearch doesn't reject any search requests based on the number
of shards the request hits. While Elasticsearch will optimize the search
execution on the coordinating node a large number of shards can have a
significant impact CPU and memory wise...

egalpin · 2020-11-26T19:20:12Z

@s1monw Would you be able to expand on the origin of the default shard threshold of 128? Is there a reasonable rule of thumb in terms of the amount of overhead per shard incurred by pre-filtering? Thanks!

s1monw added :Search/Search Search-related issues that do not fall into other categories >enhancement review v6.0.0 labels Jul 11, 2017

s1monw requested review from jpountz and jimczi July 11, 2017 21:13

epixa mentioned this pull request Jul 11, 2017

Remove "expand index pattern when searching" setting for index patterns elastic/kibana#12736

Closed

s1monw added 4 commits July 11, 2017 23:59

handle null query

f784b8e

trigger more interesting random thresholds

f05ecfb

add checks for global aggs and if source is empty

3602ae8

fix prefilter related test failures

9ade2f3

jimczi reviewed Jul 12, 2017

View reviewed changes

jpountz reviewed Jul 12, 2017

View reviewed changes

colings86 reviewed Jul 12, 2017

View reviewed changes

apply feedback

cd68719

colings86 reviewed Jul 12, 2017

View reviewed changes

jimczi approved these changes Jul 12, 2017

View reviewed changes

s1monw added 2 commits July 12, 2017 15:26

fix tests

85be0b0

change parameter

a97639c

s1monw added 3 commits July 12, 2017 15:57

fix compilation and add test for SearchService

807e7e2

add unittest for filter search phase

ae0b64a

add unittest for skipping shards in the intitial search phase

2dcd3e0

pickypg added the das awesome label Jul 12, 2017

s1monw merged commit e81804c into elastic:master Jul 12, 2017

s1monw added a commit that referenced this pull request Jul 13, 2017

Register correct response for can_match proxy response

02e9ad6

Relates to #25658 Closes #25698

s1monw mentioned this pull request Jul 13, 2017

Removes FieldStats API #25628

Merged

s1monw mentioned this pull request Jul 13, 2017

Backport can_match endpoint to 5.6 to allow 6.0 to use the optimization in mixed version #25704

Merged

epixa mentioned this pull request Jul 13, 2017

Remove _field_stats endpoint #25577

Closed

s1monw added a commit that referenced this pull request Jul 15, 2017

Bump BWC versions after #25658 backport to 5.6

ccda044

s1monw added the v5.6.0 label Jul 15, 2017

s1monw mentioned this pull request Jul 15, 2017

Prevent skipping shards if a suggest builder is present #25739

Merged

s1monw added a commit that referenced this pull request Jul 16, 2017

Prevent skipping shards if a suggest builder is present (#25739)

8364279

Even if the query part can rewrite to match none we can't skip the suggest execution since it might yield results. Relates to #25658

s1monw added a commit that referenced this pull request Jul 16, 2017

Prevent skipping shards if a suggest builder is present (#25739)

f5a0c22

Even if the query part can rewrite to match none we can't skip the suggest execution since it might yield results. Relates to #25658

jpountz reviewed Jul 18, 2017

View reviewed changes

chrisronline mentioned this pull request Jul 19, 2017

Kibana 5.5.0 breaks visualizations where field type conflict exists. elastic/kibana#12728

Closed

jpountz mentioned this pull request Jul 21, 2017

First step towards incremental reduction of query responses #23253

Merged

chrisronline mentioned this pull request Jul 24, 2017

Ensure conflicted fields can be searchable and/or aggregatable elastic/kibana#13070

Merged

clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

dougburks mentioned this pull request Oct 6, 2017

Elastic Stack Beta Release Security-Onion-Solutions/security-onion#1130

Closed

59 tasks

dougburks mentioned this pull request Oct 27, 2017

Elastic Stack Beta 2 Security-Onion-Solutions/security-onion#1132

Closed

33 tasks

Bargs mentioned this pull request Nov 1, 2017

Kibana querying indexes outside of supplied time range elastic/kibana#14633

Closed

clintongormley mentioned this pull request Apr 18, 2018

Timelion search ignores time range when choosing indices elastic/kibana#10475

Closed

easyice mentioned this pull request Apr 30, 2020

Remove pre-filter phase in search #56016

Closed

astefan mentioned this pull request Feb 8, 2021

QL: retry SQL and EQL requests in a mixed-node (rolling upgrade) cluster #68602

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a shard filter search phase to pre-filter shards based on query rewriting #25658

Add a shard filter search phase to pre-filter shards based on query rewriting #25658

s1monw commented Jul 11, 2017 •

edited

Loading

s1monw commented Jul 11, 2017

epixa commented Jul 11, 2017

jimczi left a comment

jimczi Jul 12, 2017 •

edited

Loading

jimczi Jul 12, 2017

s1monw Jul 12, 2017

jimczi Jul 12, 2017

s1monw Jul 12, 2017

jimczi Jul 12, 2017

jpountz Jul 12, 2017

s1monw Jul 12, 2017

jakommo commented Jul 12, 2017

jpountz left a comment

jpountz Jul 12, 2017

s1monw Jul 12, 2017

jpountz Jul 12, 2017

jpountz Jul 12, 2017

s1monw Jul 12, 2017

jimczi Jul 12, 2017

jpountz Jul 12, 2017

colings86 Jul 12, 2017

colings86 Jul 12, 2017

s1monw Jul 12, 2017

s1monw commented Jul 12, 2017

colings86 Jul 12, 2017

colings86 Jul 12, 2017

clintongormley commented Jul 12, 2017

jimczi left a comment

s1monw commented Jul 12, 2017

jpountz Jul 18, 2017

therealnb commented Nov 12, 2018

therealnb commented Nov 13, 2018

egalpin commented Nov 26, 2020

		@@ -58,6 +58,8 @@

		private static final ToXContent.Params FORMAT_PARAMS = new ToXContent.MapParams(Collections.singletonMap("pretty", "false"));

		public static final int DEFAULT_PRE_FILTER_SHARDS_AFTER = 1;

Add a shard filter search phase to pre-filter shards based on query rewriting #25658

Add a shard filter search phase to pre-filter shards based on query rewriting #25658

Conversation

s1monw commented Jul 11, 2017 • edited Loading

s1monw commented Jul 11, 2017

epixa commented Jul 11, 2017

jimczi left a comment

Choose a reason for hiding this comment

jimczi Jul 12, 2017 • edited Loading

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

jakommo commented Jul 12, 2017

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

s1monw commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintongormley commented Jul 12, 2017

jimczi left a comment

Choose a reason for hiding this comment

s1monw commented Jul 12, 2017

Choose a reason for hiding this comment

therealnb commented Nov 12, 2018

therealnb commented Nov 13, 2018

egalpin commented Nov 26, 2020

s1monw commented Jul 11, 2017 •

edited

Loading

jimczi Jul 12, 2017 •

edited

Loading