Skip to content

Commit d256649

Browse files
nik9000nik9000
committed
Add setting to disable aggs optimization (backport of elastic#73620)
Sometimes our fancy "run this agg as a Query" optimizations end up slower than running the aggregation in the old way. We know that and use heuristics to dissable the optimization in that case. But it turns out that the process of running the heuristics itself can be slow, depending on the query. Worse, changing the heuristics requires an upgrade, which means waiting. If the heurisics make a terrible choice folks need a quick way out. This adds such a way: a cluster level setting that contains a list of queries that are considered "too expensive" to try and optimize. If the top level query contains any of those queries we'll disable the "run as Query" optimization. The default for this settings is wildcard and term-in-set queries, which is fairly conservative. There are certainly wildcard and term-in-set queries that the optimization works well with, but there are other queries of that type that it works very badly with. So we're being careful. Better, you can modify this setting in a running cluster to disable the optimization if we find a new type of query that doesn't work well. Closes elastic#73426
1 parent 2b4d7c3 commit d256649

File tree

14 files changed

+208
-11
lines changed

14 files changed

+208
-11
lines changed

‎benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/AggConstructionContentionBenchmark.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,11 @@ public Version indexVersionCreated() {
346346
return Version.CURRENT;
347347
}
348348

349+
@Override
350+
public boolean enableRewriteToFilterByFilter() {
351+
return true;
352+
}
353+
349354
@Override
350355
public void close() {
351356
List<Releasable> releaseMe = new ArrayList<>(this.releaseMe);

‎rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/22_terms_disable_opt.yml

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
setup:
2+
- do:
3+
cluster.put_settings:
4+
body:
5+
persistent:
6+
search.aggs.rewrite_to_filter_by_filter: false
7+
8+
---
9+
teardown:
10+
- do:
11+
cluster.put_settings:
12+
body:
13+
persistent:
14+
search.aggs.rewrite_to_filter_by_filter: null
15+
16+
---
17+
does not use optimization:
18+
- skip:
19+
version: " - 7.13.99"
20+
reason: setting to disable optimization added in 7.14.0 to be backported to 7.13.2
21+
- do:
22+
bulk:
23+
index: test
24+
refresh: true
25+
body: |
26+
{ "index": {} }
27+
{ "str": "sheep" }
28+
{ "index": {} }
29+
{ "str": "sheep" }
30+
{ "index": {} }
31+
{ "str": "cow" }
32+
{ "index": {} }
33+
{ "str": "pig" }
34+
35+
- do:
36+
search:
37+
index: test
38+
body:
39+
profile: true
40+
size: 0
41+
aggs:
42+
str_terms:
43+
terms:
44+
field: str.keyword
45+
- match: { aggregations.str_terms.buckets.0.key: sheep }
46+
- match: { aggregations.str_terms.buckets.1.key: cow }
47+
- match: { aggregations.str_terms.buckets.2.key: pig }
48+
- match: { profile.shards.0.aggregations.0.type: GlobalOrdinalsStringTermsAggregator }
49+
- match: { profile.shards.0.aggregations.0.description: str_terms }

‎server/src/internalClusterTest/java/org/elasticsearch/search/profile/aggregation/AggregationProfilerIT.java

Lines changed: 89 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010

1111
import org.elasticsearch.action.index.IndexRequestBuilder;
1212
import org.elasticsearch.action.search.SearchResponse;
13+
import org.elasticsearch.common.settings.Settings;
14+
import org.elasticsearch.search.SearchService;
1315
import org.elasticsearch.search.aggregations.Aggregator.SubAggCollectionMode;
1416
import org.elasticsearch.search.aggregations.BucketOrder;
1517
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder;
@@ -29,6 +31,8 @@
2931
import java.util.Set;
3032
import java.util.stream.Collectors;
3133

34+
import static io.github.nik9000.mapmatcher.MapMatcher.assertMap;
35+
import static io.github.nik9000.mapmatcher.MapMatcher.matchesMap;
3236
import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
3337
import static org.elasticsearch.search.aggregations.AggregationBuilders.avg;
3438
import static org.elasticsearch.search.aggregations.AggregationBuilders.diversifiedSampler;
@@ -601,7 +605,7 @@ public void testFilterByFilter() throws InterruptedException, IOException {
601605
assertSearchResponse(response);
602606
Map<String, ProfileShardResult> profileResults = response.getProfileResults();
603607
assertThat(profileResults, notNullValue());
604-
assertThat(profileResults.size(), equalTo(getNumShards("idx").numPrimaries));
608+
assertThat(profileResults.size(), equalTo(getNumShards("dateidx").numPrimaries));
605609
for (ProfileShardResult profileShardResult : profileResults.values()) {
606610
assertThat(profileShardResult, notNullValue());
607611
AggregationProfileShardResult aggProfileResults = profileShardResult.getAggregationProfileResults();
@@ -651,4 +655,88 @@ public void testFilterByFilter() throws InterruptedException, IOException {
651655
assertThat(queryDebug, hasEntry("query", "ConstantScore(DocValuesFieldExistsQuery [field=date])"));
652656
}
653657
}
658+
659+
public void testDateHistogramFilterByFilterDisabled() throws InterruptedException, IOException {
660+
assertAcked(
661+
client().admin()
662+
.cluster()
663+
.prepareUpdateSettings()
664+
.setPersistentSettings(Settings.builder().put(SearchService.ENABLE_REWRITE_AGGS_TO_FILTER_BY_FILTER.getKey(), false))
665+
);
666+
try {
667+
assertAcked(
668+
client().admin()
669+
.indices()
670+
.prepareCreate("date_filter_by_filter_disabled")
671+
.setSettings(org.elasticsearch.common.collect.Map.of("number_of_shards", 1, "number_of_replicas", 0))
672+
.addMapping("_doc", "date", "type=date", "keyword", "type=keyword")
673+
.get()
674+
);
675+
List<IndexRequestBuilder> builders = new ArrayList<>();
676+
for (int i = 0; i < RangeAggregator.DOCS_PER_RANGE_TO_USE_FILTERS * 2; i++) {
677+
String date = Instant.ofEpochSecond(i).toString();
678+
builders.add(
679+
client().prepareIndex("date_filter_by_filter_disabled", "_doc")
680+
.setSource(
681+
jsonBuilder().startObject()
682+
.field("date", date)
683+
.endObject()
684+
)
685+
);
686+
}
687+
indexRandom(true, false, builders);
688+
689+
SearchResponse response = client().prepareSearch("date_filter_by_filter_disabled")
690+
.setProfile(true)
691+
.addAggregation(new DateHistogramAggregationBuilder("histo").field("date").calendarInterval(DateHistogramInterval.MONTH))
692+
.get();
693+
assertSearchResponse(response);
694+
Map<String, ProfileShardResult> profileResults = response.getProfileResults();
695+
assertThat(profileResults, notNullValue());
696+
assertThat(profileResults.size(), equalTo(getNumShards("date_filter_by_filter_disabled").numPrimaries));
697+
for (ProfileShardResult profileShardResult : profileResults.values()) {
698+
assertThat(profileShardResult, notNullValue());
699+
AggregationProfileShardResult aggProfileResults = profileShardResult.getAggregationProfileResults();
700+
assertThat(aggProfileResults, notNullValue());
701+
List<ProfileResult> aggProfileResultsList = aggProfileResults.getProfileResults();
702+
assertThat(aggProfileResultsList, notNullValue());
703+
assertThat(aggProfileResultsList.size(), equalTo(1));
704+
ProfileResult histoAggResult = aggProfileResultsList.get(0);
705+
assertThat(histoAggResult, notNullValue());
706+
assertThat(histoAggResult.getQueryName(), equalTo("DateHistogramAggregator.FromDateRange"));
707+
assertThat(histoAggResult.getLuceneDescription(), equalTo("histo"));
708+
assertThat(histoAggResult.getProfiledChildren().size(), equalTo(0));
709+
assertThat(histoAggResult.getTime(), greaterThan(0L));
710+
Map<String, Long> breakdown = histoAggResult.getTimeBreakdown();
711+
assertMap(
712+
breakdown,
713+
matchesMap().entry(INITIALIZE, greaterThan(0L))
714+
.entry(INITIALIZE + "_count", greaterThan(0L))
715+
.entry(BUILD_LEAF_COLLECTOR, greaterThan(0L))
716+
.entry(BUILD_LEAF_COLLECTOR + "_count", greaterThan(0L))
717+
.entry(COLLECT, greaterThan(0L))
718+
.entry(COLLECT + "_count", greaterThan(0L))
719+
.entry(POST_COLLECTION, greaterThan(0L))
720+
.entry(POST_COLLECTION + "_count", 1L)
721+
.entry(BUILD_AGGREGATION, greaterThan(0L))
722+
.entry(BUILD_AGGREGATION + "_count", greaterThan(0L))
723+
.entry(REDUCE, 0L)
724+
.entry(REDUCE + "_count", 0L)
725+
);
726+
Map<String, Object> debug = histoAggResult.getDebugInfo();
727+
assertMap(
728+
debug,
729+
matchesMap().entry("delegate", "RangeAggregator.NoOverlap")
730+
.entry("delegate_debug", matchesMap().entry("ranges", 1).entry("average_docs_per_range", 10000.0))
731+
);
732+
}
733+
} finally {
734+
assertAcked(
735+
client().admin()
736+
.cluster()
737+
.prepareUpdateSettings()
738+
.setPersistentSettings(Settings.builder().putNull(SearchService.ENABLE_REWRITE_AGGS_TO_FILTER_BY_FILTER.getKey()))
739+
);
740+
}
741+
}
654742
}

‎server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,7 @@ public void apply(Settings value, Settings current, Settings previous) {
469469
MultiBucketConsumerService.MAX_BUCKET_SETTING,
470470
SearchService.LOW_LEVEL_CANCELLATION_SETTING,
471471
SearchService.MAX_OPEN_SCROLL_CONTEXT,
472+
SearchService.ENABLE_REWRITE_AGGS_TO_FILTER_BY_FILTER,
472473
Node.WRITE_PORTS_FILE_SETTING,
473474
Node.NODE_NAME_SETTING,
474475
Node.NODE_ATTRIBUTES,

‎server/src/main/java/org/elasticsearch/search/SearchService.java

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,13 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv
164164
public static final Setting<Integer> MAX_OPEN_SCROLL_CONTEXT =
165165
Setting.intSetting("search.max_open_scroll_context", 500, 0, Property.Dynamic, Property.NodeScope);
166166

167+
public static final Setting<Boolean> ENABLE_REWRITE_AGGS_TO_FILTER_BY_FILTER = Setting.boolSetting(
168+
"search.aggs.rewrite_to_filter_by_filter",
169+
true,
170+
Property.Dynamic,
171+
Property.NodeScope
172+
);
173+
167174
public static final int DEFAULT_SIZE = 10;
168175
public static final int DEFAULT_FROM = 0;
169176

@@ -197,6 +204,8 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv
197204

198205
private volatile int maxOpenScrollContext;
199206

207+
private volatile boolean enableRewriteAggsToFilterByFilter;
208+
200209
private final Cancellable keepAliveReaper;
201210

202211
private final AtomicLong idGenerator = new AtomicLong();
@@ -243,6 +252,10 @@ public SearchService(ClusterService clusterService, IndicesService indicesServic
243252

244253
lowLevelCancellation = LOW_LEVEL_CANCELLATION_SETTING.get(settings);
245254
clusterService.getClusterSettings().addSettingsUpdateConsumer(LOW_LEVEL_CANCELLATION_SETTING, this::setLowLevelCancellation);
255+
256+
enableRewriteAggsToFilterByFilter = ENABLE_REWRITE_AGGS_TO_FILTER_BY_FILTER.get(settings);
257+
clusterService.getClusterSettings()
258+
.addSettingsUpdateConsumer(ENABLE_REWRITE_AGGS_TO_FILTER_BY_FILTER, this::setEnableRewriteAggsToFilterByFilter);
246259
}
247260

248261
private void validateKeepAlives(TimeValue defaultKeepAlive, TimeValue maxKeepAlive) {
@@ -279,6 +292,10 @@ private void setLowLevelCancellation(Boolean lowLevelCancellation) {
279292
this.lowLevelCancellation = lowLevelCancellation;
280293
}
281294

295+
private void setEnableRewriteAggsToFilterByFilter(boolean enableRewriteAggsToFilterByFilter) {
296+
this.enableRewriteAggsToFilterByFilter = enableRewriteAggsToFilterByFilter;
297+
}
298+
282299
@Override
283300
public void afterIndexRemoved(Index index, IndexSettings indexSettings, IndexRemovalReason reason) {
284301
// once an index is removed due to deletion or closing, we can just clean up all the pending search context information
@@ -984,7 +1001,8 @@ private void parseSource(DefaultSearchContext context, SearchSourceBuilder sourc
9841001
context.indexShard().shardId().hashCode(),
9851002
context::getRelativeTimeInMillis,
9861003
context::isCancelled,
987-
context::buildFilteredQuery
1004+
context::buildFilteredQuery,
1005+
enableRewriteAggsToFilterByFilter
9881006
);
9891007
context.addReleasable(aggContext);
9901008
try {

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,9 @@ public static FromFilters<?> adaptIntoFiltersOrNull(
353353
if (false == FiltersAggregator.canUseFilterByFilter(parent, null)) {
354354
return null;
355355
}
356+
if (false == context.enableRewriteToFilterByFilter()) {
357+
return null;
358+
}
356359
boolean wholeNumbersOnly = false == ((ValuesSource.Numeric) valuesSourceConfig.getValuesSource()).isFloatingPoint();
357360
List<QueryToFilterAdapter<?>> filters = new ArrayList<>(ranges.length);
358361
for (int i = 0; i < ranges.length; i++) {

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,7 @@ static class LowCardinality extends GlobalOrdinalsStringTermsAggregator {
284284
order,
285285
format,
286286
bucketCountThresholds,
287-
l -> true,
287+
ALWAYS_TRUE,
288288
context,
289289
parent,
290290
remapGlobalOrds,

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,7 @@ Aggregator create(String name,
360360
.getValuesSource();
361361
SortedSetDocValues values = globalOrdsValues(context, ordinalsValuesSource);
362362
long maxOrd = values.getValueCount();
363-
if (maxOrd > 0 && maxOrd <= MAX_ORDS_TO_TRY_FILTERS) {
363+
if (maxOrd > 0 && maxOrd <= MAX_ORDS_TO_TRY_FILTERS && context.enableRewriteToFilterByFilter()) {
364364
StringTermsAggregatorFromFilters adapted = StringTermsAggregatorFromFilters.adaptIntoFiltersOrNull(
365365
name,
366366
factories,

‎server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationContext.java

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,14 @@
2525
import org.elasticsearch.index.mapper.MappedFieldType;
2626
import org.elasticsearch.index.mapper.ObjectMapper;
2727
import org.elasticsearch.index.query.QueryBuilder;
28-
import org.elasticsearch.index.query.SearchExecutionContext;
2928
import org.elasticsearch.index.query.Rewriteable;
29+
import org.elasticsearch.index.query.SearchExecutionContext;
3030
import org.elasticsearch.index.query.support.NestedScope;
3131
import org.elasticsearch.script.Script;
3232
import org.elasticsearch.script.ScriptContext;
3333
import org.elasticsearch.search.aggregations.Aggregator;
3434
import org.elasticsearch.search.aggregations.MultiBucketConsumerService.MultiBucketConsumer;
35+
import org.elasticsearch.search.aggregations.bucket.filter.FiltersAggregator.FilterByFilter;
3536
import org.elasticsearch.search.internal.SubSearchContext;
3637
import org.elasticsearch.search.lookup.SearchLookup;
3738
import org.elasticsearch.search.profile.aggregation.AggregationProfiler;
@@ -247,6 +248,16 @@ public final AggregationUsageService getUsageService() {
247248
*/
248249
public abstract boolean isCacheable();
249250

251+
/**
252+
* Are aggregations allowed to try to rewrite themselves into
253+
* {@link FilterByFilter} aggregations? <strong>Often</strong>
254+
* {@linkplain FilterByFilter} is faster to execute, but it isn't
255+
* always. For now this just hooks into a cluster level setting
256+
* so users can disable the behavior when the existing heuristics
257+
* don't detect cases where its slower.
258+
*/
259+
public abstract boolean enableRewriteToFilterByFilter();
260+
250261
/**
251262
* Implementation of {@linkplain AggregationContext} for production usage
252263
* that wraps our ubiquitous {@link SearchExecutionContext} and anything else
@@ -267,6 +278,7 @@ public static class ProductionAggregationContext extends AggregationContext {
267278
private final LongSupplier relativeTimeInMillis;
268279
private final Supplier<Boolean> isCancelled;
269280
private final Function<Query, Query> filterQuery;
281+
private final boolean enableRewriteToFilterByFilter;
270282

271283
private final List<Aggregator> releaseMe = new ArrayList<>();
272284

@@ -282,7 +294,8 @@ public ProductionAggregationContext(
282294
int randomSeed,
283295
LongSupplier relativeTimeInMillis,
284296
Supplier<Boolean> isCancelled,
285-
Function<Query, Query> filterQuery
297+
Function<Query, Query> filterQuery,
298+
boolean enableRewriteToFilterByFilter
286299
) {
287300
this.context = context;
288301
if (bytesToPreallocate == 0) {
@@ -313,6 +326,7 @@ public ProductionAggregationContext(
313326
this.relativeTimeInMillis = relativeTimeInMillis;
314327
this.isCancelled = isCancelled;
315328
this.filterQuery = filterQuery;
329+
this.enableRewriteToFilterByFilter = enableRewriteToFilterByFilter;
316330
}
317331

318332
@Override
@@ -474,6 +488,11 @@ public boolean isCacheable() {
474488
return context.isCacheable();
475489
}
476490

491+
@Override
492+
public boolean enableRewriteToFilterByFilter() {
493+
return enableRewriteToFilterByFilter;
494+
}
495+
477496
@Override
478497
public void close() {
479498
/*

‎server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,14 @@
2222
import org.apache.lucene.index.IndexReader;
2323
import org.apache.lucene.index.IndexableField;
2424
import org.apache.lucene.index.RandomIndexWriter;
25+
import org.apache.lucene.index.Term;
26+
import org.apache.lucene.search.BooleanClause.Occur;
27+
import org.apache.lucene.search.BooleanQuery;
2528
import org.apache.lucene.search.DocValuesFieldExistsQuery;
2629
import org.apache.lucene.search.IndexSearcher;
2730
import org.apache.lucene.search.MatchAllDocsQuery;
2831
import org.apache.lucene.search.Query;
29-
import org.apache.lucene.search.TermInSetQuery;
32+
import org.apache.lucene.search.TermQuery;
3033
import org.apache.lucene.search.TotalHits;
3134
import org.apache.lucene.store.Directory;
3235
import org.apache.lucene.util.BytesRef;
@@ -841,7 +844,7 @@ private <T> void termsAggregator(ValueType valueType, MappedFieldType fieldType,
841844
.size(numTerms)
842845
.collectMode(randomFrom(Aggregator.SubAggCollectionMode.values()))
843846
.field("field"));
844-
context = createAggregationContext(indexSearcher, null, fieldType, filterFieldType);
847+
context = createAggregationContext(indexSearcher, new MatchAllDocsQuery(), fieldType, filterFieldType);
845848
aggregator = createAggregator(aggregationBuilder, context);
846849
aggregator.preCollection();
847850
indexSearcher.search(new MatchAllDocsQuery(), aggregator);
@@ -1026,7 +1029,7 @@ public void testUnmappedWithMissing() throws Exception {
10261029
TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("_name")
10271030
.userValueTypeHint(valueTypes[i])
10281031
.field(fieldNames[i]).missing(missingValues[i]);
1029-
AggregationContext context = createAggregationContext(indexSearcher, null, fieldType1);
1032+
AggregationContext context = createAggregationContext(indexSearcher, new MatchAllDocsQuery(), fieldType1);
10301033
Aggregator aggregator = createAggregator(aggregationBuilder, context);
10311034
aggregator.preCollection();
10321035
indexSearcher.search(new MatchAllDocsQuery(), aggregator);
@@ -1776,7 +1779,10 @@ public void testWithFilterAndPreciseSize() throws IOException {
17761779
* would trigger that bug.
17771780
*/
17781781
builder.size(2).order(BucketOrder.key(true));
1779-
Query topLevel = new TermInSetQuery("k", new BytesRef[] {new BytesRef("b"), new BytesRef("c")});
1782+
Query topLevel = new BooleanQuery.Builder()
1783+
.add(new TermQuery(new Term("k", "b")), Occur.SHOULD)
1784+
.add(new TermQuery(new Term("k", "c")), Occur.SHOULD)
1785+
.build();
17801786
testCase(builder, topLevel, buildIndex, (StringTerms terms) -> {
17811787
assertThat(
17821788
terms.getBuckets().stream().map(StringTerms.Bucket::getKey).collect(toList()),

‎test/framework/build.gradle

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ dependencies {
2626
api "commons-codec:commons-codec:${versions.commonscodec}"
2727
api "org.elasticsearch:securemock:${versions.securemock}"
2828
api "org.elasticsearch:mocksocket:${versions.mocksocket}"
29+
api "io.github.nik9000:mapmatcher:0.0.2"
2930

3031
// json schema validation dependencies
3132
api "com.fasterxml.jackson.core:jackson-core:${versions.jackson}"

0 commit comments

Comments
 (0)