Revert "Only execute one final reduction in InternalAutoDateHistogram (#45359)"

This reverts commit c0ea8a8.

Author: polyfractal

server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogram.java

@@ -500,24 +500,15 @@ public InternalAggregation doReduce(List<InternalAggregation> aggregations, Redu
         BucketReduceResult reducedBucketsResult = reduceBuckets(aggregations, reduceContext);
 
         if (reduceContext.isFinalReduce()) {
-            // Because auto-date-histo can perform multiple reductions while merging buckets, we need to pretend this is
-            // not the final reduction to prevent pipeline aggs from creating their result early.  However we want
-            // to reuse the multiBucketConsumer so that max_buckets breaker is correctly accounted for
-            ReduceContext penultimateReduceContext = new ReduceContext(reduceContext.bigArrays(), reduceContext.scriptService(),
-                reduceContext::consumeBucketsAndMaybeBreak, false);
-
             // adding empty buckets if needed
-            reducedBucketsResult = addEmptyBuckets(reducedBucketsResult, penultimateReduceContext);
+            reducedBucketsResult = addEmptyBuckets(reducedBucketsResult, reduceContext);
 
             // Adding empty buckets may have tipped us over the target so merge the buckets again if needed
             reducedBucketsResult = mergeBucketsIfNeeded(reducedBucketsResult.buckets, reducedBucketsResult.roundingIdx,
-                    reducedBucketsResult.roundingInfo, penultimateReduceContext);
+                    reducedBucketsResult.roundingInfo, reduceContext);
 
             // Now finally see if we need to merge consecutive buckets together to make a coarser interval at the same rounding
-            reducedBucketsResult = maybeMergeConsecutiveBuckets(reducedBucketsResult, penultimateReduceContext);
-
-            // Perform the final reduction which will mostly be a no-op, except for pipeline aggs
-            reducedBucketsResult = performFinalReduce(reducedBucketsResult, penultimateReduceContext);
+            reducedBucketsResult = maybeMergeConsecutiveBuckets(reducedBucketsResult, reduceContext);
         }
 
         BucketInfo bucketInfo = new BucketInfo(this.bucketInfo.roundingInfos, reducedBucketsResult.roundingIdx,
@@ -570,28 +561,6 @@ private BucketReduceResult mergeConsecutiveBuckets(List<Bucket> reducedBuckets,
         return new BucketReduceResult(mergedBuckets, roundingInfo, roundingIdx, mergeInterval);
     }
 
-    /**
-     * Execute a final reduction on `reducedBuckets`.  This should be called after all the buckets have been
-     * merged into the appropriate roundings.  After the buckets are stable, this method will perform one last
-     * reduction with finalReduce: true so that Pipeline aggs can generate their output.
-     */
-    private BucketReduceResult performFinalReduce(BucketReduceResult reducedBuckets, ReduceContext reduceContext) {
-        // We need to create another reduce context, this time setting finalReduce: true. Unlike the prior
-        // reduce context, we _do not_ want to reuse the multiBucketConsumer from the reduce context.
-        // We've already generated (and accounted for) all the buckets we will return, this method just triggers
-        // a final reduction on un-reduced items like pipelines.  If we re-use the multiBucketConsumer we would
-        // over-count the buckets
-        ReduceContext finalReduceContext = new ReduceContext(reduceContext.bigArrays(), reduceContext.scriptService(), true);
-
-        List<Bucket> finalBuckets = new ArrayList<>();
-        for (int i = 0; i < reducedBuckets.buckets.size(); i++) {
-            finalBuckets.add(reducedBuckets.buckets.get(i).reduce(Collections.singletonList(reducedBuckets.buckets.get(i)),
-                reducedBuckets.roundingInfo.rounding, finalReduceContext));
-        }
-        assert reducedBuckets.buckets.size() == finalBuckets.size();
-        return new BucketReduceResult(finalBuckets, reducedBuckets.roundingInfo, reducedBuckets.roundingIdx, reducedBuckets.innerInterval);
-    }
-
     @Override
     public XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException {
         builder.startArray(CommonFields.BUCKETS.getPreferredName());

server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java

@@ -36,15 +36,10 @@
 import org.elasticsearch.common.time.DateFormatter;
 import org.elasticsearch.index.IndexSettings;
 import org.elasticsearch.index.mapper.DateFieldMapper;
-import org.elasticsearch.index.mapper.MappedFieldType;
-import org.elasticsearch.index.mapper.NumberFieldMapper;
 import org.elasticsearch.search.aggregations.AggregationBuilders;
 import org.elasticsearch.search.aggregations.AggregatorTestCase;
 import org.elasticsearch.search.aggregations.MultiBucketConsumerService;
-import org.elasticsearch.search.aggregations.metrics.InternalMax;
 import org.elasticsearch.search.aggregations.metrics.InternalStats;
-import org.elasticsearch.search.aggregations.pipeline.DerivativePipelineAggregationBuilder;
-import org.elasticsearch.search.aggregations.pipeline.InternalSimpleValue;
 import org.elasticsearch.search.aggregations.support.AggregationInspectionHelper;
 import org.hamcrest.Matchers;
 import org.junit.Assert;
@@ -63,12 +58,9 @@
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
 
-import static org.hamcrest.Matchers.equalTo;
-
 public class AutoDateHistogramAggregatorTests extends AggregatorTestCase {
     private static final String DATE_FIELD = "date";
     private static final String INSTANT_FIELD = "instant";
-    private static final String NUMERIC_FIELD = "numeric";
 
     private static final List<ZonedDateTime> DATES_WITH_TIME = Arrays.asList(
         ZonedDateTime.of(2010, 3, 12, 1, 7, 45, 0, ZoneOffset.UTC),
@@ -726,35 +718,6 @@ public void testIntervalSecond() throws IOException {
         );
     }
 
-    public void testWithPipelineReductions() throws IOException {
-        testSearchAndReduceCase(DEFAULT_QUERY, DATES_WITH_TIME,
-            aggregation -> aggregation.setNumBuckets(1).field(DATE_FIELD)
-                .subAggregation(AggregationBuilders.histogram("histo").field(NUMERIC_FIELD).interval(1)
-                    .subAggregation(AggregationBuilders.max("max").field(NUMERIC_FIELD))
-                    .subAggregation(new DerivativePipelineAggregationBuilder("deriv", "max"))),
-            histogram -> {
-                assertTrue(AggregationInspectionHelper.hasValue(histogram));
-                final List<? extends Histogram.Bucket> buckets = histogram.getBuckets();
-                assertEquals(1, buckets.size());
-
-                Histogram.Bucket bucket = buckets.get(0);
-                assertEquals("2010-01-01T00:00:00.000Z", bucket.getKeyAsString());
-                assertEquals(10, bucket.getDocCount());
-                assertThat(bucket.getAggregations().asList().size(), equalTo(1));
-                InternalHistogram histo = (InternalHistogram) bucket.getAggregations().asList().get(0);
-                assertThat(histo.getBuckets().size(), equalTo(10));
-                for (int i = 0; i < 10; i++) {
-                    assertThat(histo.getBuckets().get(i).key, equalTo((double)i));
-                    assertThat(((InternalMax)histo.getBuckets().get(i).aggregations.get("max")).getValue(), equalTo((double)i));
-                    if (i > 0) {
-                        assertThat(((InternalSimpleValue)histo.getBuckets().get(i).aggregations.get("deriv")).getValue(), equalTo(1.0));
-                    }
-                }
-
-
-            });
-    }
-
     private void testSearchCase(final Query query, final List<ZonedDateTime> dataset,
                                 final Consumer<AutoDateHistogramAggregationBuilder> configure,
                                 final Consumer<InternalAutoDateHistogram> verify) throws IOException {
@@ -794,7 +757,6 @@ private void executeTestCase(final boolean reduced, final Query query, final Lis
         try (Directory directory = newDirectory()) {
             try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
                 final Document document = new Document();
-                int i = 0;
                 for (final ZonedDateTime date : dataset) {
                     if (frequently()) {
                         indexWriter.commit();
@@ -803,10 +765,8 @@ private void executeTestCase(final boolean reduced, final Query query, final Lis
                     final long instant = date.toInstant().toEpochMilli();
                     document.add(new SortedNumericDocValuesField(DATE_FIELD, instant));
                     document.add(new LongPoint(INSTANT_FIELD, instant));
-                    document.add(new SortedNumericDocValuesField(NUMERIC_FIELD, i));
                     indexWriter.addDocument(document);
                     document.clear();
-                    i += 1;
                 }
             }
 
@@ -823,19 +783,11 @@ private void executeTestCase(final boolean reduced, final Query query, final Lis
                 fieldType.setHasDocValues(true);
                 fieldType.setName(aggregationBuilder.field());
 
-                MappedFieldType instantFieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG);
-                instantFieldType.setName(INSTANT_FIELD);
-                instantFieldType.setHasDocValues(true);
-
-                MappedFieldType numericFieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG);
-                numericFieldType.setName(NUMERIC_FIELD);
-                numericFieldType.setHasDocValues(true);
-
                 final InternalAutoDateHistogram histogram;
                 if (reduced) {
-                    histogram = searchAndReduce(indexSearcher, query, aggregationBuilder, fieldType, instantFieldType, numericFieldType);
+                    histogram = searchAndReduce(indexSearcher, query, aggregationBuilder, fieldType);
                 } else {
-                    histogram = search(indexSearcher, query, aggregationBuilder, fieldType, instantFieldType);
+                    histogram = search(indexSearcher, query, aggregationBuilder, fieldType);
                 }
                 verify.accept(histogram);
             }

server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java

@@ -50,15 +50,11 @@ public class InternalAutoDateHistogramTests extends InternalMultiBucketAggregati
 
     private DocValueFormat format;
     private RoundingInfo[] roundingInfos;
-    private int nbBuckets;
 
     @Override
     public void setUp() throws Exception {
         super.setUp();
-        // these need to be the same for each new instance created so that {@link #testReduceRandom()}
-        // has mergeable instances to work with
         format = randomNumericDocValueFormat();
-        nbBuckets = randomNumberOfBuckets();
     }
 
     @Override
@@ -68,7 +64,7 @@ protected InternalAutoDateHistogram createTestInstance(String name,
                                                        InternalAggregations aggregations) {
 
         roundingInfos = AutoDateHistogramAggregationBuilder.buildRoundings(null, null);
-
+        int nbBuckets = randomNumberOfBuckets();
         int targetBuckets = randomIntBetween(1, nbBuckets * 2 + 1);
         List<InternalAutoDateHistogram.Bucket> buckets = new ArrayList<>(nbBuckets);