[TEST] Increase InternalHistogramTests coverage

In this test we were randomizing different values but minDocCount was hardcoded to 1. It's important to test other values, especially `0` as it's the default. The test needed some adapting in the way buckets are randomly generated: all aggs need to share the same interval, minDocCount and emptyBucketInfo. Also assertions need to take into account that more (or less) buckets are expected depending on minDocCount.

This was originated by elastic#35921 and its need to test adding empty buckets as part of the reduce phase.
Also relates to elastic#26856 as one more key comparison needed to use `Double.compare` to properly handle `NaN` values, this was triggered by the increased test coverage.

Author: javanna

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

@@ -213,7 +213,7 @@ public int hashCode() {
     private final DocValueFormat format;
     private final boolean keyed;
     private final long minDocCount;
-    private final EmptyBucketInfo emptyBucketInfo;
+    final EmptyBucketInfo emptyBucketInfo;
 
     InternalHistogram(String name, List<Bucket> buckets, BucketOrder order, long minDocCount, EmptyBucketInfo emptyBucketInfo,
             DocValueFormat formatter, boolean keyed, List<PipelineAggregator> pipelineAggregators,
@@ -302,7 +302,7 @@ private List<Bucket> reduceBuckets(List<InternalAggregation> aggregations, Reduc
         final PriorityQueue<IteratorAndCurrent> pq = new PriorityQueue<IteratorAndCurrent>(aggregations.size()) {
             @Override
             protected boolean lessThan(IteratorAndCurrent a, IteratorAndCurrent b) {
-                return a.current.key < b.current.key;
+                return Double.compare(a.current.key, b.current.key) < 0;
             }
         };
         for (InternalAggregation aggregation : aggregations) {
@@ -405,7 +405,7 @@ private void addEmptyBuckets(List<Bucket> list, ReduceContext reduceContext) {
                         iter.add(new Bucket(key, 0, keyed, format, reducedEmptySubAggs));
                         key = nextKey(key);
                     }
-                    assert key == nextBucket.key;
+                    assert key == nextBucket.key || Double.isNaN(nextBucket.key) : "key: " + key + ", nextBucket.key: " + nextBucket.key;
                 }
                 lastBucket = iter.next();
             } while (iter.hasNext());

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

@@ -25,9 +25,9 @@
 import org.elasticsearch.search.aggregations.BucketOrder;
 import org.elasticsearch.search.aggregations.InternalAggregation;
 import org.elasticsearch.search.aggregations.InternalAggregations;
-import org.elasticsearch.test.InternalMultiBucketAggregationTestCase;
 import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation;
 import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
+import org.elasticsearch.test.InternalMultiBucketAggregationTestCase;
 
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -40,29 +40,52 @@ public class InternalHistogramTests extends InternalMultiBucketAggregationTestCa
 
     private boolean keyed;
     private DocValueFormat format;
+    private int interval;
+    private int minDocCount;
+    private InternalHistogram.EmptyBucketInfo emptyBucketInfo;
+    private int offset;
 
     @Override
-    public void setUp() throws Exception{
+    public void setUp() throws Exception {
         super.setUp();
         keyed = randomBoolean();
         format = randomNumericDocValueFormat();
+        interval = randomIntBetween(1, 3);
+        offset = randomIntBetween(0, 3);
+        if (randomBoolean()) {
+            minDocCount = randomIntBetween(1, 10);
+            emptyBucketInfo = null;
+        } else {
+            minDocCount = 0;
+            //it's ok if minBound and maxBound are outside the range of the generated buckets, that will just mean that
+            //empty buckets won't be added before the first bucket and/or after the last one
+            int minBound = randomInt(50) - 30;
+            int maxBound =  randomNumberOfBuckets() * interval + randomIntBetween(0, 10);
+            emptyBucketInfo = new InternalHistogram.EmptyBucketInfo(interval, offset, minBound, maxBound, InternalAggregations.EMPTY);
+        }
+    }
+
+    private double round(double key) {
+        return Math.floor((key - offset) / interval) * interval + offset;
     }
 
     @Override
     protected InternalHistogram createTestInstance(String name,
                                                    List<PipelineAggregator> pipelineAggregators,
                                                    Map<String, Object> metaData,
                                                    InternalAggregations aggregations) {
-        final int base = randomInt(50) - 30;
+        final double base = round(randomInt(50) - 30);
         final int numBuckets = randomNumberOfBuckets();
-        final int interval = randomIntBetween(1, 3);
         List<InternalHistogram.Bucket> buckets = new ArrayList<>();
         for (int i = 0; i < numBuckets; ++i) {
-            final int docCount = TestUtil.nextInt(random(), 1, 50);
-            buckets.add(new InternalHistogram.Bucket(base + i * interval, docCount, keyed, format, aggregations));
+            //rarely leave some holes to be filled up with empty buckets in case minDocCount is set to 0
+            if (frequently()) {
+                final int docCount = TestUtil.nextInt(random(), 1, 50);
+                buckets.add(new InternalHistogram.Bucket(base + i * interval, docCount, keyed, format, aggregations));
+            }
         }
         BucketOrder order = BucketOrder.key(randomBoolean());
-        return new InternalHistogram(name, buckets, order, 1, null, format, keyed, pipelineAggregators, metaData);
+        return new InternalHistogram(name, buckets, order, minDocCount, emptyBucketInfo, format, keyed, pipelineAggregators, metaData);
     }
 
     // issue 26787
@@ -88,13 +111,36 @@ public void testHandlesNaN() {
 
     @Override
     protected void assertReduced(InternalHistogram reduced, List<InternalHistogram> inputs) {
-        Map<Double, Long> expectedCounts = new TreeMap<>();
+        TreeMap<Double, Long> expectedCounts = new TreeMap<>();
         for (Histogram histogram : inputs) {
             for (Histogram.Bucket bucket : histogram.getBuckets()) {
                 expectedCounts.compute((Double) bucket.getKey(),
                         (key, oldValue) -> (oldValue == null ? 0 : oldValue) + bucket.getDocCount());
             }
         }
+        if (minDocCount == 0) {
+            double minBound = round(emptyBucketInfo.minBound);
+            if (expectedCounts.isEmpty() && emptyBucketInfo.minBound < emptyBucketInfo.maxBound) {
+                expectedCounts.put(minBound, 0L);
+            }
+            if (expectedCounts.isEmpty() == false) {
+                Double nextKey = expectedCounts.firstKey();
+                while (nextKey < expectedCounts.lastKey()) {
+                    expectedCounts.putIfAbsent(nextKey, 0L);
+                    nextKey += interval;
+                }
+                while (minBound < expectedCounts.firstKey()) {
+                    expectedCounts.put(expectedCounts.firstKey() - interval, 0L);
+                }
+                double maxBound = round(emptyBucketInfo.maxBound);
+                while (expectedCounts.lastKey() < maxBound) {
+                    expectedCounts.put(expectedCounts.lastKey() + interval, 0L);
+                }
+            }
+        }
+
+        expectedCounts.entrySet().removeIf(doubleLongEntry -> doubleLongEntry.getValue() < minDocCount);
+
         Map<Double, Long> actualCounts = new TreeMap<>();
         for (Histogram.Bucket bucket : reduced.getBuckets()) {
             actualCounts.compute((Double) bucket.getKey(),
@@ -121,6 +167,7 @@ protected InternalHistogram mutateInstance(InternalHistogram instance) {
         long minDocCount = instance.getMinDocCount();
         List<PipelineAggregator> pipelineAggregators = instance.pipelineAggregators();
         Map<String, Object> metaData = instance.getMetaData();
+        InternalHistogram.EmptyBucketInfo emptyBucketInfo = instance.emptyBucketInfo;
         switch (between(0, 4)) {
         case 0:
             name += randomAlphaOfLength(5);
@@ -135,6 +182,7 @@ protected InternalHistogram mutateInstance(InternalHistogram instance) {
             break;
         case 3:
             minDocCount += between(1, 10);
+            emptyBucketInfo = null;
             break;
         case 4:
             if (metaData == null) {
@@ -147,6 +195,6 @@ protected InternalHistogram mutateInstance(InternalHistogram instance) {
         default:
             throw new AssertionError("Illegal randomisation branch");
         }
-        return new InternalHistogram(name, buckets, order, minDocCount, null, format, keyed, pipelineAggregators, metaData);
+        return new InternalHistogram(name, buckets, order, minDocCount, emptyBucketInfo, format, keyed, pipelineAggregators, metaData);
     }
 }