You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "dajiaohuang (via GitHub)" <gi...@apache.org> on 2026/10/26 20:39:08 UTC
[PR] Fix floating-point generateArray range lengths [pinot]
dajiaohuang opened a new pull request, #20059:
URL: https://github.com/apache/pinot/pull/20059
## Summary
Fixes #19814 by rounding a floating-point sequence's step count to its nearest integer only when the difference is within an ULP-based tolerance derived from the start, end, and increment. Otherwise, the implementation floors the step count so an end genuinely between steps remains excluded. The float path continues to compute its step count in float arithmetic, matching the existing single-stage behavior.
Regression coverage includes the issue's decimal-step examples for both float and double ranges, ascending and descending sequences, endpoints between steps, and a large-magnitude range.
## Validation
- `./mvnw -pl pinot-common -am -Dtest=GenerateArrayFunctionsTest -Dsurefire.failIfNoSpecifiedTests=false -Dmaven.compiler.showDeprecation=true -Dmaven.compiler.showWarnings=true test` — passed, 17 tests.
- `./mvnw spotless:apply -pl pinot-common` — passed.
- `./mvnw license:format -pl pinot-common` — passed; no unrelated files changed.
- `./mvnw checkstyle:check -pl pinot-common` — passed, 0 violations.
- `./mvnw license:check -pl pinot-common` — passed, 709 files checked.
The full repository `clean install -Pbin-dist` was not run; validation was scoped to the affected module and focused test.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix floating-point generateArray range lengths [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20059:
URL: https://github.com/apache/pinot/pull/20059#issuecomment-6430111642
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/20059?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 59.11%. Comparing base ([`d085f94`](https://app.codecov.io/gh/apache/pinot/commit/d085f946779eec3287398af31ea0effa3754aed7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0c5d4df`](https://app.codecov.io/gh/apache/pinot/commit/0c5d4dfa1c4498918376c1faccfa32b3cb348aa7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (d085f94) and HEAD (0c5d4df). Click for more details.
>
> <details><summary>HEAD has 24 uploads less than BASE</summary>
>
>| Flag | BASE (d085f94) | HEAD (0c5d4df) |
>|------|------|------|
>|unittests|2|1|
>|java-25|6|1|
>|temurin|6|1|
>|lane-a|2|0|
>|integration|4|0|
>|integration1|2|0|
>|lane-b|2|0|
>|unittests2|1|0|
>|integration2|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #20059 +/- ##
=============================================
- Coverage 69.95% 59.11% -10.85%
+ Complexity 1519 1 -1518
=============================================
Files 3558 2743 -815
Lines 236374 173374 -63000
Branches 37958 28621 -9337
=============================================
- Hits 165360 102487 -62873
- Misses 58306 62230 +3924
+ Partials 12708 8657 -4051
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/20059/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/20059/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/20059/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/20059/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/20059/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.11% <100.00%> (-10.85%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/20059/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/20059/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/20059/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.11% <100.00%> (-10.85%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/20059/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.11% <100.00%> (-10.85%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/20059/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.11% <100.00%> (+0.01%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/20059/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/20059?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix floating-point generateArray range lengths [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix floating-point generateArray range lengths [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix floating-point generateArray range lengths [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #20059:
URL: https://github.com/apache/pinot/pull/20059#discussion_r4382476767
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ArrayFunctions.java:
##########
@@ -645,18 +646,26 @@ private static int generatedArrayLength(long start, long end, long inc) {
/// in `float` arithmetic, as the single-stage engine always did.
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);
+ // Keep float arithmetic for the step count, but use double precision to compare its error with the input ULPs.
Review Comment:
Small doc nit: the method javadoc just above (line 646) still says the step count is "computed in `float` arithmetic, as the single-stage engine always did," but this now computes `(end - start) / inc` in `double`. Behavior is unchanged (I verified the lengths match either way), so it's just the comment that's stale — worth updating so the javadoc matches the code.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix floating-point generateArray range lengths [pinot]
Posted by "dajiaohuang (via GitHub)" <gi...@apache.org>.
dajiaohuang commented on code in PR #20059:
URL: https://github.com/apache/pinot/pull/20059#discussion_r4382988326
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ArrayFunctions.java:
##########
@@ -645,18 +646,26 @@ private static int generatedArrayLength(long start, long end, long inc) {
/// in `float` arithmetic, as the single-stage engine always did.
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);
+ // Keep float arithmetic for the step count, but use double precision to compare its error with the input ULPs.
Review Comment:
Thanks, you are right. The float overload computes the step count in double precision; I updated the Javadoc and inline comment to say that and clarified that the tolerance uses ULPs of the original float inputs. I also corrected the PR description, which had repeated the inaccurate float-arithmetic statement. The comment-only update is commit `0c5d4dfa1c4498918376c1faccfa32b3cb348aa7`; `git diff --check` passed. The focused 17-test run remains from the unchanged implementation commit; I did not rerun tests for this documentation-only change.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org