xiangfu0 commented on code in PR #20059:
URL: https://github.com/apache/pinot/pull/20059#discussion_r4383814659
##########
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:
Still reproduces at this head: the tolerance can snap a genuine fractional
overshoot up to a whole step. `generateFloatArray(1000000f, 1000000.75f, 1f)`
returns `[1000000, 1000001]` (base: just `[1000000]`) — steps is exactly 0.75
with no rounding error, but `roundingError` is ~0.25 so it rounds up to 1 step.
Same shape for `generateDoubleArray(1e15, 1e15+1.5, 1d)`, which wrongly
includes `1e15+2`.
The tolerance grows with the magnitude of the bounds, so large offsets
swallow real quarter/half steps. Worth bounding it well below 0.5 (cap it, or
check the snapped endpoint against `end` directly) so an exact fractional step
count is never promoted.
##########
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:
The javadoc and inline comment now say the step count is computed in double
precision, but `(end - start) / inc` here is still all-float — only the
finished result widens to double. They do differ: for `0f, 0.1f, 0.3f` the
float path gives 0.3333333 vs 0.33333332505491076 in true double. Either
promote an operand first (`((double) end - start) / inc`) to match the docs, or
keep float and reword. Minor, and the behavior is pre-existing — it's just the
new wording that's off.
--
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]
