Fix minimum required bucket count for exponential histogram merging (#133711)

Author: JonasKunz

libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramMerger.java

@@ -56,12 +56,21 @@ public class ExponentialHistogramMerger implements Accountable, Releasable {
     /**
      * Creates a new instance with the specified bucket limit.
      *
-     * @param bucketLimit the maximum number of buckets the result histogram is allowed to have
+     * @param bucketLimit the maximum number of buckets the result histogram is allowed to have, must be at least 4
      * @param circuitBreaker the circuit breaker to use to limit memory allocations
      */
     public static ExponentialHistogramMerger create(int bucketLimit, ExponentialHistogramCircuitBreaker circuitBreaker) {
         circuitBreaker.adjustBreaker(BASE_SIZE);
-        return new ExponentialHistogramMerger(bucketLimit, circuitBreaker);
+        boolean success = false;
+        try {
+            ExponentialHistogramMerger result = new ExponentialHistogramMerger(bucketLimit, circuitBreaker);
+            success = true;
+            return result;
+        } finally {
+            if (success == false) {
+                circuitBreaker.adjustBreaker(-BASE_SIZE);
+            }
+        }
     }
 
     private ExponentialHistogramMerger(int bucketLimit, ExponentialHistogramCircuitBreaker circuitBreaker) {
@@ -70,6 +79,10 @@ private ExponentialHistogramMerger(int bucketLimit, ExponentialHistogramCircuitB
 
     // Only intended for testing, using this in production means an unnecessary reduction of precision
     private ExponentialHistogramMerger(int bucketLimit, int maxScale, ExponentialHistogramCircuitBreaker circuitBreaker) {
+        // We need at least four buckets to represent any possible distribution
+        if (bucketLimit < 4) {
+            throw new IllegalArgumentException("The bucket limit must be at least 4");
+        }
         this.bucketLimit = bucketLimit;
         this.maxScale = maxScale;
         this.circuitBreaker = circuitBreaker;

libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramGeneratorTests.java

@@ -33,7 +33,7 @@ public class ExponentialHistogramGeneratorTests extends ExponentialHistogramTest
 
     public void testVeryLargeValue() {
         double value = Double.MAX_VALUE / 10;
-        ExponentialHistogram histo = createAutoReleasedHistogram(1, value);
+        ExponentialHistogram histo = createAutoReleasedHistogram(4, value);
         long index = histo.positiveBuckets().iterator().peekIndex();
         int scale = histo.scale();
 

libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramMergerTests.java

@@ -36,6 +36,7 @@
 import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MIN_INDEX;
 import static org.elasticsearch.exponentialhistogram.ExponentialScaleUtils.adjustScale;
 import static org.hamcrest.Matchers.closeTo;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.greaterThan;
 
@@ -112,7 +113,7 @@ public void testAggregatesCorrectness() {
         try (
             // Merge some empty histograms too to test that code path
             ReleasableExponentialHistogram merged = ExponentialHistogram.merge(
-                2,
+                4,
                 breaker(),
                 ExponentialHistogram.empty(),
                 createAutoReleasedHistogram(10, firstValues),
@@ -153,6 +154,15 @@ public void testUpscalingDoesNotExceedIndexLimits() {
         }
     }
 
+    public void testMinimumBucketCountBounded() {
+        try {
+            ExponentialHistogram.merge(3, breaker(), ExponentialHistogram.empty(), ExponentialHistogram.empty());
+            fail("Expected exception");
+        } catch (IllegalArgumentException e) {
+            assertThat(e.getMessage(), containsString("limit must be at least 4"));
+        }
+    }
+
     /**
      * Verify that the resulting histogram is independent of the order of elements and therefore merges performed.
      */

libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramUtilsTests.java

@@ -31,7 +31,7 @@ public class ExponentialHistogramUtilsTests extends ExponentialHistogramTestCase
     public void testRandomDataSumEstimation() {
         for (int i = 0; i < 100; i++) {
             int valueCount = randomIntBetween(100, 10_000);
-            int bucketCount = randomIntBetween(2, 500);
+            int bucketCount = randomIntBetween(4, 500);
 
             double correctSum = 0;
             double sign = randomBoolean() ? 1 : -1;

libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/QuantileAccuracyTests.java

@@ -159,7 +159,7 @@ public void testEmptyHistogram() {
     }
 
     public void testSingleValueHistogram() {
-        ExponentialHistogram histo = createAutoReleasedHistogram(1, 42.0);
+        ExponentialHistogram histo = createAutoReleasedHistogram(4, 42.0);
         for (double q : QUANTILES_TO_TEST) {
             assertThat(ExponentialHistogramQuantile.getQuantile(histo, q), closeTo(42, 0.0000001));
         }