Review fixes

Author: JonasKunz

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

@@ -14,6 +14,7 @@
 import static org.elasticsearch.exponentialhistogram.ExponentialScaleUtils.computeIndex;
 
 /**
+ * Only intended for use in tests currently.
  * A class for accumulating raw values into an {@link ExponentialHistogram} with a given maximum number of buckets.
  *
  * If the number of values is less than or equal to the bucket capacity, the resulting histogram is guaranteed
@@ -86,9 +87,13 @@ private void mergeValuesToHistogram() {
         valueBuffer.reset();
         int scale = valueBuffer.scale();
 
-        // Buckets must be provided in ascending index-order
-        // for the negative range, smaller bigger correspond to -INF and smaller ones closer to zero
-        // therefore we have to iterate the negative values in reverse order, from the value closest to -INF to the value closest to zero
+        // Buckets must be provided with their indices in ascending order.
+        // For the negative range, higher bucket indices correspond to bucket boundaries closer to -INF
+        // and smaller bucket indices correspond to bucket boundaries closer to zero.
+        // therefore we have to iterate the negative values in the sorted rawValueBuffer reverse order,
+        // from the value closest to -INF to the value closest to zero.
+        // not that i here is the index of the value in the rawValueBuffer array
+        // and is unrelated to the histogram bucket index for the value.
         for (int i = negativeValuesCount - 1; i >= 0; i--) {
             long count = 1;
             long index = computeIndex(rawValueBuffer[i], scale);

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

@@ -88,7 +88,7 @@ private void doMerge(ExponentialHistogram b) {
         CopyableBucketIterator negBucketsB = b.negativeBuckets().iterator();
 
         ZeroBucket zeroBucket = a.zeroBucket().merge(b.zeroBucket());
-        zeroBucket = zeroBucket.collapseOverlappingBuckets(posBucketsA, negBucketsA, posBucketsB, negBucketsB);
+        zeroBucket = zeroBucket.collapseOverlappingBucketsForAll(posBucketsA, negBucketsA, posBucketsB, negBucketsB);
 
         buffer.setZeroBucket(zeroBucket);
 

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

@@ -49,21 +49,17 @@ public static double getQuantile(ExponentialHistogram histo, double quantile) {
 
         ValueAndPreviousValue values = getElementAtRank(histo, upperRank);
 
+        double result;
         if (lowerRank == upperRank) {
-            return values.valueAtRank();
+            result = values.valueAtRank();
         } else {
-            return values.valueAtPreviousRank() * (1 - upperFactor) + values.valueAtRank() * upperFactor;
+            result = values.valueAtPreviousRank() * (1 - upperFactor) + values.valueAtRank() * upperFactor;
         }
+        return removeNegativeZero(result);
     }
 
-    /**
-     * @param valueAtPreviousRank the value at the rank before the desired rank, NaN if not applicable.
-     * @param valueAtRank         the value at the desired rank
-     */
-    private record ValueAndPreviousValue(double valueAtPreviousRank, double valueAtRank) {
-        ValueAndPreviousValue negateAndSwap() {
-            return new ValueAndPreviousValue(-valueAtRank, -valueAtPreviousRank);
-        }
+    private static double removeNegativeZero(double result) {
+        return result == 0.0 ? 0.0 : result;
     }
 
     private static ValueAndPreviousValue getElementAtRank(ExponentialHistogram histo, long rank) {
@@ -144,4 +140,14 @@ private static ValueAndPreviousValue getBucketMidpointForRank(BucketIterator buc
         }
         throw new IllegalStateException("The total number of elements in the buckets is less than the desired rank.");
     }
+
+    /**
+     * @param valueAtPreviousRank the value at the rank before the desired rank, NaN if not applicable.
+     * @param valueAtRank         the value at the desired rank
+     */
+    private record ValueAndPreviousValue(double valueAtPreviousRank, double valueAtRank) {
+        ValueAndPreviousValue negateAndSwap() {
+            return new ValueAndPreviousValue(-valueAtRank, -valueAtPreviousRank);
+        }
+    }
 }

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

@@ -87,8 +87,8 @@ void setZeroBucket(ZeroBucket zeroBucket) {
      * <br>
      * Callers must adhere to the following rules:
      * <ul>
-     *     <li>All buckets from the negative range must be provided before the first one from the positive range.</li>
-     *     <li>For both the negative and positive ranges, buckets must be provided in ascending index order.</li>
+     *     <li>All buckets for the negative values range must be provided before the first one from the positive values range.</li>
+     *     <li>For both the negative and positive ranges, buckets must be provided with their indices in ascending order.</li>
      *     <li>It is not allowed to provide the same bucket more than once.</li>
      *     <li>It is not allowed to add empty buckets ({@code count <= 0}).</li>
      * </ul>
@@ -105,7 +105,7 @@ void setZeroBucket(ZeroBucket zeroBucket) {
     boolean tryAddBucket(long index, long count, boolean isPositive) {
         assert index >= MIN_INDEX && index <= MAX_INDEX : "index must be in range [" + MIN_INDEX + ".." + MAX_INDEX + "]";
         assert isPositive || positiveBuckets.numBuckets == 0 : "Cannot add negative buckets after a positive bucket has been added";
-        assert count > 0 : "Cannot add an empty or negative bucket";
+        assert count > 0 : "Cannot add a bucket with empty or negative count";
         if (isPositive) {
             return positiveBuckets.tryAddBucket(index, count);
         } else {
@@ -144,7 +144,7 @@ private class Buckets implements ExponentialHistogram.Buckets {
         }
 
         /**
-         * @return the array index of the first bucket of this set of buckets within {@link #bucketCounts} and {@link #bucketIndices}.
+         * @return the position of the first bucket of this set of buckets within {@link #bucketCounts} and {@link #bucketIndices}.
          */
         int startSlot() {
             return isPositive ? negativeBuckets.numBuckets : 0;
@@ -158,6 +158,8 @@ final void reset() {
 
         boolean tryAddBucket(long index, long count) {
             int slot = startSlot() + numBuckets;
+            assert numBuckets == 0 || bucketIndices[slot - 1] < index
+                : "Histogram buckets must be added with their indices in ascending order";
             if (slot >= bucketCounts.length) {
                 return false; // no more space
             }

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

@@ -19,6 +19,9 @@
 /**
  * Represents the bucket for values around zero in an exponential histogram.
  * The range of this bucket is {@code [-zeroThreshold, +zeroThreshold]}.
+ * To allow efficient comparison with bucket boundaries, this class internally
+ * represents the zero threshold as a exponential histogram bucket index with a scale,
+ * computed via {@link ExponentialScaleUtils#computeIndex(double, int)}.
  *
  * @param index The index used with the scale to determine the zero threshold.
  * @param scale The scale used with the index to determine the zero threshold.
@@ -63,7 +66,8 @@ public static ZeroBucket minimalWithCount(long count) {
     /**
      * Merges this zero bucket with another one.
      * <ul>
-     *     <li>If the other zero bucket is empty, this instance is returned unchanged.</li>
+     *     <li>If the other zero bucket or both are empty, this instance is returned unchanged.</li>
+     *     <li>If the this zero bucket is empty and the other one is populated, the other instance is returned unchanged.</li>
      *     <li>Otherwise, the zero threshold is increased if necessary (by taking the maximum of the two), and the counts are summed.</li>
      * </ul>
      *
@@ -73,6 +77,8 @@ public static ZeroBucket minimalWithCount(long count) {
     public ZeroBucket merge(ZeroBucket other) {
         if (other.count == 0) {
             return this;
+        } else if (count == 0) {
+            return other;
         } else {
             long totalCount = count + other.count;
             // Both are populated, so we need to use the higher zero-threshold.
@@ -91,7 +97,7 @@ public ZeroBucket merge(ZeroBucket other) {
      * @param bucketIterators The iterators whose buckets may be collapsed.
      * @return A potentially updated {@link ZeroBucket} with the collapsed buckets' counts and an adjusted threshold.
      */
-    public ZeroBucket collapseOverlappingBuckets(BucketIterator... bucketIterators) {
+    public ZeroBucket collapseOverlappingBucketsForAll(BucketIterator... bucketIterators) {
         ZeroBucket current = this;
         ZeroBucket previous;
         do {

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

@@ -27,6 +27,7 @@
 import java.util.stream.IntStream;
 
 import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MAX_SCALE;
+import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MIN_INDEX;
 import static org.elasticsearch.exponentialhistogram.ExponentialScaleUtils.computeIndex;
 import static org.hamcrest.Matchers.closeTo;
 import static org.hamcrest.Matchers.equalTo;
@@ -43,6 +44,15 @@ private static int randomBucketCount() {
         return (int) Math.round(5 + Math.pow(1995, randomDouble()));
     }
 
+    public void testNoNegativeZeroReturned() {
+        FixedCapacityExponentialHistogram histogram = new FixedCapacityExponentialHistogram(2);
+        histogram.resetBuckets(MAX_SCALE);
+        // add a single, negative bucket close to zero
+        histogram.tryAddBucket(MIN_INDEX, 3, false);
+        double median = ExponentialHistogramQuantile.getQuantile(histogram, 0.5);
+        assertThat(median, equalTo(0.0));
+    }
+
     public void testUniformDistribution() {
         testDistributionQuantileAccuracy(new UniformRealDistribution(new Well19937c(randomInt()), 0, 100));
     }