[SPARK-58040][SQL] Extract a MathUtils.doubleToLong helper to simplify Ceil/Floor overflow codegen#57143
Open
gengliangwang wants to merge 2 commits into
Open
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
|
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)