Jackie-Jiang commented on code in PR #20059:
URL: https://github.com/apache/pinot/pull/20059#discussion_r4383723560
##########
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:
The tolerance can admit a genuinely between-step endpoint even when no
rounding error occurs. For example, `generateFloatArray(1000000f, 1000000.75f,
1f)` now returns `[1000000, 1000001]`, whereas the base returns only
`[1000000]`. All inputs and the `0.75` step count are exactly representable,
but the tolerance is about `0.250000238`, so it accepts the quarter-step
overshoot. Likewise, `generateDoubleArray(1e15, 1e15 + 1.5, 1d)` now includes
`1e15 + 2`, beyond the end. Please tighten the snapping criterion so large
offset ULPs do not turn genuine fractional step counts into whole steps, and
add regression cases for both types.
##########
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;
Review Comment:
Minor: this still computes both the subtraction and division in `float`;
assignment to `double` only widens the completed result. To calculate in double
precision as the new comments and PR description claim, promote an operand
before subtraction, e.g. `((double) end - start) / inc`, or correct the
documentation if retaining float arithmetic is intentional. The existing
arithmetic behavior predates this PR; the mismatch is in the new documentation.
--
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]
