andygrove commented on PR #5162:
URL:
https://github.com/apache/datafusion-comet/pull/5162#issuecomment-5195864399
Thanks for the revision. Two things left.
**1. Drop the empty-string sentinel in `checkOverflow` and fold the version
branch into a helper.**
`CometExpressionSuite.scala:2327-2344`. The comment explaining why a local
`def` cannot take
`= sparkExpected` is a sign the signature is fighting the language. The
version-dependent Spark
string is also written three times, once in the tuple list and once in each
of the byte and short
scalar loops. Both go away with a small helper:
```scala
// Spark 3.4/3.5 throw `_LEGACY_ERROR_TEMP_2043` ("- <sqlValue> caused
overflow.") for byte and
// short. Spark 4.x routes them through `MathUtils.negateExact` and agrees
with Comet, which
// always renders `SparkError::ArithmeticOverflow` with the Spark type name.
def sparkOverflowMsg(dtype: String): String =
if (isSpark40Plus) s"$dtype overflow" else "caused overflow"
def checkOverflow(query: String, sparkExpected: String, cometExpected:
String): Unit = ...
```
Every call site then passes both strings, the byte case reads
`checkOverflow(query, sparkOverflowMsg("byte"), "byte overflow")`, and the
`if (isSpark40Plus)`
branching disappears from all three places.
**2. The float rows never exercise `checkOverflow`.**
`CometExpressionSuite.scala:2423-2425`. Float negation cannot overflow, so
those two queries always
land in the `(None, None)` branch and the `"float overflow"` argument is
never asserted.
`checkSparkAnswerAndOperator` still verifies the result and native
execution, so nothing is wrong,
but the argument implies a check that never happens. Since you just removed
the dead interval block
for the same reason, could these move to a plain
`checkSparkAnswerAndOperator` call, or get a
one-line comment saying they are there to confirm float negation stays
native and does not throw?
--
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]
