Remove useless aggregation helper (elastic#58571)

`descendsFromBucketAggregator` was important before we removed `asMultiBucketAggregator` but now that it is gone `collectsFromSingleBucket` is good enough. Relates to elastic#56487
nik9000 · Jun 25, 2020 · 8d767c9 · 8d767c9
1 parent a524800
commit 8d767c9
Show file tree

Hide file tree

Showing 3 changed files with 11 additions and 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
@@ -28,7 +28,6 @@
 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 interface Parser {
         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/...org/elasticsearch/search/aggregations/bucket/terms/SignificantTermsAggregatorFactory.java b/...org/elasticsearch/search/aggregations/bucket/terms/SignificantTermsAggregatorFactory.java
@@ -296,15 +296,13 @@ Aggregator create(String name,
 
                 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/.../main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java b/.../main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java
@@ -351,14 +351,14 @@ Aggregator create(String name,
 
                 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 @@ Aggregator create(String name,
                 } 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;
                     }
                 }