Skip to content

Commit fb08047

Browse files
puzpuzpuzpuzpuzpuz
committed
Revert the fix for bugs 1-3 and fix a few more issues
1 parent fa588c6 commit fb08047

File tree

8 files changed

+23
-30
lines changed

8 files changed

+23
-30
lines changed

‎core/src/main/java/io/questdb/griffin/engine/functions/groupby/AvgDoubleGroupByFunction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ public void computeBatch(MapValue mapValue, long ptr, int count, long startRowId
5252
final double batchSum = Vect.sumDoubleAcc(ptr, count, countPtr);
5353
if (!Numbers.isNull(batchSum)) {
5454
final double prevSum = mapValue.getDouble(valueIndex);
55-
if (!Double.isNaN(prevSum)) {
55+
if (Numbers.isFinite(prevSum)) {
5656
mapValue.putDouble(valueIndex, prevSum + batchSum);
5757
} else {
5858
mapValue.putDouble(valueIndex, batchSum);

‎core/src/main/java/io/questdb/griffin/engine/functions/groupby/FirstNotNullDoubleGroupByFunction.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ public void computeBatch(MapValue mapValue, long ptr, int count, long startRowId
6161
@Override
6262
public void computeNext(MapValue mapValue, Record record, long rowId) {
6363
double val = arg.getDouble(record);
64-
if (Numbers.isFinite(val)) {
64+
if (!Numbers.isNull(val)) {
6565
if (Numbers.isNull(mapValue.getDouble(valueIndex + 1)) || rowId < mapValue.getLong(valueIndex)) {
6666
mapValue.putLong(valueIndex, rowId);
6767
mapValue.putDouble(valueIndex + 1, val);
@@ -77,7 +77,7 @@ public String getName() {
7777
@Override
7878
public void merge(MapValue destValue, MapValue srcValue) {
7979
double srcVal = srcValue.getDouble(valueIndex + 1);
80-
if (Numbers.isFinite(srcVal)) {
80+
if (!Numbers.isNull(srcVal)) {
8181
long srcRowId = srcValue.getLong(valueIndex);
8282
long destRowId = destValue.getLong(valueIndex);
8383
// srcRowId is non-null at this point since we know that the value is non-null

‎core/src/main/java/io/questdb/griffin/engine/functions/groupby/LastNotNullDoubleGroupByFunction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ public void computeBatch(MapValue mapValue, long ptr, int count, long startRowId
6060

6161
@Override
6262
public void computeNext(MapValue mapValue, Record record, long rowId) {
63-
if (Numbers.isFinite(arg.getDouble(record))) {
63+
if (!Numbers.isNull(arg.getDouble(record))) {
6464
if (Numbers.isNull(mapValue.getDouble(valueIndex + 1)) || rowId > mapValue.getLong(valueIndex)) {
6565
computeFirst(mapValue, record, rowId);
6666
}

‎core/src/main/java/io/questdb/griffin/engine/functions/groupby/SumDoubleGroupByFunction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ public void computeBatch(MapValue mapValue, long ptr, int count, long startRowId
5050
final double batchSum = Vect.sumDouble(ptr, count);
5151
if (Numbers.isFinite(batchSum)) {
5252
final double existing = mapValue.getDouble(valueIndex);
53-
if (!Double.isNaN(existing)) {
53+
if (Numbers.isFinite(existing)) {
5454
mapValue.putDouble(valueIndex, existing + batchSum);
5555
} else {
5656
mapValue.putDouble(valueIndex, batchSum);

‎core/src/main/java/io/questdb/griffin/engine/functions/groupby/SumFloatGroupByFunction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public void computeBatch(MapValue mapValue, long ptr, int count, long startRowId
5858
}
5959
if (hasFinite) {
6060
final float existing = mapValue.getFloat(valueIndex);
61-
if (!Float.isNaN(existing)) {
61+
if (Float.isFinite(existing)) {
6262
mapValue.putFloat(valueIndex, existing + acc);
6363
} else {
6464
mapValue.putFloat(valueIndex, acc);

‎core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/CharGroupByFunctionBatchTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ public void testMinCharBatchSimple() {
241241
value.putChar(function.getValueIndex(), CharConstant.ZERO.getChar(null));
242242

243243
long ptr = allocateChars('d', 'b', 'c');
244-
function.computeBatch(value, ptr, 4, 0);
244+
function.computeBatch(value, ptr, 3, 0);
245245

246246
Assert.assertEquals('b', function.getChar(value));
247247
Assert.assertTrue(function.supportsBatchComputation());

‎core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/DoubleGroupByFunctionBatchTest.java‎

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -57,13 +57,10 @@ public void tearDown() {
5757
}
5858
}
5959

60-
// Reproduces Bug 1 from CodeRabbit review of PR #6805:
61-
// AvgDoubleGroupByFunction.computeBatch uses Numbers.isFinite(prevSum) to distinguish "not yet
62-
// initialized" (NaN sentinel) from "has accumulated value". But isFinite() returns false for both
63-
// NaN and ±Infinity. When the running sum legitimately overflows to +Infinity across batches,
64-
// the next finite batch overwrites it instead of accumulating into it.
60+
// Verify that computeBatch is consistent with computeNext: when the running sum
61+
// overflows to +Infinity, the next finite batch overwrites it (resets the accumulator).
6562
@Test
66-
public void testAvgDoubleBatchAccumulatedInfinityIsPreserved() {
63+
public void testAvgDoubleBatchAccumulatedInfinityIsOverwritten() {
6764
AvgDoubleGroupByFunction function = new AvgDoubleGroupByFunction(DoubleColumn.newInstance(COLUMN_INDEX));
6865
try (SimpleMapValue value = prepare(function)) {
6966
// Batch 1: running sum = MAX_VALUE (finite)
@@ -74,12 +71,12 @@ public void testAvgDoubleBatchAccumulatedInfinityIsPreserved() {
7471
ptr = allocateDoubles(Double.MAX_VALUE);
7572
function.computeBatch(value, ptr, 1, 0);
7673

77-
// Batch 3: a finite batch must not overwrite accumulated Infinity
74+
// Batch 3: Infinity is not finite, so the accumulator resets to 1.0
7875
ptr = allocateDoubles(1.0);
7976
function.computeBatch(value, ptr, 1, 0);
8077

81-
// avg = Infinity / 3 = Infinity
82-
Assert.assertTrue(Double.isInfinite(function.getDouble(value)));
78+
// avg = 1.0 / 3 (count is still 3)
79+
Assert.assertEquals(1.0 / 3.0, function.getDouble(value), 1e-15);
8380
}
8481
}
8582

@@ -421,12 +418,10 @@ public void testSumDoubleBatch() {
421418
}
422419
}
423420

424-
// Reproduces Bug 2 from CodeRabbit review of PR #6805:
425-
// SumDoubleGroupByFunction.computeBatch uses Numbers.isFinite(existing) to distinguish the NaN
426-
// sentinel (empty state) from a real accumulated value. But isFinite() returns false for ±Infinity
427-
// too, so when the running sum overflows to +Infinity, the next finite batch replaces it.
421+
// Verify that computeBatch is consistent with computeNext: when the running sum
422+
// overflows to +Infinity, the next finite batch overwrites it (resets the accumulator).
428423
@Test
429-
public void testSumDoubleBatchAccumulatedInfinityIsPreserved() {
424+
public void testSumDoubleBatchAccumulatedInfinityIsOverwritten() {
430425
SumDoubleGroupByFunction function = new SumDoubleGroupByFunction(DoubleColumn.newInstance(COLUMN_INDEX));
431426
try (SimpleMapValue value = prepare(function)) {
432427
// Batch 1: running sum = MAX_VALUE (finite)
@@ -439,10 +434,10 @@ public void testSumDoubleBatchAccumulatedInfinityIsPreserved() {
439434
function.computeBatch(value, ptr, 1, 0);
440435
Assert.assertEquals(Double.POSITIVE_INFINITY, function.getDouble(value), 0.0);
441436

442-
// Batch 3: accumulated Infinity must not be replaced by the finite batch sum
437+
// Batch 3: Infinity is not finite, so the accumulator resets to 1.0
443438
ptr = allocateDoubles(1.0);
444439
function.computeBatch(value, ptr, 1, 0);
445-
Assert.assertEquals(Double.POSITIVE_INFINITY, function.getDouble(value), 0.0);
440+
Assert.assertEquals(1.0, function.getDouble(value), 0.0);
446441
}
447442
}
448443

‎core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/FloatGroupByFunctionBatchTest.java‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -392,12 +392,10 @@ public void testSumFloatBatch() {
392392
}
393393
}
394394

395-
// Reproduces Bug 3 from CodeRabbit review of PR #6805:
396-
// SumFloatGroupByFunction.computeBatch uses Float.isFinite(existing) to distinguish the NaN
397-
// sentinel (empty state) from a real accumulated value. But isFinite() returns false for ±Infinity
398-
// too, so when the running sum overflows to +Infinity, the next finite batch replaces it.
395+
// Verify that computeBatch is consistent with computeNext: when the running sum
396+
// overflows to +Infinity, the next finite batch overwrites it (resets the accumulator).
399397
@Test
400-
public void testSumFloatBatchAccumulatedInfinityIsPreserved() {
398+
public void testSumFloatBatchAccumulatedInfinityIsOverwritten() {
401399
SumFloatGroupByFunction function = new SumFloatGroupByFunction(FloatColumn.newInstance(COLUMN_INDEX));
402400
try (SimpleMapValue value = prepare(function)) {
403401
// Batch 1: running sum = MAX_VALUE (finite)
@@ -410,10 +408,10 @@ public void testSumFloatBatchAccumulatedInfinityIsPreserved() {
410408
function.computeBatch(value, ptr, 1, 0);
411409
Assert.assertEquals(Float.POSITIVE_INFINITY, function.getFloat(value), 0.0f);
412410

413-
// Batch 3: accumulated Infinity must not be replaced by the finite batch sum
411+
// Batch 3: Infinity is not finite, so the accumulator resets to 1.0
414412
ptr = allocateFloats(1.0f);
415413
function.computeBatch(value, ptr, 1, 0);
416-
Assert.assertEquals(Float.POSITIVE_INFINITY, function.getFloat(value), 0.0f);
414+
Assert.assertEquals(1.0f, function.getFloat(value), 0.0f);
417415
}
418416
}
419417

0 commit comments

Comments
 (0)