Felix review fixes

Author: JonasKunz

libs/exponential-histogram/README.md

@@ -2,13 +2,13 @@ This library provides an implementation of merging and analysis algorithms for e
 
 ## Overview
 
-The library implements base-2 exponential histograms with perfect subsetting. The most imporant properties are:
+The library implements base-2 exponential histograms with perfect subsetting. The most important properties are:
 
 * The histogram has a scale parameter, which defines the accuracy. A higher scale implies a higher accuracy.
 * The `base` for the buckets is defined as `base = 2^(2^-scale)`.
 * The histogram bucket at index `i` has the range `(base^i, base^(i+1)]`
 * Negative values are represented by a separate negative range of buckets with the boundaries `(-base^(i+1), -base^i]`
-* Histograms are perfectly subsetting: increasing the scale by one merges each pair of neighboring buckets
+* Histograms support perfect subsetting: when the scale is decreased by one, each pair of adjacent buckets is merged into a single bucket without introducing error
 * A special zero bucket with a zero-threshold is used to handle zero and close-to-zero values
 
 For more details please refer to the [OpenTelemetry definition](https://opentelemetry.io/docs/specs/otel/metrics/data-model/#exponentialhistogram).
@@ -89,7 +89,7 @@ bucketIndiciesToCounts: {
 ```
 
 Downscaling on the sparse representation only happens if either:
- * The number of populated buckets would become bigger than our maximum bucket count. We have to downscale to make neighboring, populated buckets combine to a single bucket until we are below our limit again.
+ * The number of populated buckets would become bigger than our maximum bucket count. We have to downscale to combine neighboring, populated buckets to a single bucket until we are below our limit again.
  * The highest or smallest indices require more bits to store than we allow. This does not happen in our implementation for normal inputs, because we allow up to 62 bits for index storage, which fits the entire numeric range of IEEE 754 double precision floats at our maximum scale.
 
 ### Handling Explicit Bucket Histograms

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

@@ -9,6 +9,8 @@
 
 package org.elasticsearch.exponentialhistogram;
 
+import java.util.OptionalLong;
+
 import static org.elasticsearch.exponentialhistogram.ExponentialScaleUtils.getMaximumScaleIncrease;
 
 /**
@@ -97,14 +99,18 @@ private void doMerge(ExponentialHistogram b) {
         if (targetScale > b.scale()) {
             if (negBucketsB.hasNext()) {
                 long smallestIndex = negBucketsB.peekIndex();
-                long highestIndex = b.negativeBuckets().maxBucketIndex().getAsLong();
-                int maxScaleIncrease = Math.min(getMaximumScaleIncrease(smallestIndex), getMaximumScaleIncrease(highestIndex));
+                OptionalLong maximumIndex = b.negativeBuckets().maxBucketIndex();
+                assert maximumIndex.isPresent()
+                    : "We checked that the negative bucket range is not empty, therefore the maximum index should be present";
+                int maxScaleIncrease = Math.min(getMaximumScaleIncrease(smallestIndex), getMaximumScaleIncrease(maximumIndex.getAsLong()));
                 targetScale = Math.min(targetScale, b.scale() + maxScaleIncrease);
             }
             if (posBucketsB.hasNext()) {
                 long smallestIndex = posBucketsB.peekIndex();
-                long highestIndex = b.positiveBuckets().maxBucketIndex().getAsLong();
-                int maxScaleIncrease = Math.min(getMaximumScaleIncrease(smallestIndex), getMaximumScaleIncrease(highestIndex));
+                OptionalLong maximumIndex = b.positiveBuckets().maxBucketIndex();
+                assert maximumIndex.isPresent()
+                    : "We checked that the positive bucket range is not empty, therefore the maximum index should be present";
+                int maxScaleIncrease = Math.min(getMaximumScaleIncrease(smallestIndex), getMaximumScaleIncrease(maximumIndex.getAsLong()));
                 targetScale = Math.min(targetScale, b.scale() + maxScaleIncrease);
             }
         }

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

@@ -20,27 +20,19 @@
 final class FixedCapacityExponentialHistogram implements ExponentialHistogram {
 
     // These arrays represent both the positive and the negative buckets.
-    // They store all negative buckets first, in ascending index order, followed by all positive buckets, also in ascending index order.
+    // They store all buckets for the negative range first, in ascending index order,
+    // followed by all buckets for the positive range, also in ascending index order.
+    // This means we store the buckets ordered by their boundaries in ascending order (from -INF to +INF).
     private final long[] bucketIndices;
     private final long[] bucketCounts;
 
     private int bucketScale;
 
-    private final AbstractBuckets negativeBuckets = new AbstractBuckets() {
-        @Override
-        int startSlot() {
-            return 0;
-        }
-    };
+    private final Buckets negativeBuckets = new Buckets(false);
 
     private ZeroBucket zeroBucket;
 
-    private final AbstractBuckets positiveBuckets = new AbstractBuckets() {
-        @Override
-        int startSlot() {
-            return negativeBuckets.numBuckets;
-        }
-    };
+    private final Buckets positiveBuckets = new Buckets(true);
 
     /**
      * Creates an empty histogram with the given capacity and a {@link ZeroBucket#minimalEmpty()} zero bucket.
@@ -57,7 +49,7 @@ int startSlot() {
     /**
      * Resets this histogram to the same state as a newly constructed one with the same capacity.
      */
-    public void reset() {
+    void reset() {
         setZeroBucket(ZeroBucket.minimalEmpty());
         resetBuckets(MAX_SCALE);
     }
@@ -67,7 +59,7 @@ public void reset() {
      *
      * @param scale the scale to set for this histogram
      */
-    public void resetBuckets(int scale) {
+    void resetBuckets(int scale) {
         if (scale > MAX_SCALE || scale < MIN_SCALE) {
             throw new IllegalArgumentException("scale must be in range [" + MIN_SCALE + ".." + MAX_SCALE + "]");
         }
@@ -88,7 +80,7 @@ public ZeroBucket zeroBucket() {
      *
      * @param zeroBucket the zero bucket to set
      */
-    public void setZeroBucket(ZeroBucket zeroBucket) {
+    void setZeroBucket(ZeroBucket zeroBucket) {
         this.zeroBucket = zeroBucket;
     }
 
@@ -112,7 +104,7 @@ public void setZeroBucket(ZeroBucket zeroBucket) {
      * @param isPositive {@code true} if the bucket belongs to the positive range, {@code false} if it belongs to the negative range
      * @return {@code true} if the bucket was added, {@code false} if it could not be added due to insufficient capacity
      */
-    public boolean tryAddBucket(long index, long count, boolean isPositive) {
+    boolean tryAddBucket(long index, long count, boolean isPositive) {
         if (index < MIN_INDEX || index > MAX_INDEX) {
             throw new IllegalArgumentException("index must be in range [" + MIN_INDEX + ".." + MAX_INDEX + "]");
         }
@@ -135,29 +127,36 @@ public int scale() {
     }
 
     @Override
-    public Buckets negativeBuckets() {
+    public ExponentialHistogram.Buckets negativeBuckets() {
         return negativeBuckets;
     }
 
     @Override
-    public Buckets positiveBuckets() {
+    public ExponentialHistogram.Buckets positiveBuckets() {
         return positiveBuckets;
     }
 
-    private abstract class AbstractBuckets implements Buckets {
+    private class Buckets implements ExponentialHistogram.Buckets {
 
+        private final boolean isPositive;
         private int numBuckets;
         private int cachedValueSumForNumBuckets;
         private long cachedValueSum;
 
-        AbstractBuckets() {
+        /**
+         * @param isPositive true, if this object should represent the positive bucket range, false for the negative range
+         */
+        Buckets(boolean isPositive) {
+            this.isPositive = isPositive;
             reset();
         }
 
         /**
          * @return the array index of the first bucket of this set of buckets within {@link #bucketCounts} and {@link #bucketIndices}.
          */
-        abstract int startSlot();
+        int startSlot() {
+            return isPositive ? negativeBuckets.numBuckets : 0;
+        }
 
         final void reset() {
             numBuckets = 0;