You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "gengliangwang (via GitHub)" <gi...@apache.org> on 2026/05/17 23:30:47 UTC
[PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
gengliangwang opened a new pull request, #55938:
URL: https://github.com/apache/spark/pull/55938
**Title**: [SPARK-56913][SQL] Refactor BinaryArithmetic byte/short codegen under ANSI mode
**Base**: master (independent)
**Head**: gengliangwang:SPARK-56913-arithmetic-byte-short
### What changes were proposed in this pull request?
Introduce `ArithmeticUtils.java` with six static helpers (`byteAddExact`, `byteSubtractExact`, `byteMultiplyExact`, `shortAddExact`, `shortSubtractExact`, `shortMultiplyExact`) and use them from `BinaryArithmetic.doGenCode` and from `Add` / `Subtract` / `Multiply.nullSafeEval`.
The `Byte`/`Short` ANSI overflow-check branch of `BinaryArithmetic.doGenCode` previously emitted ~7 lines per call site (int tmpResult + overflow check + cast back). After this PR it emits a single `ArithmeticUtils.<type><Op>Exact(...)` call.
The eval-path counterparts for Add/Subtract/Multiply also delegate to the helpers under ANSI mode, replacing the previous fall-through to `numeric.plus`/`minus`/`times` (which threw a generic `ArithmeticException`) -- the eval path now produces the same SQL-formatted `BINARY_ARITHMETIC_OVERFLOW` error as the codegen path.
Primitive `int`/`long`/`float`/`double` branches are intentionally left inline (single bytecode op; routing 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. Compiled behavior is identical; the eval path now produces a SQL-formatted overflow error matching the codegen path (the previous generic `ArithmeticException` was an inconsistency).
### 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
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #55938:
URL: https://github.com/apache/spark/pull/55938#discussion_r3261864397
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##########
@@ -458,6 +449,10 @@ case class Add(
MathUtils.addExact(input1.asInstanceOf[Int], input2.asInstanceOf[Int], getContextOrNull())
case _: LongType if failOnError =>
MathUtils.addExact(input1.asInstanceOf[Long], input2.asInstanceOf[Long], getContextOrNull())
+ case _: ByteType if failOnError =>
+ ArithmeticUtils.byteAddExact(input1.asInstanceOf[Byte], input2.asInstanceOf[Byte])
+ case _: ShortType if failOnError =>
+ ArithmeticUtils.shortAddExact(input1.asInstanceOf[Short], input2.asInstanceOf[Short])
Review Comment:
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.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #55938:
URL: https://github.com/apache/spark/pull/55938#discussion_r3261847386
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##########
@@ -301,31 +301,22 @@ abstract class BinaryArithmetic extends BinaryOperator with SupportQueryContext
val mathUtils = IntervalMathUtils.getClass.getCanonicalName.stripSuffix("$")
defineCodeGen(ctx, ev, (eval1, eval2) => s"$mathUtils.${exactMathMethod.get}($eval1, $eval2)")
// byte and short are casted into int when add, minus, times or divide
+ case ByteType | ShortType if failOnError =>
+ val opName = symbol match {
+ case "+" => "Add"
+ case "-" => "Subtract"
+ case "*" => "Multiply"
+ case _ =>
+ throw QueryExecutionErrors.notOverrideExpectedMethodsError(this.getClass.getName,
+ s"genCode for Byte/Short with symbol '$symbol'", "genCode")
Review Comment:
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.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #55938:
URL: https://github.com/apache/spark/pull/55938#discussion_r3261869314
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##########
@@ -625,6 +624,10 @@ case class Multiply(
input1.asInstanceOf[Long],
input2.asInstanceOf[Long],
getContextOrNull())
+ case _: ByteType if failOnError =>
+ ArithmeticUtils.byteMultiplyExact(input1.asInstanceOf[Byte], input2.asInstanceOf[Byte])
+ case _: ShortType if failOnError =>
+ ArithmeticUtils.shortMultiplyExact(input1.asInstanceOf[Short], input2.asInstanceOf[Short])
Review Comment:
ditto
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #55938:
URL: https://github.com/apache/spark/pull/55938#discussion_r3261864397
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##########
@@ -458,6 +449,10 @@ case class Add(
MathUtils.addExact(input1.asInstanceOf[Int], input2.asInstanceOf[Int], getContextOrNull())
case _: LongType if failOnError =>
MathUtils.addExact(input1.asInstanceOf[Long], input2.asInstanceOf[Long], getContextOrNull())
+ case _: ByteType if failOnError =>
+ ArithmeticUtils.byteAddExact(input1.asInstanceOf[Byte], input2.asInstanceOf[Byte])
+ case _: ShortType if failOnError =>
+ ArithmeticUtils.shortAddExact(input1.asInstanceOf[Short], input2.asInstanceOf[Short])
Review Comment:
Consider dropping the eval-side branches (Add/Subtract/Multiply.nullSafeEval byte/short cases). They're redundant with the existing numeric.plus-via-ByteExactNumeric route.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #55938:
URL: https://github.com/apache/spark/pull/55938#discussion_r3274453292
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ArithmeticUtils.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.expressions;
+
+import org.apache.spark.sql.errors.QueryExecutionErrors;
+
+/**
+ * Static helpers used by {@code BinaryArithmetic.doGenCode} for ANSI
+ * overflow-checked {@code byte} and {@code short} arithmetic. These mirror
+ * {@code ByteExactNumeric} / {@code ShortExactNumeric} in {@code numerics.scala}
+ * (which the eval path uses); the Java helpers exist only because Scala
+ * {@code object} methods can't be called from generated code without going
+ * through the {@code references[]} array. Primitive {@code int} / {@code long} /
+ * {@code float} / {@code double} arithmetic stays inline -- routing those
+ * single-bytecode operations through a static method would be a runtime
+ * regression.
+ */
Review Comment:
The rationale here is contradicted by existing code. `MathUtils` and `IntervalMathUtils` are both top-level Scala `object`s 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.:
```scala
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.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #55938:
URL: https://github.com/apache/spark/pull/55938#issuecomment-4472860367
---
### Stack overview (SPARK-56908 umbrella)
This PR is part of a stack of 8 PRs against SPARK-56908. Order:
1. #55934 — [SPARK-56909][SQL] Simplify Cast to int/long codegen under ANSI mode *(this stack base)*
2. #55935 — [SPARK-56910][SQL] Simplify Cast to byte/short codegen under ANSI mode
3. #55936 — [SPARK-56911][SQL] Simplify Cast to decimal codegen under ANSI mode
4. #55937 — [SPARK-56912][SQL] Simplify Cast to boolean codegen under ANSI mode
5. #55939 — [SPARK-56914][SQL] Simplify decimal arithmetic codegen under ANSI mode *(depends on #55936)*
6. #55938 — [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode *(independent)*
7. #55940 — [SPARK-56915][SQL] Simplify MakeDate/MakeInterval codegen under ANSI mode *(independent)*
8. #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.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #55938:
URL: https://github.com/apache/spark/pull/55938#discussion_r3279943747
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##########
@@ -301,31 +301,21 @@ abstract class BinaryArithmetic extends BinaryOperator with SupportQueryContext
val mathUtils = IntervalMathUtils.getClass.getCanonicalName.stripSuffix("$")
defineCodeGen(ctx, ev, (eval1, eval2) => s"$mathUtils.${exactMathMethod.get}($eval1, $eval2)")
// byte and short are casted into int when add, minus, times or divide
+ 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)")
case ByteType | ShortType =>
nullSafeCodeGen(ctx, ev, (eval1, eval2) => {
- val tmpResult = ctx.freshName("tmpResult")
- val try_suggestion = symbol match {
- case "+" => "try_add"
- case "-" => "try_subtract"
- case "*" => "try_multiply"
- case _ => "unknown_function"
- }
- val overflowCheck = if (failOnError) {
- val javaType = CodeGenerator.boxedType(dataType)
- s"""
- |if ($tmpResult < $javaType.MIN_VALUE || $tmpResult > $javaType.MAX_VALUE) {
- | throw QueryExecutionErrors.binaryArithmeticCauseOverflowError(
- | $eval1, "$symbol", $eval2, "$try_suggestion");
- |}
- """.stripMargin
- } else {
- ""
- }
- s"""
- |${CodeGenerator.JAVA_INT} $tmpResult = $eval1 $symbol $eval2;
- |$overflowCheck
- |${ev.value} = (${CodeGenerator.javaType(dataType)})($tmpResult);
- """.stripMargin
+ s"${ev.value} = (${CodeGenerator.javaType(dataType)})($eval1 $symbol $eval2);"
})
Review Comment:
nit (optional): now that the body is a single `ev.value = …;` assignment, this branch could also use `defineCodeGen` for visual symmetry with the new `failOnError` branch directly above (and with the `AnsiIntervalType` branch on lines 297–302):
```suggestion
case ByteType | ShortType =>
defineCodeGen(ctx, ev, (eval1, eval2) =>
s"(${CodeGenerator.javaType(dataType)})($eval1 $symbol $eval2)")
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #55938:
URL: https://github.com/apache/spark/pull/55938#discussion_r3261837817
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ArithmeticUtils.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.expressions;
+
+import org.apache.spark.sql.errors.QueryExecutionErrors;
+
+/**
+ * Static helpers used by {@code BinaryArithmetic.doGenCode} (and
+ * corresponding eval paths) for ANSI overflow-checked {@code byte} and
+ * {@code short} arithmetic. Primitive {@code int} / {@code long} /
+ * {@code float} / {@code double} arithmetic stays inline -- routing those
+ * single-bytecode operations through a static method would be a runtime
+ * regression.
+ */
+public final class ArithmeticUtils {
Review Comment:
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).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #55938:
URL: https://github.com/apache/spark/pull/55938#discussion_r3261866421
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##########
@@ -555,6 +550,10 @@ case class Subtract(
input1.asInstanceOf[Long],
input2.asInstanceOf[Long],
getContextOrNull())
+ case _: ByteType if failOnError =>
+ ArithmeticUtils.byteSubtractExact(input1.asInstanceOf[Byte], input2.asInstanceOf[Byte])
+ case _: ShortType if failOnError =>
+ ArithmeticUtils.shortSubtractExact(input1.asInstanceOf[Short], input2.asInstanceOf[Short])
Review Comment:
ditto
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #55938:
URL: https://github.com/apache/spark/pull/55938#discussion_r3277644474
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ArithmeticUtils.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.expressions;
+
+import org.apache.spark.sql.errors.QueryExecutionErrors;
+
+/**
+ * Static helpers used by {@code BinaryArithmetic.doGenCode} for ANSI
+ * overflow-checked {@code byte} and {@code short} arithmetic. These mirror
+ * {@code ByteExactNumeric} / {@code ShortExactNumeric} in {@code numerics.scala}
+ * (which the eval path uses); the Java helpers exist only because Scala
+ * {@code object} methods can't be called from generated code without going
+ * through the {@code references[]} array. Primitive {@code int} / {@code long} /
+ * {@code float} / {@code double} arithmetic stays inline -- routing those
+ * single-bytecode operations through a static method would be a runtime
+ * regression.
+ */
Review Comment:
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`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #55938: [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode
URL: https://github.com/apache/spark/pull/55938
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #55938:
URL: https://github.com/apache/spark/pull/55938#issuecomment-4481678703
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.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #55938:
URL: https://github.com/apache/spark/pull/55938#discussion_r3269035728
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ArithmeticUtils.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.expressions;
+
+import org.apache.spark.sql.errors.QueryExecutionErrors;
+
+/**
+ * Static helpers used by {@code BinaryArithmetic.doGenCode} (and
+ * corresponding eval paths) for ANSI overflow-checked {@code byte} and
+ * {@code short} arithmetic. Primitive {@code int} / {@code long} /
+ * {@code float} / {@code double} arithmetic stays inline -- routing those
+ * single-bytecode operations through a static method would be a runtime
+ * regression.
+ */
+public final class ArithmeticUtils {
Review Comment:
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.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##########
@@ -301,31 +301,22 @@ abstract class BinaryArithmetic extends BinaryOperator with SupportQueryContext
val mathUtils = IntervalMathUtils.getClass.getCanonicalName.stripSuffix("$")
defineCodeGen(ctx, ev, (eval1, eval2) => s"$mathUtils.${exactMathMethod.get}($eval1, $eval2)")
// byte and short are casted into int when add, minus, times or divide
+ case ByteType | ShortType if failOnError =>
+ val opName = symbol match {
+ case "+" => "Add"
+ case "-" => "Subtract"
+ case "*" => "Multiply"
+ case _ =>
+ throw QueryExecutionErrors.notOverrideExpectedMethodsError(this.getClass.getName,
+ s"genCode for Byte/Short with symbol '$symbol'", "genCode")
Review Comment:
Done — switched to `SparkException.internalError(s"Unexpected symbol '$symbol' for Byte/Short BinaryArithmetic")`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##########
@@ -458,6 +449,10 @@ case class Add(
MathUtils.addExact(input1.asInstanceOf[Int], input2.asInstanceOf[Int], getContextOrNull())
case _: LongType if failOnError =>
MathUtils.addExact(input1.asInstanceOf[Long], input2.asInstanceOf[Long], getContextOrNull())
+ case _: ByteType if failOnError =>
+ ArithmeticUtils.byteAddExact(input1.asInstanceOf[Byte], input2.asInstanceOf[Byte])
+ case _: ShortType if failOnError =>
+ ArithmeticUtils.shortAddExact(input1.asInstanceOf[Short], input2.asInstanceOf[Short])
Review Comment:
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.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##########
@@ -555,6 +550,10 @@ case class Subtract(
input1.asInstanceOf[Long],
input2.asInstanceOf[Long],
getContextOrNull())
+ case _: ByteType if failOnError =>
+ ArithmeticUtils.byteSubtractExact(input1.asInstanceOf[Byte], input2.asInstanceOf[Byte])
+ case _: ShortType if failOnError =>
+ ArithmeticUtils.shortSubtractExact(input1.asInstanceOf[Short], input2.asInstanceOf[Short])
Review Comment:
Reverted — `Subtract.nullSafeEval` now falls through to `numeric.minus` for byte/short.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##########
@@ -625,6 +624,10 @@ case class Multiply(
input1.asInstanceOf[Long],
input2.asInstanceOf[Long],
getContextOrNull())
+ case _: ByteType if failOnError =>
+ ArithmeticUtils.byteMultiplyExact(input1.asInstanceOf[Byte], input2.asInstanceOf[Byte])
+ case _: ShortType if failOnError =>
+ ArithmeticUtils.shortMultiplyExact(input1.asInstanceOf[Short], input2.asInstanceOf[Short])
Review Comment:
Reverted — `Multiply.nullSafeEval` now falls through to `numeric.times` for byte/short.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56913][SQL] Simplify BinaryArithmetic byte/short codegen under ANSI mode [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #55938:
URL: https://github.com/apache/spark/pull/55938#issuecomment-4508762375
thanks , merging to master/4.x!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org