refactor(zerobucket): explicit lazy flags, factories, value semantics, retain original API

Author: Charles-Lee03

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

@@ -23,24 +23,26 @@
 
 import org.apache.lucene.util.RamUsageEstimator;
 
+import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MAX_INDEX;
 import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MAX_SCALE;
+import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MIN_INDEX;
 import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MIN_SCALE;
 import static org.elasticsearch.exponentialhistogram.ExponentialScaleUtils.compareExponentiallyScaledValues;
 import static org.elasticsearch.exponentialhistogram.ExponentialScaleUtils.computeIndex;
 import static org.elasticsearch.exponentialhistogram.ExponentialScaleUtils.exponentiallyScaledToDoubleValue;
 
 /**
  * Represents the bucket for values around zero in an exponential histogram.
- * Range: [-zeroThreshold, +zeroThreshold].
+ * The range of this bucket is {@code [-zeroThreshold, +zeroThreshold]}.
  *
  * Refactor (Task 1):
- * - Added static factory methods (fromThreshold, fromIndexAndScale,
- * minimalEmpty).
- * - Replaced sentinel lazy state (Long.MAX_VALUE / Double.NaN) with explicit
- * booleans
- * (indexComputed, thresholdComputed).
- * - Added equals, hashCode, toString for value semantics.
- * - Added package-private isIndexComputed()/isThresholdComputed() for tests.
+ * - Added static factories (fromThreshold, fromIndexAndScale) while keeping
+ * original public constructor
+ * - Introduced explicit lazy flags (indexComputed, thresholdComputed) instead
+ * of sentinels
+ * - Added value semantics (equals, hashCode, toString)
+ * - Preserved original API: minimalEmpty, minimalWithCount, merge,
+ * collapseOverlappingBuckets[ForAll], compareZeroThreshold
  */
 public final class ZeroBucket {
 
@@ -51,51 +53,102 @@ public final class ZeroBucket {
     private double realThreshold;
     private final long count;
 
+    // Explicit lazy flags
     private boolean indexComputed;
     private boolean thresholdComputed;
 
-    // Singleton minimal empty
+    // Original minimal empty singleton (index known, threshold lazy)
     private static final ZeroBucket MINIMAL_EMPTY = new ZeroBucket(
-            0L,
+            MIN_INDEX,
             MIN_SCALE,
             0L,
             true,
-            true,
-            0.0d);
+            false,
+            Double.NaN);
+
+    /*
+     * ===================== Original Public Constructor (kept)
+     * =====================
+     */
+
+    /**
+     * Original public constructor (threshold authoritative). Kept for source
+     * compatibility.
+     * Deprecated in favor of {@link #fromThreshold(double, long)}.
+     */
+    @Deprecated
+    public ZeroBucket(double zeroThreshold, long count) {
+        if (zeroThreshold < 0.0) {
+            throw new IllegalArgumentException("zeroThreshold must be >= 0 (was " + zeroThreshold + ")");
+        }
+        this.scale = MAX_SCALE;
+        this.count = count;
+        this.realThreshold = zeroThreshold;
+        this.index = 0L; // placeholder until computed
+        this.indexComputed = false;
+        this.thresholdComputed = true;
+    }
 
     /* ===================== Factory Methods ===================== */
 
+    /**
+     * Create a ZeroBucket from an explicit threshold (index lazy).
+     */
     public static ZeroBucket fromThreshold(double zeroThreshold, long count) {
         if (zeroThreshold < 0.0) {
             throw new IllegalArgumentException("zeroThreshold must be >= 0 (was " + zeroThreshold + ")");
         }
         return new ZeroBucket(
-                0L, // placeholder until index computed
+                0L,
                 MAX_SCALE,
                 count,
-                false, // index not computed yet
-                true, // threshold known
+                false,
+                true,
                 zeroThreshold);
     }
 
+    /**
+     * Create a ZeroBucket from an index + scale (threshold lazy).
+     */
     public static ZeroBucket fromIndexAndScale(long index, int scale, long count) {
         if (scale < MIN_SCALE || scale > MAX_SCALE) {
             throw new IllegalArgumentException("scale out of range: " + scale);
         }
+        if (index < MIN_INDEX || index > MAX_INDEX) {
+            throw new IllegalArgumentException("index out of range: " + index);
+        }
         return new ZeroBucket(
                 index,
                 scale,
                 count,
-                true, // index known
-                false, // threshold lazy
+                true,
+                false,
                 Double.NaN);
     }
 
+    /**
+     * @return singleton empty minimal bucket.
+     */
     public static ZeroBucket minimalEmpty() {
         return MINIMAL_EMPTY;
     }
 
-    /* ===================== Private Constructor ===================== */
+    /**
+     * Creates a zero bucket with the smallest possible threshold and a given count.
+     * If count == 0 returns the singleton.
+     */
+    public static ZeroBucket minimalWithCount(long count) {
+        if (count == 0) {
+            return MINIMAL_EMPTY;
+        }
+        // Resolve lazy threshold & index of singleton
+        double threshold = MINIMAL_EMPTY.zeroThreshold();
+        long idx = MINIMAL_EMPTY.index();
+        return resolved(threshold, idx, MINIMAL_EMPTY.scale(), count);
+    }
+
+    /* ===================== Private Constructors ===================== */
+
     private ZeroBucket(
             long index,
             int scale,
@@ -111,44 +164,54 @@ private ZeroBucket(
         this.realThreshold = realThreshold;
     }
 
-    /* ===================== Public API ===================== */
+    private static ZeroBucket resolved(double threshold, long index, int scale, long count) {
+        return new ZeroBucket(index, scale, count, true, true, threshold);
+    }
+
+    /* ===================== Accessors ===================== */
 
     public long count() {
         return count;
     }
 
+    public int scale() {
+        return scale;
+    }
+
+    /**
+     * Returns index; if threshold authoritative, compute with +1 rule (matches
+     * original code).
+     */
     public long index() {
         computeIndexIfNeeded();
         return index;
     }
 
+    /**
+     * Returns threshold; if index authoritative compute it lazily.
+     */
     public double zeroThreshold() {
         computeThresholdIfNeeded();
         return realThreshold;
     }
 
-    public int scale() {
-        return scale;
-    }
-
-    /* ===================== Lazy Computation ===================== */
+    /* ===================== Lazy Computation Helpers ===================== */
 
     private void computeIndexIfNeeded() {
         if (indexComputed == false) {
-            index = computeIndex(realThreshold, scale);
+            index = computeIndex(realThreshold, scale) + 1;
             indexComputed = true;
         }
     }
 
     private void computeThresholdIfNeeded() {
         if (thresholdComputed == false) {
-            realThreshold = exponentiallyScaledToDoubleValue(index, scale);
+            realThreshold = exponentiallyScaledToDoubleValue(index(), scale);
             thresholdComputed = true;
         }
     }
 
-    /* ===================== Package-Private for Tests ===================== */
-
+    /* Package-private for tests */
     boolean isIndexComputed() {
         return indexComputed;
     }
@@ -157,10 +220,59 @@ boolean isThresholdComputed() {
         return thresholdComputed;
     }
 
-    /* ===================== Comparison Helper ===================== */
+    /* ===================== Original Functional API ===================== */
 
-    int compareTo(long otherIndex, int otherScale) {
-        return compareExponentiallyScaledValues(index(), scale, otherIndex, otherScale);
+    public int compareZeroThreshold(ZeroBucket other) {
+        return compareExponentiallyScaledValues(index(), scale(), other.index(), other.scale());
+    }
+
+    public ZeroBucket merge(ZeroBucket other) {
+        if (other.count == 0) {
+            return this;
+        } else if (this.count == 0) {
+            return other;
+        } else {
+            long total = this.count + other.count;
+            if (this.compareZeroThreshold(other) >= 0) {
+                return resolved(this.zeroThreshold(), this.index(), this.scale(), total);
+            } else {
+                return resolved(other.zeroThreshold(), other.index(), other.scale(), total);
+            }
+        }
+    }
+
+    public ZeroBucket collapseOverlappingBucketsForAll(BucketIterator... bucketIterators) {
+        ZeroBucket current = this;
+        ZeroBucket previous;
+        do {
+            previous = current;
+            for (BucketIterator b : bucketIterators) {
+                current = current.collapseOverlappingBuckets(b);
+            }
+        } while (previous.compareZeroThreshold(current) != 0);
+        return current;
+    }
+
+    public ZeroBucket collapseOverlappingBuckets(BucketIterator buckets) {
+        long collapsedCount = 0;
+        long highestCollapsedIndex = 0;
+        while (buckets.hasNext()
+                && compareExponentiallyScaledValues(buckets.peekIndex(), buckets.scale(), index(), scale()) < 0) {
+            highestCollapsedIndex = buckets.peekIndex();
+            collapsedCount += buckets.peekCount();
+            buckets.advance();
+        }
+        if (collapsedCount == 0) {
+            return this;
+        } else {
+            long newZeroCount = count + collapsedCount;
+            long collapsedUpperBoundIndex = highestCollapsedIndex + 1;
+            if (compareExponentiallyScaledValues(index(), scale(), collapsedUpperBoundIndex, buckets.scale()) >= 0) {
+                return resolved(this.zeroThreshold(), this.index(), this.scale(), newZeroCount);
+            } else {
+                return fromIndexAndScale(collapsedUpperBoundIndex, buckets.scale(), newZeroCount);
+            }
+        }
     }
 
     /* ===================== Value Semantics ===================== */

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

@@ -33,7 +33,7 @@ public void testFromThresholdLazyIndex() {
         assertTrue(z.isThresholdComputed());
         assertFalse(z.isIndexComputed());
         assertEquals(1.25d, z.zeroThreshold(), 0.0);
-        z.index(); // compute index
+        z.index();
         assertTrue(z.isIndexComputed());
     }
 
@@ -47,30 +47,80 @@ public void testFromIndexLazyThreshold() {
         assertTrue(z.isThresholdComputed());
     }
 
+    @Test
+    public void testMinimalEmptySingleton() {
+        ZeroBucket m1 = ZeroBucket.minimalEmpty();
+        ZeroBucket m2 = ZeroBucket.minimalEmpty();
+        assertSame(m1, m2);
+        assertEquals(0L, m1.count());
+        double threshold = m1.zeroThreshold();
+        assertTrue("threshold should be non-negative", threshold >= 0.0);
+    }
+
+    @Test
+    public void testMinimalWithCount() {
+        ZeroBucket m = ZeroBucket.minimalWithCount(5L);
+        assertEquals(5L, m.count());
+        assertTrue(m.zeroThreshold() >= 0.0);
+    }
+
+    @Test
+    public void testMergeKeepsLargerThreshold() {
+        ZeroBucket a = ZeroBucket.fromThreshold(0.5d, 4L);
+        ZeroBucket b = ZeroBucket.fromThreshold(1.0d, 6L);
+        ZeroBucket merged = a.merge(b);
+        assertEquals(10L, merged.count());
+        assertEquals(b.zeroThreshold(), merged.zeroThreshold(), 0.0);
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testInvalidNegativeThreshold() {
+        ZeroBucket.fromThreshold(-0.01d, 1L);
+    }
+
     @Test
     public void testEqualityAndHashStable() {
         ZeroBucket a = ZeroBucket.fromThreshold(0.6d, 10L);
         ZeroBucket b = ZeroBucket.fromThreshold(0.6d, 10L);
         assertEquals(a, b);
         assertEquals(a.hashCode(), b.hashCode());
-        a.index(); // force index
+        a.index();
         assertEquals(a, b);
         b.index();
         assertEquals(a.hashCode(), b.hashCode());
     }
 
     @Test
-    public void testMinimalEmptySingleton() {
-        ZeroBucket m1 = ZeroBucket.minimalEmpty();
-        ZeroBucket m2 = ZeroBucket.minimalEmpty();
-        assertSame(m1, m2);
-        assertEquals(0L, m1.count());
-        assertEquals(0.0, m1.zeroThreshold(), 0.0);
-    }
+    public void testCollapseNoOverlapReturnsSame() {
+        ZeroBucket z = ZeroBucket.fromThreshold(0.5d, 2L);
+        // Empty iterator scenario -> remains same
+        BucketIterator empty = new BucketIterator() {
+            @Override
+            public void advance() {
+            }
 
-    @Test(expected = IllegalArgumentException.class)
-    public void testInvalidNegativeThreshold() {
-        ZeroBucket.fromThreshold(-0.01d, 1L);
+            @Override
+            public boolean hasNext() {
+                return false;
+            }
+
+            @Override
+            public long peekCount() {
+                throw new IllegalStateException();
+            }
+
+            @Override
+            public long peekIndex() {
+                throw new IllegalStateException();
+            }
+
+            @Override
+            public int scale() {
+                return ExponentialHistogram.MAX_SCALE;
+            }
+        };
+        ZeroBucket result = z.collapseOverlappingBuckets(empty);
+        assertSame(z, result);
     }
 
     @Test