Skip to content

Commit a5243a1

Browse files
authored
fix: NumericHistogram handle NaN cases (prestodb#25793)
## Description fix: NumericHistogram handle NaN cases Previous it will fail when value is Nan, and the penalty becomes Nan, which is larger than Inf. So it breaks the assumptions: right is guaranteed to exist because we set the penalty of the last bucket to infinity. ## Motivation and Context The unit test will fail before the fix and succeed after fix. ## Impact fix some edge cases for the function ## Test Plan Unit Tests ## Contributor checklist - [x] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [x] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [x] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [x] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [x] Adequate tests were added if applicable. - [x] CI passed. ## Release Notes ``` == NO RELEASE NOTE == ```
1 parent 6b10400 commit a5243a1

File tree

2 files changed

+20
-3
lines changed

2 files changed

+20
-3
lines changed

presto-main-base/src/main/java/com/facebook/presto/operator/aggregation/NumericHistogram.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ private static PriorityQueue<Entry> mergeBuckets(double[] values, double[] weigh
184184

185185
Entry right = current.getRight();
186186

187-
// right is guaranteed to exist because we set the penalty of the last bucket to infinity
187+
// right is guaranteed to exist because we set the penalty of the last bucket to NaN
188188
// so the first current in the queue can never be the last bucket
189189
checkState(right != null, "Expected right to be != null");
190190
checkState(right.isValid(), "Expected right to be valid");
@@ -250,7 +250,7 @@ private static int mergeSameBuckets(double[] values, double[] weights, int nextI
250250

251251
int current = 0;
252252
for (int i = 1; i < nextIndex; i++) {
253-
if (values[current] == values[i]) {
253+
if (values[current] == values[i] || (Double.isNaN(values[current]) && Double.isNaN(values[i]))) {
254254
weights[current] += weights[i];
255255
}
256256
else {
@@ -350,7 +350,7 @@ private Entry(int id, double value, double weight, Entry left, Entry right)
350350
penalty = computePenalty(value, weight, right.value, right.weight);
351351
}
352352
else {
353-
penalty = Double.POSITIVE_INFINITY;
353+
penalty = Double.NaN;
354354
}
355355

356356
if (left != null) {

presto-main-base/src/test/java/com/facebook/presto/operator/aggregation/TestNumericHistogram.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,4 +133,21 @@ public void testMergeDifferent()
133133
histogram1.mergeWith(histogram2);
134134
assertEquals(histogram1.getBuckets(), expected.getBuckets());
135135
}
136+
137+
@Test
138+
public void testNaN()
139+
{
140+
NumericHistogram histogram = new NumericHistogram(2, 100);
141+
142+
histogram.add(Double.NaN, 1);
143+
histogram.add(2, 1);
144+
histogram.add(Double.NaN, 1);
145+
146+
Map<Double, Double> expected = ImmutableMap.<Double, Double>builder()
147+
.put(Double.NaN, 2.0)
148+
.put(2.0, 1.0)
149+
.build();
150+
151+
assertEquals(histogram.getBuckets(), expected);
152+
}
136153
}

0 commit comments

Comments
 (0)