Skip to content

Commit 5eb93ba

Browse files
authored
Remove useless aggregation helper (#58571)
`descendsFromBucketAggregator` was important before we removed `asMultiBucketAggregator` but now that it is gone `collectsFromSingleBucket` is good enough. Relates to #56487
1 parent dda78ff commit 5eb93ba

File tree

3 files changed

+11
-27
lines changed

3 files changed

+11
-27
lines changed

server/src/main/java/org/elasticsearch/search/aggregations/Aggregator.java

-14
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@
2828
import org.elasticsearch.common.xcontent.DeprecationHandler;
2929
import org.elasticsearch.common.xcontent.XContentBuilder;
3030
import org.elasticsearch.common.xcontent.XContentParser;
31-
import org.elasticsearch.search.aggregations.bucket.BucketsAggregator;
3231
import org.elasticsearch.search.aggregations.support.AggregationPath;
3332
import org.elasticsearch.search.internal.SearchContext;
3433
import org.elasticsearch.search.sort.SortOrder;
@@ -64,19 +63,6 @@ public interface Parser {
6463
AggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException;
6564
}
6665

67-
/**
68-
* Returns whether one of the parents is a {@link BucketsAggregator}.
69-
*/
70-
public static boolean descendsFromBucketAggregator(Aggregator parent) {
71-
while (parent != null) {
72-
if (parent instanceof BucketsAggregator) {
73-
return true;
74-
}
75-
parent = parent.parent();
76-
}
77-
return false;
78-
}
79-
8066
/**
8167
* Return the name of this aggregator.
8268
*/

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

+4-6
Original file line numberDiff line numberDiff line change
@@ -296,15 +296,13 @@ Aggregator create(String name,
296296

297297
final IncludeExclude.OrdinalsFilter filter = includeExclude == null ? null : includeExclude.convertToOrdinalsFilter(format);
298298
boolean remapGlobalOrd = true;
299-
if (Aggregator.descendsFromBucketAggregator(parent) == false &&
300-
factories == AggregatorFactories.EMPTY &&
301-
includeExclude == null) {
302-
/**
299+
if (collectsFromSingleBucket && factories == AggregatorFactories.EMPTY && includeExclude == null) {
300+
/*
303301
* We don't need to remap global ords iff this aggregator:
304-
* - is not a child of a bucket aggregator AND
302+
* - collects from a single bucket AND
305303
* - has no include/exclude rules AND
306304
* - has no sub-aggregator
307-
**/
305+
*/
308306
remapGlobalOrd = false;
309307
}
310308

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

+7-7
Original file line numberDiff line numberDiff line change
@@ -351,14 +351,14 @@ Aggregator create(String name,
351351

352352
if (factories == AggregatorFactories.EMPTY &&
353353
includeExclude == null &&
354-
Aggregator.descendsFromBucketAggregator(parent) == false &&
354+
collectsFromSingleBucket &&
355355
ordinalsValuesSource.supportsGlobalOrdinalsMapping() &&
356356
// we use the static COLLECT_SEGMENT_ORDS to allow tests to force specific optimizations
357357
(COLLECT_SEGMENT_ORDS!= null ? COLLECT_SEGMENT_ORDS.booleanValue() : ratio <= 0.5 && maxOrd <= 2048)) {
358-
/**
358+
/*
359359
* We can use the low cardinality execution mode iff this aggregator:
360360
* - has no sub-aggregator AND
361-
* - is not a child of a bucket aggregator AND
361+
* - collects from a single bucket AND
362362
* - has a values source that can map from segment to global ordinals
363363
* - At least we reduce the number of global ordinals look-ups by half (ration <= 0.5) AND
364364
* - the maximum global ordinal is less than 2048 (LOW_CARDINALITY has additional memory usage,
@@ -382,16 +382,16 @@ Aggregator create(String name,
382382
} else {
383383
remapGlobalOrds = true;
384384
if (includeExclude == null &&
385-
Aggregator.descendsFromBucketAggregator(parent) == false &&
385+
collectsFromSingleBucket &&
386386
(factories == AggregatorFactories.EMPTY ||
387387
(isAggregationSort(order) == false && subAggCollectMode == SubAggCollectionMode.BREADTH_FIRST))) {
388-
/**
388+
/*
389389
* We don't need to remap global ords iff this aggregator:
390390
* - has no include/exclude rules AND
391-
* - is not a child of a bucket aggregator AND
391+
* - only collects from a single bucket AND
392392
* - has no sub-aggregator or only sub-aggregator that can be deferred
393393
* ({@link SubAggCollectionMode#BREADTH_FIRST}).
394-
**/
394+
*/
395395
remapGlobalOrds = false;
396396
}
397397
}

0 commit comments

Comments
 (0)