Skip to content

Commit 91193bc

Browse files
JonasKunzJonasKunz
committed
Check for sane scale and indices
1 parent 2f293d0 commit 91193bc

File tree

7 files changed

+66
-36
lines changed

7 files changed

+66
-36
lines changed

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,18 @@
1111

1212
public interface ExponentialHistogram {
1313

14+
// scale of 38 is the largest scale where at the borders we don't run into problems due to floating point precision
15+
// theoretically, a MAX_SCALE of 51 would work and would still cover the entire range of double values
16+
// if we want to use something larger, we'll have to rework the math of converting from double to indices and back
17+
// One option would be to use "Quadruple": https://github.com/m-vokhm/Quadruple
18+
int MAX_SCALE = 38;
19+
20+
// Only use 62 bit at max to allow to compute the difference between the smallest and largest index without causing overflow
21+
// Also the extra bit gives us room for some tricks for compact storage
22+
int MAX_INDEX_BITS = 62;
23+
long MAX_INDEX = (1L << MAX_INDEX_BITS) - 1;
24+
long MIN_INDEX = -MAX_INDEX;
25+
1426
int scale();
1527

1628
ZeroBucket zeroBucket();

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@
99

1010
package org.elasticsearch.exponentialhistogram;
1111

12+
import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MAX_INDEX;
13+
import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MAX_INDEX_BITS;
14+
import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MIN_INDEX;
15+
1216
public class ExponentialHistogramUtils {
1317

1418
/** Bit mask used to isolate exponent of IEEE 754 double precision number. */
@@ -76,10 +80,13 @@ public static int compareLowerBoundaries(long idxA, int scaleA, long idxB, int s
7680
* of the index.
7781
*/
7882
public static int getMaximumScaleIncrease(long index) {
83+
if (index < MIN_INDEX || index > MAX_INDEX) {
84+
throw new IllegalArgumentException("index must be in range ["+MIN_INDEX+".."+MAX_INDEX+"]");
85+
}
7986
if (index < 0) {
8087
index = ~index;
8188
}
82-
return Long.numberOfLeadingZeros(index) - 1;
89+
return Long.numberOfLeadingZeros(index) - (64 - MAX_INDEX_BITS);
8390
}
8491

8592
public static double getUpperBucketBoundary(long index, int scale) {
@@ -93,7 +100,6 @@ public static double getLowerBucketBoundary(long index, int scale) {
93100
}
94101

95102
public static double getPointOfLeastRelativeError(long bucketIndex, int scale) {
96-
// TODO: handle numeric limits, implement exact algorithms with 128 bit precision
97103
double inverseFactor = Math.scalb(LN_2, -scale);
98104
return Math.exp(inverseFactor * (bucketIndex + 1/3.0));
99105
}

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

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,6 @@
1111

1212
public final class FixedSizeExponentialHistogram implements ExponentialHistogramBuilder, ExponentialHistogram {
1313

14-
// scale of 38 is the largest scale where the index computation doesn't suffer much rounding
15-
// if we want to use something larger, we'll have to rework the math
16-
public static final int DEFAULT_BUCKET_SCALE = 38;
17-
1814
private final long[] bucketIndices;
1915
private final long[] bucketCounts;
2016
private int negativeBucketCount;
@@ -32,11 +28,14 @@ public FixedSizeExponentialHistogram(int bucketCount) {
3228

3329
void reset() {
3430
setZeroBucket(ZeroBucket.minimalEmpty());
35-
resetBuckets(DEFAULT_BUCKET_SCALE);
31+
resetBuckets(MAX_SCALE);
3632
}
3733

3834
@Override
3935
public void resetBuckets(int newScale) {
36+
if (newScale > MAX_SCALE) {
37+
throw new IllegalArgumentException("scale must be <= MAX_SCALE ("+MAX_SCALE+")");
38+
}
4039
negativeBucketCount = 0;
4140
positiveBucketCount = 0;
4241
bucketScale = newScale;
@@ -54,6 +53,9 @@ public void setZeroBucket(ZeroBucket zeroBucket) {
5453

5554
@Override
5655
public boolean tryAddBucket(long index, long count, boolean isPositive) {
56+
if (index < MIN_INDEX || index > MAX_INDEX) {
57+
throw new IllegalArgumentException("index must be in range ["+MIN_INDEX+".."+MAX_INDEX+"]");
58+
}
5759
if (isPositive == false && positiveBucketCount > 0) {
5860
throw new IllegalArgumentException("Cannot add negative buckets after a positive bucket was added");
5961
}

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,17 +9,18 @@
99

1010
package org.elasticsearch.exponentialhistogram;
1111

12+
import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MIN_INDEX;
1213
import static org.elasticsearch.exponentialhistogram.ExponentialHistogramUtils.compareLowerBoundaries;
1314
import static org.elasticsearch.exponentialhistogram.ExponentialHistogramUtils.computeIndex;
1415
import static org.elasticsearch.exponentialhistogram.ExponentialHistogramUtils.getLowerBucketBoundary;
15-
import static org.elasticsearch.exponentialhistogram.FixedSizeExponentialHistogram.DEFAULT_BUCKET_SCALE;
16+
import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MAX_SCALE;
1617

1718
public record ZeroBucket(long index, int scale, long count) {
1819

19-
private static final ZeroBucket MINIMAL_EMPTY = new ZeroBucket(Long.MIN_VALUE, Integer.MIN_VALUE / 256, 0);
20+
private static final ZeroBucket MINIMAL_EMPTY = new ZeroBucket(MIN_INDEX, Integer.MIN_VALUE / 256, 0);
2021

2122
public ZeroBucket(double zeroThreshold, long count) {
22-
this(computeIndex(zeroThreshold, DEFAULT_BUCKET_SCALE) + 1, DEFAULT_BUCKET_SCALE, count);
23+
this(computeIndex(zeroThreshold, MAX_SCALE) + 1, MAX_SCALE, count);
2324
}
2425

2526
public static ZeroBucket minimalEmpty() {

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import java.util.stream.IntStream;
2121

2222
import static org.elasticsearch.exponentialhistogram.FixedSizeExponentialHistogramTests.printMidpoints;
23+
import static org.hamcrest.Matchers.closeTo;
2324
import static org.hamcrest.Matchers.equalTo;
2425

2526
public class ExponentialHistogramMergerTests extends ESTestCase {
@@ -62,7 +63,7 @@ public void testZeroThresholdCollapsesOverlappingBuckets() {
6263
third.setZeroBucket(new ZeroBucket(45.0, 1));
6364

6465
mergeResult = mergeWithMinimumScale(100, 0, mergeResult, third);
65-
assertThat(mergeResult.zeroBucket().zeroThreshold(), equalTo(45.0));
66+
assertThat(mergeResult.zeroBucket().zeroThreshold(), closeTo(45.0, 0.000001));
6667
assertThat(mergeResult.zeroBucket().count(), equalTo(1L + 14L + 42L + 7L));
6768
assertThat(mergeResult.positiveBuckets().hasNext(), equalTo(false));
6869
assertThat(mergeResult.negativeBuckets().hasNext(), equalTo(false));

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

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -13,57 +13,59 @@
1313

1414
import java.util.Random;
1515

16+
import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MAX_INDEX;
17+
import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MAX_SCALE;
18+
import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MIN_INDEX;
1619
import static org.elasticsearch.exponentialhistogram.ExponentialHistogramUtils.adjustScale;
1720
import static org.elasticsearch.exponentialhistogram.ExponentialHistogramUtils.compareLowerBoundaries;
1821
import static org.elasticsearch.exponentialhistogram.ExponentialHistogramUtils.getLowerBucketBoundary;
1922
import static org.elasticsearch.exponentialhistogram.ExponentialHistogramUtils.getMaximumScaleIncrease;
2023
import static org.elasticsearch.exponentialhistogram.ExponentialHistogramUtils.getUpperBucketBoundary;
2124
import static org.hamcrest.Matchers.equalTo;
25+
import static org.hamcrest.Matchers.greaterThan;
26+
import static org.hamcrest.Matchers.lessThan;
2227
import static org.hamcrest.Matchers.lessThanOrEqualTo;
2328

2429
public class ExponentialHistogramUtilsTests extends ESTestCase {
2530

2631
public void testMaxValue() {
27-
assertThat(getMaximumScaleIncrease(Long.MAX_VALUE), equalTo(0));
28-
assertThat(getMaximumScaleIncrease(Long.MAX_VALUE >> 1), equalTo(1));
32+
assertThat(getMaximumScaleIncrease(MAX_INDEX), equalTo(0));
33+
assertThat(getMaximumScaleIncrease(MAX_INDEX >> 1), equalTo(1));
2934

30-
assertThat(adjustScale(Long.MAX_VALUE, 0), equalTo(Long.MAX_VALUE));
31-
assertThat(adjustScale(Long.MAX_VALUE >> 1, 1), equalTo(Long.MAX_VALUE - 1));
32-
assertThat(adjustScale(Long.MAX_VALUE >> 2, 2), equalTo((Long.MAX_VALUE & ~3) + 1));
33-
assertThat(adjustScale(Long.MAX_VALUE >> 4, 4), equalTo((Long.MAX_VALUE & ~15) + 6));
35+
assertThrows(ArithmeticException.class, () -> Math.multiplyExact(MAX_INDEX, 4));
3436
}
3537

3638
public void testMinValue() {
37-
assertThat(getMaximumScaleIncrease(Long.MIN_VALUE), equalTo(0));
38-
assertThat(getMaximumScaleIncrease(Long.MIN_VALUE >> 1), equalTo(1));
39-
40-
assertThat(adjustScale(Long.MIN_VALUE, 0), equalTo(Long.MIN_VALUE));
41-
assertThat(adjustScale(Long.MIN_VALUE >> 1, 1), equalTo(Long.MIN_VALUE));
42-
assertThat(adjustScale(Long.MIN_VALUE >> 2, 2), equalTo((Long.MIN_VALUE & ~3) + 1));
43-
assertThat(adjustScale(Long.MIN_VALUE >> 4, 4), equalTo((Long.MIN_VALUE & ~15) + 6));
39+
assertThat(getMaximumScaleIncrease(MIN_INDEX), equalTo(0));
40+
assertThat(getMaximumScaleIncrease(MIN_INDEX >> 1), equalTo(1));
4441
}
4542

46-
public void testRandom() {
43+
public void testRandomIndicesScaleAdjustement() {
4744
Random rnd = new Random(42);
4845

4946
for (int i = 0; i < 100_000; i++) {
50-
long index = rnd.nextLong();
47+
long index = rnd.nextLong() % MAX_INDEX;
5148
int maxScale = getMaximumScaleIncrease(index);
5249

5350
assertThat(adjustScale(adjustScale(index, maxScale), -maxScale), equalTo(index));
54-
assertThrows(ArithmeticException.class, () -> Math.multiplyExact(adjustScale(index, maxScale), 2));
51+
if (index >0) {
52+
assertThat(adjustScale(index, maxScale) *2, greaterThan(MAX_INDEX));
53+
} else {
54+
assertThat(adjustScale(index, maxScale) *2, lessThan(MIN_INDEX));
55+
56+
}
5557
}
5658

5759
}
5860

59-
public void testRandomComparison() {
61+
public void testRandomBucketBoundaryComparison() {
6062
Random rnd = new Random(42);
6163

6264
for (int i = 0; i < 100_000; i++) {
63-
long indexA = rnd.nextLong();
64-
long indexB = rnd.nextLong();
65-
int scaleA = rnd.nextInt() % 40;
66-
int scaleB = rnd.nextInt() % 40;
65+
long indexA = rnd.nextLong() % MAX_INDEX;
66+
long indexB = rnd.nextLong() % MAX_INDEX;
67+
int scaleA = rnd.nextInt() % MAX_SCALE;
68+
int scaleB = rnd.nextInt() % MAX_SCALE;
6769

6870
double lowerBoundA = getLowerBucketBoundary(indexA, scaleA);
6971
while (Double.isInfinite(lowerBoundA)) {
@@ -77,7 +79,6 @@ public void testRandomComparison() {
7779
}
7880

7981
if (lowerBoundA != lowerBoundB) {
80-
System.out.println("Comparing " + lowerBoundA + " to " + lowerBoundB);
8182
assertThat(Double.compare(lowerBoundA, lowerBoundB), equalTo(compareLowerBoundaries(indexA, scaleA, indexB, scaleB)));
8283
}
8384
}
@@ -98,10 +99,9 @@ public void testSaneBucketBoundaries() {
9899
assertThat(getLowerBucketBoundary(0, 42), equalTo(1.0));
99100
assertThat(getLowerBucketBoundary(1, 0), equalTo(2.0));
100101
assertThat(getLowerBucketBoundary(1, -1), equalTo(4.0));
101-
assertThat(getLowerBucketBoundary(1, -2), equalTo(16.0));
102102

103-
double limit1 = getLowerBucketBoundary(Long.MAX_VALUE - 1, 56);
104-
double limit2 = getLowerBucketBoundary(Long.MAX_VALUE, 56);
103+
double limit1 = getLowerBucketBoundary(MIN_INDEX, MAX_SCALE);
104+
double limit2 = getLowerBucketBoundary(MIN_INDEX, MAX_SCALE);
105105
assertThat(limit1, lessThanOrEqualTo(limit2));
106106
}
107107
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,16 @@
1313

1414
import java.util.stream.IntStream;
1515

16+
import static org.hamcrest.Matchers.equalTo;
17+
1618
public class FixedSizeExponentialHistogramTests extends ESTestCase {
1719

20+
21+
public void testDefaultZeroBucketHasZeroThreshold() {
22+
ExponentialHistogram histo = ExponentialHistogramGenerator.createFor();
23+
assertThat(histo.zeroBucket().zeroThreshold(), equalTo(0.0));
24+
}
25+
1826
public void testPrintBuckets() {
1927
ExponentialHistogram first = ExponentialHistogramGenerator.createFor(0.01234, 42, 56789);
2028
ExponentialHistogram second = ExponentialHistogramGenerator.createFor(38, 50, 250, 257, 10001.1234);

0 commit comments

Comments
 (0)