andygrove commented on code in PR #5084:
URL: https://github.com/apache/datafusion-comet/pull/5084#discussion_r3677786457
##########
native/common/src/error.rs:
##########
@@ -69,7 +69,7 @@ pub enum SparkError {
#[error("[ARITHMETIC_OVERFLOW] {from_type} overflow. If necessary set
\"spark.sql.ansi.enabled\" to \"false\" to bypass this error.")]
ArithmeticOverflow { from_type: String },
- #[error("[ARITHMETIC_OVERFLOW] Overflow in integral divide. Use
`try_divide` to tolerate overflow and return NULL instead. If necessary set
\"spark.sql.ansi.enabled\" to \"false\" to bypass this error.")]
+ #[error("[ARITHMETIC_OVERFLOW] Overflow in integral divide. Use
'try_divide' to tolerate overflow and return NULL instead. If necessary set
\"spark.sql.ansi.enabled\" to \"false\" to bypass this error.")]
Review Comment:
Good catch to question it — it does look inconsistent with the
`DIVIDE_BY_ZERO` variant a few lines below, but the single quote (plain ASCII
`U+0027`) is intentional here. The inconsistency is Spark's, not ours.
The two messages are built by different mechanisms:
**`DIVIDE_BY_ZERO`** has the hint hardcoded in the message template in
`error-classes.json` / `error-conditions.json`, with backticks:
```
"DIVIDE_BY_ZERO" : {
"message" : [
"Division by zero. Use `try_divide` to tolerate divisor being 0 and
return NULL instead. If necessary set <config> to \"false\" to bypass this
error."
],
```
**`ARITHMETIC_OVERFLOW`** has no hardcoded hint — the template just
interpolates an `<alternative>` parameter:
```
"ARITHMETIC_OVERFLOW" : {
"message" : [
"<message>.<alternative> If necessary set <config> to \"false\" to
bypass this error."
],
```
and `<alternative>` is filled in from Scala, which uses a single quote:
```scala
val alternative = if (hint.nonEmpty) {
s" Use '$hint' to tolerate overflow and return NULL instead."
} else ""
```
(`QueryExecutionErrors.arithmeticOverflowError` in Spark 3.4/3.5, moved to
`ExecutionErrors` in `sql/api` for Spark 4.x.)
I confirmed this by dumping the string constants out of the compiled Spark
jars for 3.4.3, 3.5.8, 4.0.1, 4.1.1 and 4.2.0 — all five yield `` Use ' `` / ``
' to tolerate overflow and return NULL instead. ``, i.e. `U+0027` on both
sides, with no version drift. So the backticks were the bug and this line now
matches Spark exactly.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
