[SPARK-58040][SQL] Extract a MathUtils.doubleToLong helper to simplify Ceil/Floor overflow codegen#57143
Closed
gengliangwang wants to merge 2 commits into
Closed
Conversation
…y Ceil/Floor overflow codegen
Member
Author
|
cc @ycli12 |
dongjoon-hyun
approved these changes
Jul 8, 2026
dongjoon-hyun
left a comment
Member
There was a problem hiding this comment.
+1, LGTM. Thank you, @gengliangwang .
LuciferYang
approved these changes
Jul 9, 2026
Contributor
|
Member
Author
|
@dongjoon-hyun @LuciferYang thanks for the review! |
gengliangwang
added a commit
that referenced
this pull request
Jul 10, 2026
…y Ceil/Floor overflow codegen
### What changes were proposed in this pull request?
`Ceil` and `Floor` (ANSI mode, `DOUBLE -> LONG`) check for overflow before casting the rounded
double to long. That check is currently implemented twice, in a private `CeilFloor` helper object in
`mathExpressions.scala`:
- `CeilFloor.doubleToLong` for the interpreted path, and
- `CeilFloor.doubleToLongCode`, which re-emits the bound check as ~9 lines of Java into every
`Ceil`/`Floor` codegen stage.
This PR replaces both with a single shared `MathUtils.doubleToLong(value, context)` utility that the
interpreted and generated code both call, and deletes the `CeilFloor` object. The generated code
collapses from the inline block:
```java
double roundedValue = java.lang.Math.ceil(input);
if (!java.lang.Double.isNaN(roundedValue) &&
(roundedValue < (double) java.lang.Long.MIN_VALUE ||
roundedValue >= (double) java.lang.Long.MAX_VALUE)) {
throw QueryExecutionErrors.arithmeticOverflowError("long overflow", "", errCtx);
}
result = (long) roundedValue;
```
to a single call:
```java
result = org.apache.spark.sql.catalyst.util.MathUtils.doubleToLong(
java.lang.Math.ceil(input), errCtx);
```
`MathUtils` is the natural home: it already hosts the other arithmetic-overflow helpers
(`addExact`, `negateExact`, `withOverflow`) and follows the same "shared by the eval and codegen
paths so the two never diverge" convention already used for `pmod`. Placing `doubleToLong` there also
means one fewer copy of the bound check emitted per stage (part of the broader effort to shrink
whole-stage-codegen output, SPARK-56908).
### Why are the changes needed?
The overflow check is type-independent bookkeeping that does not need to be re-emitted into each
generated stage. Consolidating it into `MathUtils`:
- removes the duplicated logic (the `CeilFloor.doubleToLong` eval copy and the
`doubleToLongCode` string builder collapse into one method), so the two paths cannot drift, and
- shrinks the generated code for every ANSI `CEIL`/`FLOOR` over a `DOUBLE` from the multi-line
bound check to a single call.
### Does this PR introduce _any_ user-facing change?
No. The check, the error (`ARITHMETIC_OVERFLOW`), and the query context are identical; `defineCodeGen`
applies the same null-safety wrapper the previous `nullSafeCodeGen` did. This is a pure
code-organization change.
### How was this patch tested?
Existing tests, run with whole-stage codegen both on and off:
- `MathExpressionsSuite` (`ceil` and `floor`, including the ANSI overflow cases and
`checkConsistencyBetweenInterpretedAndCodegenAllowingException` for `DoubleType`), and
- regenerated `postgreSQL/float8.sql.out` (unchanged, confirming the behavior is preserved).
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)
Closes #57143 from gengliangwang/SPARK-58040-mathutils-doubletolong.
Authored-by: Gengliang Wang <gengliang@apache.org>
Signed-off-by: Gengliang Wang <gengliang@apache.org>
(cherry picked from commit bbe1b19)
Signed-off-by: Gengliang Wang <gengliang@apache.org>
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
CeilandFloor(ANSI mode,DOUBLE -> LONG) check for overflow before casting the roundeddouble to long. That check is currently implemented twice, in a private
CeilFloorhelper object inmathExpressions.scala:CeilFloor.doubleToLongfor the interpreted path, andCeilFloor.doubleToLongCode, which re-emits the bound check as ~9 lines of Java into everyCeil/Floorcodegen stage.This PR replaces both with a single shared
MathUtils.doubleToLong(value, context)utility that theinterpreted and generated code both call, and deletes the
CeilFloorobject. The generated codecollapses from the inline block:
to a single call:
MathUtilsis the natural home: it already hosts the other arithmetic-overflow helpers(
addExact,negateExact,withOverflow) and follows the same "shared by the eval and codegenpaths so the two never diverge" convention already used for
pmod. PlacingdoubleToLongthere alsomeans one fewer copy of the bound check emitted per stage (part of the broader effort to shrink
whole-stage-codegen output, SPARK-56908).
Why are the changes needed?
The overflow check is type-independent bookkeeping that does not need to be re-emitted into each
generated stage. Consolidating it into
MathUtils:CeilFloor.doubleToLongeval copy and thedoubleToLongCodestring builder collapse into one method), so the two paths cannot drift, andCEIL/FLOORover aDOUBLEfrom the multi-linebound check to a single call.
Does this PR introduce any user-facing change?
No. The check, the error (
ARITHMETIC_OVERFLOW), and the query context are identical;defineCodeGenapplies the same null-safety wrapper the previous
nullSafeCodeGendid. This is a purecode-organization change.
How was this patch tested?
Existing tests, run with whole-stage codegen both on and off:
MathExpressionsSuite(ceilandfloor, including the ANSI overflow cases andcheckConsistencyBetweenInterpretedAndCodegenAllowingExceptionforDoubleType), andpostgreSQL/float8.sql.out(unchanged, confirming the behavior is preserved).Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)