From 67e9d39932e920036bea2467b91ce41a1f23927e Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Fri, 26 Jun 2020 15:58:44 -0400 Subject: [PATCH] Remove useless aggregation helper (#58571) (#58578) `descendsFromBucketAggregator` was important before we removed `asMultiBucketAggregator` but now that it is gone `collectsFromSingleBucket` is good enough. Relates to #56487 Co-authored-by: Elastic Machine --- .../search/aggregations/Aggregator.java | 14 -------------- .../terms/SignificantTermsAggregatorFactory.java | 10 ++++------ .../bucket/terms/TermsAggregatorFactory.java | 14 +++++++------- 3 files changed, 11 insertions(+), 27 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/Aggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/Aggregator.java index aefba9bac33..061640e73b5 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/Aggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/Aggregator.java @@ -28,7 +28,6 @@ import org.elasticsearch.common.lease.Releasable; import org.elasticsearch.common.xcontent.DeprecationHandler; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.search.aggregations.bucket.BucketsAggregator; import org.elasticsearch.search.aggregations.support.AggregationPath; import org.elasticsearch.search.internal.SearchContext; import org.elasticsearch.search.sort.SortOrder; @@ -64,19 +63,6 @@ public abstract class Aggregator extends BucketCollector implements Releasable { AggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException; } - /** - * Returns whether one of the parents is a {@link BucketsAggregator}. - */ - public static boolean descendsFromBucketAggregator(Aggregator parent) { - while (parent != null) { - if (parent instanceof BucketsAggregator) { - return true; - } - parent = parent.parent(); - } - return false; - } - /** * Return the name of this aggregator. */ diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantTermsAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantTermsAggregatorFactory.java index ba962477c38..0dc9960ef48 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantTermsAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantTermsAggregatorFactory.java @@ -296,15 +296,13 @@ public class SignificantTermsAggregatorFactory extends ValuesSourceAggregatorFac final IncludeExclude.OrdinalsFilter filter = includeExclude == null ? null : includeExclude.convertToOrdinalsFilter(format); boolean remapGlobalOrd = true; - if (Aggregator.descendsFromBucketAggregator(parent) == false && - factories == AggregatorFactories.EMPTY && - includeExclude == null) { - /** + if (collectsFromSingleBucket && factories == AggregatorFactories.EMPTY && includeExclude == null) { + /* * We don't need to remap global ords iff this aggregator: - * - is not a child of a bucket aggregator AND + * - collects from a single bucket AND * - has no include/exclude rules AND * - has no sub-aggregator - **/ + */ remapGlobalOrd = false; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java index b445f32965d..cd970b6a029 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java @@ -351,14 +351,14 @@ public class TermsAggregatorFactory extends ValuesSourceAggregatorFactory { if (factories == AggregatorFactories.EMPTY && includeExclude == null && - Aggregator.descendsFromBucketAggregator(parent) == false && + collectsFromSingleBucket && ordinalsValuesSource.supportsGlobalOrdinalsMapping() && // we use the static COLLECT_SEGMENT_ORDS to allow tests to force specific optimizations (COLLECT_SEGMENT_ORDS!= null ? COLLECT_SEGMENT_ORDS.booleanValue() : ratio <= 0.5 && maxOrd <= 2048)) { - /** + /* * We can use the low cardinality execution mode iff this aggregator: * - has no sub-aggregator AND - * - is not a child of a bucket aggregator AND + * - collects from a single bucket AND * - has a values source that can map from segment to global ordinals * - At least we reduce the number of global ordinals look-ups by half (ration <= 0.5) AND * - the maximum global ordinal is less than 2048 (LOW_CARDINALITY has additional memory usage, @@ -382,16 +382,16 @@ public class TermsAggregatorFactory extends ValuesSourceAggregatorFactory { } else { remapGlobalOrds = true; if (includeExclude == null && - Aggregator.descendsFromBucketAggregator(parent) == false && + collectsFromSingleBucket && (factories == AggregatorFactories.EMPTY || (isAggregationSort(order) == false && subAggCollectMode == SubAggCollectionMode.BREADTH_FIRST))) { - /** + /* * We don't need to remap global ords iff this aggregator: * - has no include/exclude rules AND - * - is not a child of a bucket aggregator AND + * - only collects from a single bucket AND * - has no sub-aggregator or only sub-aggregator that can be deferred * ({@link SubAggCollectionMode#BREADTH_FIRST}). - **/ + */ remapGlobalOrds = false; } }