Short circuited to MatchNone for non-participating slice #51207
Conversation
In case of numSlices = numShards , use a MatchNone query instead of boolean with a MatchNone - MUST clause
cbuescher
added
the
:Search/Search
|
Pinging @elastic/es-search (:Search/Search) |
|
@elasticmachine test this please |
There was a problem hiding this comment.
thanks @nirmalc, nice enhancement!
I left small comments to be addressed
| Query slicedQuery = sliceBuilder.toFilter(clusterService, request, queryShardContext); | ||
| if ( slicedQuery instanceof MatchNoDocsQuery){ | ||
| return slicedQuery; | ||
| }else { |
There was a problem hiding this comment.
nit: space before else
| @@ -274,7 +275,12 @@ public Query buildFilteredQuery(Query query) { | |||
| } | |||
|
|
|||
| if (sliceBuilder != null) { | |||
| filters.add(sliceBuilder.toFilter(clusterService, request, queryShardContext)); | |||
| Query slicedQuery = sliceBuilder.toFilter(clusterService, request, queryShardContext); | |||
| if ( slicedQuery instanceof MatchNoDocsQuery){ | |||
There was a problem hiding this comment.
extra space after (
| when(queryShardContext.fieldMapper(anyString())).thenReturn(mock(MappedFieldType.class)); | ||
| when(shardSearchRequest.indexRoutings()).thenReturn(new String[0]); | ||
|
|
||
| DefaultSearchContext context4 = new DefaultSearchContext(3L, shardSearchRequest, target, searcher, null, |
There was a problem hiding this comment.
nit: new DefaultSearchContext(3L, => new DefaultSearchContext(4L,
| @@ -178,6 +182,20 @@ public void testPreProcess() throws Exception { | |||
| ParsedQuery parsedQuery = ParsedQuery.parsedMatchAllQuery(); | |||
| context3.sliceBuilder(null).parsedQuery(parsedQuery).preProcess(false); | |||
| assertEquals(context3.query(), context3.buildFilteredQuery(parsedQuery.query())); | |||
|
|
|||
| when(queryShardContext.getIndexSettings()).thenReturn(indexSettings); | |||
| when(indexService.newQueryShardContext(anyInt(),anyObject(),anyObject(),anyString())).thenReturn(queryShardContext); | |||
There was a problem hiding this comment.
Looks like, line 187 in not necessary
There was a problem hiding this comment.
Thanks for review , it is not necessary. I think i added in my first attempt to include use-case with routings in test. I've reformatted code with intelliJ - sorry for formatting issues.
|
@elasticmachine test this please |
|
@elasticmachine run elasticsearch-ci/default-distro |
|
@elasticmachine run elasticsearch-ci/bwc |
|
@elasticmachine update branch |
|
@elasticmachine test this please |
In case of numSlices = numShards, use a MatchNone query instead of boolean with a MatchNone - MUST clause Backport for #51207
|
@nirmalc Thank you for the PR, your change has ben merged, and will be available from 7.7 |
When the query is expensive parallel usage of scroll/slice API overloads cluster, ( In my case a
span_multiquery causes heap / gc issues )Looks like its because same query is processed against all non-matching shards with a
MatchNoDocsand matching-shards withMatchAllDocsfilter. In case of number of shards =< number of slices, returning aMatchNoDocsquery instead of filter seems to avoid that problem.