sunchao commented on code in PR #6732:
URL: https://github.com/apache/datafusion-comet/pull/6732#discussion_r4214246827
##########
spark/src/test/scala/org/apache/comet/exec/CometAggregateSuite.scala:
##########
@@ -3297,20 +3297,38 @@ class CometAggregateSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
(1 to 50).flatMap(_ => Seq((maxDec38_0, 1)))
}
+ /**
+ * Spark's integral SUM adds through `Add`, so an ANSI overflow is `long
overflow` with the
+ * `try_add` suggestion. Compare the structured error, not just its error
class.
+ */
+ private def assertAnsiSumOverflowMatchesSpark(df: DataFrame): Unit = {
+ val (sparkError, cometError) = checkSparkAnswerMaybeThrows(df)
+ def structured(error: Option[Throwable]): SparkThrowable with Throwable = {
+ val failure = error.getOrElse(fail("Expected SUM overflow in ANSI mode"))
+ causeChain(failure)
+ .collect { case e: SparkThrowable with Throwable => e }
+ .lastOption
+ .getOrElse(fail(s"Expected SparkThrowable: $failure"))
+ }
+ val expected = structured(sparkError)
+ val actual = structured(cometError)
+ assert(expected.getErrorClass == "ARITHMETIC_OVERFLOW")
+ assert(actual.getClass == expected.getClass)
+ assert(actual.getErrorClass == expected.getErrorClass)
+ assert(actual.getSqlState == expected.getSqlState)
+ assert(actual.getMessageParameters == expected.getMessageParameters)
+ assert(actual.getMessage.contains("long overflow"))
Review Comment:
[P2] [P2] Use Spark’s version-specific overflow wording in this test. With
`-Pspark-4.2`, Spark 4.2.0 canonicalizes `long overflow` to `overflow` in
`ExecutionErrors.arithmeticOverflowError`. Comet’s existing converter calls
that same factory, so both exceptions have matching parameters, but this
assertion still fails. Consequently, the updated `ANSI support - SUM function`
test fails on the supported 4.2 profile even when the fix behaves correctly.
Please remove this literal-text assertion and retain the preceding
`getMessageParameters` comparison against Spark.
Evidence: A standalone probe using Spark 4.2.0 jars caught
`MathUtils.addExact(Long.MaxValue, 1L, null)` and compared it with
`QueryExecutionErrors.arithmeticOverflowError("long overflow", "try_add",
null)`, the call used by Comet’s JVM converter. Both returned
`message=overflow` and the same `try_add` alternative. Parameter equality was
true, while `getMessage.contains("long overflow")` was false. Spark’s
normalization is defined in
https://github.com/apache/spark/blob/v4.2.0/sql/api/src/main/scala/org/apache/spark/sql/errors/ExecutionErrors.scala#L118.
This validates the assertion failure without claiming a full Comet JVM-suite
run.
--
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]
