xiangfu0 commented on code in PR #20059:
URL: https://github.com/apache/pinot/pull/20059#discussion_r4431045580
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ArrayFunctions.java:
##########
@@ -642,21 +643,29 @@ private static int generatedArrayLength(long start, long
end, long inc) {
}
/// Length of the `float` sequence `start, start + inc, ...` bounded by
`end`, inclusive. The step count is computed
- /// in `float` arithmetic, as the single-stage engine always did.
+ /// in double precision; the rounding tolerance is based on the ULPs of the
original float inputs.
private static int generatedArrayLength(float start, float end, float inc) {
checkFloatingPointArguments(start, end, inc);
- // TODO: decimal steps such as 0.1 are not exact in binary, so this can
drop the end; see apache/pinot#19814
- return checkGeneratedArrayLength((long) ((end - start) / inc + 1) - 1);
+ // Use double precision for the step count and compare its error with ULPs
of the original float inputs.
+ double steps = (end - start) / inc;
+ return generatedFloatingArrayLength(steps, Math.ulp(start), Math.ulp(end),
Math.ulp(inc), inc);
}
/// Length of the `double` sequence `start, start + inc, ...` bounded by
`end`, inclusive.
private static int generatedArrayLength(double start, double end, double
inc) {
checkFloatingPointArguments(start, end, inc);
- // Adding 1 before truncating matches the length the single-stage engine
always computed. It absorbs part of the
- // rounding error of decimal steps, so 0.1 to 0.3 by 0.1 keeps its end.
- // TODO: it does not absorb all of it, so 0.1 to 0.7 by 0.1 still drops
its end; see apache/pinot#19814
- // A span too wide for a long saturates the cast at Long.MAX_VALUE, which
the length check below rejects.
- return checkGeneratedArrayLength((long) ((end - start) / inc + 1) - 1);
+ return generatedFloatingArrayLength((end - start) / inc, Math.ulp(start),
Math.ulp(end), Math.ulp(inc), inc);
+ }
+
+ /// Snaps a floating-point step count to an integer only when the distance
is within the rounding error of the input
+ /// bounds and increments. Otherwise, truncates toward the last step that
does not pass the end.
+ private static int generatedFloatingArrayLength(double steps, double
startUlp, double endUlp, double incUlp,
+ double inc) {
+ double nearestSteps = Math.rint(steps);
+ double roundingError = 2 * (startUlp + endUlp + Math.abs(nearestSteps) *
incUlp) / Math.abs(inc);
+ double fullSteps = Math.abs(steps - nearestSteps) <= roundingError ?
nearestSteps : Math.floor(steps);
Review Comment:
**Major:** the overshoot also happens with moderate `float` inputs, not just
at 1e6/1e15 magnitudes:
```java
ArrayFunctions.generateFloatArray(91321.76f, 91322.45f, 0.09f)
// head: [91321.76, ..., 91322.39, 91322.48] (9 elements; last > end)
// base: 8 elements, ending at 91322.39 (correct)
```
In float, `steps = 7.7257` and `roundingError = 2 * (0.0078125 + 0.0078125 +
8 * ulp(0.09f)) / 0.09 ≈ 0.347`. So `|steps - 8| = 0.274` gets snapped to 8.
The generated last element, `91322.4765625`, is about 3 float ULPs past `end`
(`91322.453125`). The bounds can clearly tell the two values apart, so this is
not rounding noise. It breaks the documented contract ("stopping at the last
value that does not pass `end`"), and for a gap-filling grid it adds an extra
row.
The problem is general: when `ulp(start) + ulp(end)` is close to `inc`, the
tolerance gets close to or above 0.5 step, and rounding up to `rint` stops
meaning "within rounding error". Some options:
- Before snapping up, check that the element that will actually be
generated, `start + n * inc` in the element type, is within a few ULPs of
`end`. In a quick prototype, this kept every double decimal case correct and
rejected all three repros (this one, `1e6f..1e6f+0.75f by 1`, and
`1e15..1e15+1.5 by 1`).
- At minimum, cap the tolerance well below 0.5.
Please add regression cases for float and double.
_AI-generated review via IntentLab · Model: Claude Opus 5.5
(claude-opus-5-5)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
