0lai0 commented on PR #6732:
URL:
https://github.com/apache/datafusion-comet/pull/6732#issuecomment-6085371113
Hi @DeviousCardi, I had opened #6764 for the same issue and am closing it in
favor of this one. A few points from @comphead's review there that likely apply
here too:
1. #6069 (approved) adds a fourth ANSI site in `sum_int.rs`,
`SlidingSumIntegerAccumulator::evaluate`, which uses
`arithmetic_overflow_error("integer")`. Once both land, that site should report
`long overflow` with `try_add` too, since Spark recomputes sliding frames
through the same `Add`. Whichever PR lands second needs to update it, or it may
fail to compile if the import is removed.
2. #6661 asks the Scala test to compare the message parameters with Spark's.
`checkSparkError` only compares the error class, exception class and SQLSTATE.
In #6764 I added a `compareMessageParameters` flag to `checkSparkError` for
this, and checked that it fails when `from_type` is `integer` but `try_add` is
kept.
3. To force the final-merge path in the Scala test, Spark can pack two small
Parquet files into one partition. Setting `spark.sql.files.openCostInBytes` to
the max split size and asserting the scan's partition count keeps the merge
case honest.
Thanks for picking this up!
--
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]
