Skip to content

[SPARK-58040][SQL] Extract a MathUtils.doubleToLong helper to simplify Ceil/Floor overflow codegen#57143

Open
gengliangwang wants to merge 2 commits into
apache:masterfrom
gengliangwang:SPARK-58040-mathutils-doubletolong
Open

[SPARK-58040][SQL] Extract a MathUtils.doubleToLong helper to simplify Ceil/Floor overflow codegen#57143
gengliangwang wants to merge 2 commits into
apache:masterfrom
gengliangwang:SPARK-58040-mathutils-doubletolong

Conversation

@gengliangwang

Copy link
Copy Markdown
Member

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:

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:

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)

@gengliangwang

Copy link
Copy Markdown
Member Author

cc @ycli12

@dongjoon-hyun dongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Thank you, @gengliangwang .

@LuciferYang

Copy link
Copy Markdown
Contributor
Run ./dev/lint-scala
Scalastyle checks passed.
The scalafmt check failed on sql/connect or sql/connect at following occurrences:

org.apache.maven.plugin.MojoExecutionException: Scalafmt: Unformatted files found
Error:  Failed to execute goal org.antipathy:mvn-scalafmt_2.13:1.1.1713302731.c3d0074:format (default-cli) on project spark-sql-api_2.13: Error formatting Scala files: Scalafmt: Unformatted files found -> [Help 1]

Before submitting your change, please make sure to format your code using the following command:
./build/mvn scalafmt:format -Dscalafmt.skip=false -Dscalafmt.validateOnly=false -Dscalafmt.changedOnly=false -pl sql/api -pl sql/connect/common -pl sql/connect/server -pl sql/connect/shims -pl sql/connect/client/jvm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants