diff --git a/docs/changelog/111758.yaml b/docs/changelog/111758.yaml new file mode 100644 index 0000000000000..c95cdf48bc8a7 --- /dev/null +++ b/docs/changelog/111758.yaml @@ -0,0 +1,6 @@ +pr: 111758 +summary: Revert "Avoid bucket copies in Aggs" +area: Aggregations +type: bug +issues: + - 111679 diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java b/server/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java index 297bb81b27b25..4f234c33b13a6 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java @@ -226,34 +226,11 @@ public static InternalAggregations topLevelReduce(List agg } if (context.isFinalReduce()) { List reducedInternalAggs = reduced.getInternalAggregations(); - List internalAggregations = null; - for (int i = 0; i < reducedInternalAggs.size(); i++) { - InternalAggregation agg = reducedInternalAggs.get(i); - InternalAggregation internalAggregation = agg.reducePipelines( - agg, - context, - context.pipelineTreeRoot().subTree(agg.getName()) - ); - if (internalAggregation.equals(agg) == false) { - if (internalAggregations == null) { - internalAggregations = new ArrayList<>(reducedInternalAggs); - } - internalAggregations.set(i, internalAggregation); - } - } + reducedInternalAggs = reducedInternalAggs.stream() + .map(agg -> agg.reducePipelines(agg, context, context.pipelineTreeRoot().subTree(agg.getName()))) + .collect(Collectors.toCollection(ArrayList::new)); - var pipelineAggregators = context.pipelineTreeRoot().aggregators(); - if (pipelineAggregators.isEmpty()) { - if (internalAggregations == null) { - return reduced; - } - return from(internalAggregations); - } - if (internalAggregations != null) { - reducedInternalAggs = internalAggregations; - } - reducedInternalAggs = new ArrayList<>(reducedInternalAggs); - for (PipelineAggregator pipelineAggregator : pipelineAggregators) { + for (PipelineAggregator pipelineAggregator : context.pipelineTreeRoot().aggregators()) { SiblingPipelineAggregator sib = (SiblingPipelineAggregator) pipelineAggregator; InternalAggregation newAgg = sib.doReduce(from(reducedInternalAggs), context); reducedInternalAggs.add(newAgg); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregation.java b/server/src/main/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregation.java index e046b5fc9244c..de19c26daff92 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregation.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregation.java @@ -207,31 +207,16 @@ public void forEachBucket(Consumer consumer) { } private List reducePipelineBuckets(AggregationReduceContext reduceContext, PipelineTree pipelineTree) { - List reducedBuckets = null; - var buckets = getBuckets(); - for (int bucketIndex = 0; bucketIndex < buckets.size(); bucketIndex++) { - B bucket = buckets.get(bucketIndex); - List aggs = null; - int aggIndex = 0; - for (InternalAggregation agg : bucket.getAggregations()) { + List reducedBuckets = new ArrayList<>(); + for (B bucket : getBuckets()) { + List aggs = new ArrayList<>(); + for (Aggregation agg : bucket.getAggregations()) { PipelineTree subTree = pipelineTree.subTree(agg.getName()); - var reduced = agg.reducePipelines(agg, reduceContext, subTree); - if (reduced.equals(agg) == false) { - if (aggs == null) { - aggs = bucket.getAggregations().copyResults(); - } - aggs.set(aggIndex, reduced); - } - aggIndex++; - } - if (aggs != null) { - if (reducedBuckets == null) { - reducedBuckets = new ArrayList<>(buckets); - } - reducedBuckets.set(bucketIndex, createBucket(InternalAggregations.from(aggs), bucket)); + aggs.add(((InternalAggregation) agg).reducePipelines((InternalAggregation) agg, reduceContext, subTree)); } + reducedBuckets.add(createBucket(InternalAggregations.from(aggs), bucket)); } - return reducedBuckets == null ? buckets : reducedBuckets; + return reducedBuckets; } public abstract static class InternalBucket implements Bucket, Writeable { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AbstractHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AbstractHistogramAggregator.java index 04028de5656ca..62b7a0747ca00 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AbstractHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AbstractHistogramAggregator.java @@ -84,9 +84,6 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I double key = roundKey * interval + offset; return new InternalHistogram.Bucket(key, docCount, keyed, formatter, subAggregationResults); }, (owningBucketOrd, buckets) -> { - if (buckets.isEmpty()) { - return buildEmptyAggregation(); - } // the contract of the histogram aggregation is that shards must return buckets ordered by key in ascending order CollectionUtil.introSort(buckets, BucketOrder.key(true).comparator()); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregator.java index cb01aa5a31a9a..2c57bd4b38a04 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregator.java @@ -340,9 +340,6 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I return buildAggregationsForVariableBuckets(owningBucketOrds, bucketOrds, (bucketValue, docCount, subAggregationResults) -> { return new InternalDateHistogram.Bucket(bucketValue, docCount, keyed, formatter, subAggregationResults); }, (owningBucketOrd, buckets) -> { - if (buckets.isEmpty()) { - return buildEmptyAggregation(); - } // the contract of the histogram aggregation is that shards must return buckets ordered by key in ascending order CollectionUtil.introSort(buckets, BucketOrder.key(true).comparator()); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalHistogram.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalHistogram.java index 2404de76fdd35..e27a09802d6c4 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalHistogram.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalHistogram.java @@ -259,17 +259,11 @@ BucketOrder getOrder() { @Override public InternalHistogram create(List buckets) { - if (this.buckets.equals(buckets)) { - return this; - } return new InternalHistogram(name, buckets, order, minDocCount, emptyBucketInfo, format, keyed, metadata); } @Override public Bucket createBucket(InternalAggregations aggregations, Bucket prototype) { - if (prototype.aggregations.equals(aggregations)) { - return prototype; - } return new Bucket(prototype.key, prototype.docCount, prototype.keyed, prototype.format, aggregations); } @@ -456,9 +450,6 @@ public InternalAggregation get() { CollectionUtil.introSort(reducedBuckets, order.comparator()); } } - if (reducedBuckets.equals(buckets)) { - return InternalHistogram.this; - } return new InternalHistogram(getName(), reducedBuckets, order, minDocCount, emptyBucketInfo, format, keyed, getMetadata()); } }; @@ -504,9 +495,14 @@ public Number getKey(MultiBucketsAggregation.Bucket bucket) { } @Override - @SuppressWarnings({ "rawtypes", "unchecked" }) public InternalAggregation createAggregation(List buckets) { - return new InternalHistogram(name, (List) buckets, order, minDocCount, emptyBucketInfo, format, keyed, getMetadata()); + // convert buckets to the right type + List buckets2 = new ArrayList<>(buckets.size()); + for (Object b : buckets) { + buckets2.add((Bucket) b); + } + buckets2 = Collections.unmodifiableList(buckets2); + return new InternalHistogram(name, buckets2, order, minDocCount, emptyBucketInfo, format, keyed, getMetadata()); } @Override