[SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode

[gengliangwang]

What changes were proposed in this pull request?

In BinaryArithmetic.doGenCode, the Byte/Short ANSI overflow-check branch previously emitted ~7 lines per call site (int tmpResult + overflow check + cast back). After this PR it emits a single static call into the existing ByteExactNumeric / ShortExactNumeric (in numerics.scala), which already implements the same overflow check + BINARY_ARITHMETIC_OVERFLOW error that the eval path uses.

The codegen rewrite uses the same getClass.getCanonicalName.stripSuffix("$") pattern as the adjacent MathUtils / IntervalMathUtils calls. The Scala compiler emits public static forwarders on the companion class of top-level objects, so the generated Java code calls e.g. org.apache.spark.sql.types.ByteExactNumeric.plus(a, b) directly.

Primitive int/long/float/double branches are intentionally left inline (single bytecode op; routing those through a static method would be a runtime regression).

Why are the changes needed?

Part of SPARK-56908 (umbrella). The Byte/Short ANSI branch is the largest single inline body in BinaryArithmetic.doGenCode.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

build/sbt "catalyst/testOnly *ArithmeticExpressionSuite"

35/35 pass.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Cursor 1.x

[gengliangwang]

---

Stack overview (SPARK-56908 umbrella)

This PR is part of a stack of 8 PRs against SPARK-56908. Order:

1. [SPARK-56909][SQL] Simplify Cast to int/long codegen under ANSI mode #55934 — [SPARK-56909][SQL] Simplify Cast to int/long codegen under ANSI mode (this stack base)
2. [SPARK-56910][SQL] Simplify Cast to byte/short codegen under ANSI mode #55935 — [SPARK-56910][SQL] Simplify Cast to byte/short codegen under ANSI mode
3. [SPARK-56911][SQL] Simplify Cast to decimal codegen under ANSI mode #55936 — [SPARK-56911][SQL] Simplify Cast to decimal codegen under ANSI mode
4. [SPARK-56912][SQL] Simplify Cast to boolean codegen under ANSI mode #55937 — [SPARK-56912][SQL] Simplify Cast to boolean codegen under ANSI mode
5. [SPARK-56914][SQL] Simplify decimal arithmetic codegen under ANSI mode #55939 — [SPARK-56914][SQL] Simplify decimal arithmetic codegen under ANSI mode (depends on [SPARK-56911][SQL] Simplify Cast to decimal codegen under ANSI mode #55936)
6. [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode #55938 — [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode (independent)
7. [SPARK-56915][SQL] Simplify MakeDate/MakeInterval codegen under ANSI mode #55940 — [SPARK-56915][SQL] Simplify MakeDate/MakeInterval codegen under ANSI mode (independent)
8. [SPARK-56916][SQL] Simplify ElementAt array codegen under ANSI mode #55941 — [SPARK-56916][SQL] Simplify ElementAt array codegen under ANSI mode (independent)

PRs 1-4 are linearly stacked on each other (each branch is based on the previous one). PR 5 (decimal arithmetic) is stacked on top of PR 3 (cast decimal) since it uses CastUtils.changePrecisionExact. PRs 6, 7, 8 branch off master independently.

[viirya]

A focused refactor that pulls the ANSI overflow-check for Byte/Short arithmetic out of an inline codegen template and into a small Java helper. Behaviorally a no-op — but the PR description's justification for the eval-side changes (claiming the eval path used to throw a "generic ArithmeticException") is factually wrong: the eval path already produced BINARY_ARITHMETIC_OVERFLOW via ByteExactNumeric/ShortExactNumeric. The code itself is fine; the description needs correction and the eval-side additions are arguably redundant.

[viirya]

PR description:

the eval path now produces the same SQL-formatted BINARY_ARITHMETIC_OVERFLOW error as the codegen path (the previous generic ArithmeticException was an inconsistency).

This is wrong. Before this PR, the eval path for ANSI Byte goes:

• nullSafeEval → case _ => numeric.plus(input1, input2) (line 461)
• numeric is TypeUtils.getNumeric(dataType, failOnError=true) (line 442), which returns ByteExactNumeric
• ByteExactNumeric.plus(x, y) (numerics.scala:33-37) computes tmp = x + y, then calls checkOverflow which throws
  QueryExecutionErrors.binaryArithmeticCauseOverflowError(x, "+", y, "try_add")

That is the same call as the new helper, with the same error class BINARY_ARITHMETIC_OVERFLOW and identical message parameters. There was no generic ArithmeticException. Same for Short.

So the new case _: ByteType if failOnError => ArithmeticUtils.byteAddExact(...) branches added to Add/Subtract/Multiply are redundant: they replace one route to the same error with a different route to the same error. Behaviorally identical.

[viirya]

Duplication with ByteExactNumeric / ShortExactNumeric. numerics.scala already has plus/minus/times that do exactly the same int-promoted check and throw the same error. Now there are two implementations of the same logic — one in numerics.scala (still used) and one in ArithmeticUtils.java (used by the new eval branches and by codegen).

[gengliangwang]

Good catch. The new commit adds a class-javadoc note explicitly calling out that these helpers mirror ByteExactNumeric / ShortExactNumeric and exist only because Scala object methods can't be called from generated code without going through references[]. I also dropped the redundant eval-path branches (see other replies), so numerics.scala is now the sole eval-path implementation.

[viirya]

In practice, only Add/Subtract/Multiply reach BinaryArithmetic.doGenCode with byte/short (Divide/Remainder/Pmod/IntegralDivide override doGenCode via DivModLike). Reasonable defensive coding, but the notOverrideExpectedMethodsError message ("genCode") is a slightly awkward fit — it's normally used for missing method overrides, not for a runtime symbol mismatch. A SparkException.internalError(s"Unexpected symbol $symbol for Byte/Short BinaryArithmetic") would read more accurately.

[gengliangwang]

Done — switched to SparkException.internalError(s"Unexpected symbol '$symbol' for Byte/Short BinaryArithmetic").

[viirya]

Why route Add/Subtract/Multiply.nullSafeEval through the helper rather than just leaving them as numeric.plus(...)? The pre-PR case _ => numeric.plus(...) was concise and already correct. The new ANSI-specific branches add 8 lines × 3 expressions = 24 lines of arguably unnecessary code.

Consider dropping the eval-side branches (Add/Subtract/Multiply.nullSafeEval byte/short cases). They're redundant with the existing numeric.plus-via-ByteExactNumeric route.

[gengliangwang]

Agreed — reverted. Add.nullSafeEval now falls through to numeric.plus for byte/short (which already routes through ByteExactNumeric/ShortExactNumeric and throws the same SQL-formatted overflow error). The pre-PR statement in the description about a generic ArithmeticException was wrong; sorry for the noise.

[viirya]

ditto

[gengliangwang]

Reverted — Subtract.nullSafeEval now falls through to numeric.minus for byte/short.

[viirya]

ditto

[gengliangwang]

Reverted — Multiply.nullSafeEval now falls through to numeric.times for byte/short.

[cloud-fan]

Summary

The Byte/Short ANSI overflow branch of BinaryArithmetic.doGenCode previously emitted ~7 inline lines per call site (int temp + range check + cast back). This PR extracts those lines into 6 helpers in a new ArithmeticUtils.java and reduces the codegen to a single call. Non-ANSI byte/short is also collapsed to a one-line inline cast. Int/long branches are intentionally left inline.

After @viirya's review, the duplicate eval-path branches for Add/Subtract/Multiply have been reverted, so the eval path still routes through numeric.plus/minus/times → ByteExactNumeric/ShortExactNumeric, producing the same BINARY_ARITHMETIC_OVERFLOW error as the new codegen path. Existing *ArithmeticExpressionSuite tests (including checkConsistencyBetweenInterpretedAndCodegenAllowingException for ByteType/ShortType under both ANSI modes) cover the path, so no new tests are required.

Key design decision worth revisiting

The Javadoc and PR description justify the new Java helper by claiming that "Scala object methods can't be called from generated code without going through the references[] array." This isn't quite right — see inline comment.

[cloud-fan]

The rationale here is contradicted by existing code. MathUtils and IntervalMathUtils are both top-level Scala objects and are called directly from BinaryArithmetic.doGenCode via getCanonicalName.stripSuffix("$") (arithmetic.scala:301, 324) — no references[] needed. The trick works because the Scala compiler emits public static forwarders on top-level objects' companion class.

ByteExactNumeric and ShortExactNumeric are also top-level objects, and javap on the compiled classes confirms the forwarders exist:

$ javap .../org/apache/spark/sql/types/ByteExactNumeric.class
public final class org.apache.spark.sql.types.ByteExactNumeric {
  public static byte plus(byte, byte);
  public static byte minus(byte, byte);
  public static byte times(byte, byte);
  ...

(private[sql] is enforced at Scala compile time only; at the bytecode level the class is public final and the methods are public static.) So the codegen could call them directly, e.g.:

case ByteType | ShortType if failOnError =>
  val methodName = symbol match {
    case "+" => "plus"
    case "-" => "minus"
    case "*" => "times"
    case _ => throw SparkException.internalError(
      s"Unexpected symbol '$symbol' for Byte/Short BinaryArithmetic")
  }
  val numericObj = (if (dataType == ByteType) ByteExactNumeric else ShortExactNumeric)
    .getClass.getCanonicalName.stripSuffix("$")
  defineCodeGen(ctx, ev, (eval1, eval2) => s"$numericObj.$methodName($eval1, $eval2)")

This would eliminate the new file entirely and resolve the duplication @viirya raised, rather than documenting it. Did you try this path and hit a concrete problem? If so it'd be worth capturing in the Javadoc — as written, the justification is misleading.

[gengliangwang]

You're right, sorry — private[sql] is enforced only by scalac, the bytecode is public final with public static forwarders, and Janino can call them just fine. I verified locally:

$ javap .../org/apache/spark/sql/types/ByteExactNumeric.class
public final class org.apache.spark.sql.types.ByteExactNumeric {
  public static byte plus(byte, byte);
  public static byte minus(byte, byte);
  public static byte times(byte, byte);
  ...

Pushed 94014a0 which deletes ArithmeticUtils.java entirely and rewrites the codegen to call ByteExactNumeric / ShortExactNumeric directly via getCanonicalName.stripSuffix("$") — same pattern as the MathUtils / IntervalMathUtils calls on the adjacent lines. No duplication left, and the PR is purely a codegen tightening now.

I hadn't tried it before writing the helper; that was a mistake. Thanks for the verification with javap.