comphead commented on code in PR #6764:
URL: https://github.com/apache/datafusion-comet/pull/6764#discussion_r4231743466
##########
spark/src/test/scala/org/apache/comet/exec/CometAggregateSuite.scala:
##########
@@ -3297,36 +3297,49 @@ class CometAggregateSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
(1 to 50).flatMap(_ => Seq((maxDec38_0, 1)))
}
+ /** Spark's integral `SUM` overflow suggests `try_add`, so Comet's error
must too. */
+ private def assertTryAddSuggestion(error: SparkThrowable, clue: String):
Unit = {
+ val alternative = error.getMessageParameters.get("alternative")
+ assert(
+ alternative != null && alternative.contains("'try_add'"),
+ s"$clue -> alternative=$alternative")
Review Comment:
#6661 asks for this test to compare the message parameters with Spark's.
`assertTryAddSuggestion` only checks that Comet's `alternative` mentions
`try_add`, so the Scala test would still pass if `message` came back as
`integer overflow`. Only the Rust tests pin `from_type`. `checkSparkError` in
`CometTestBase` already holds Spark's error as `expected`, and
`CometExpressionSuite` and `CometTemporalExpressionSuite` each hand-roll the
`getMessageParameters` comparison today. Could `checkSparkError` take a flag to
also assert `actual.getMessageParameters == expected.getMessageParameters`?
That checks `message` against Spark on every profile, including 4.2's
`overflow`, without hard-coding the wording.
##########
native/spark-expr/src/agg_funcs/sum_int.rs:
##########
@@ -1017,4 +1020,87 @@ mod tests {
acc.merge_batch(&[states]).unwrap();
assert_eq!(acc.evaluate().unwrap(), ScalarValue::Int64(Some(60)));
}
+
+ /// Spark's integral `SUM` adds through `Add` on `LONG`, so every ANSI
overflow path must
+ /// report a long overflow with the `try_add` suggestion.
+ fn assert_long_add_overflow(error: DataFusionError) {
+ let DataFusionError::External(error) = error else {
+ panic!("Expected structured Spark error, got {error:?}")
+ };
+ match error.downcast_ref::<SparkError>() {
+ Some(SparkError::ArithmeticOverflow {
+ from_type,
+ function_name,
+ }) => {
+ assert_eq!(from_type, "long");
+ assert_eq!(function_name, "try_add");
+ }
+ other => panic!("Expected ArithmeticOverflow, got {other:?}"),
+ }
+ }
+
+ fn int64_array(values: Vec<i64>) -> ArrayRef {
+ Arc::new(Int64Array::from(values))
+ }
+
+ #[test]
+ fn test_ansi_accumulator_update_batch_overflow() {
+ let mut acc = SumIntegerAccumulatorAnsi::new();
+ let error = acc
+ .update_batch(&[int64_array(vec![i64::MAX, 1])])
+ .unwrap_err();
+ assert_long_add_overflow(error);
+ }
+
+ #[test]
+ fn test_ansi_accumulator_merge_batch_overflow() {
+ let mut acc = SumIntegerAccumulatorAnsi::new();
+ acc.merge_batch(&[int64_array(vec![i64::MAX])]).unwrap();
+ let error = acc.merge_batch(&[int64_array(vec![1])]).unwrap_err();
+ assert_long_add_overflow(error);
+ }
+
+ #[test]
+ fn test_ansi_accumulator_update_batch_underflow() {
Review Comment:
These six tests reach three `map_err` sites. Underflow takes the same
`add_checked` branch as overflow, and `SumIntegerAccumulatorAnsi::merge_batch`
only calls `update_batch`. So `test_ansi_accumulator_update_batch_underflow`,
`test_ansi_accumulator_merge_batch_overflow` and
`test_ansi_groups_accumulator_merge_batch_underflow` each repeat a site another
test here already pins, and the Scala test drives both signs through all three
sites. Could this be one test per site?
##########
native/spark-expr/src/agg_funcs/sum_int.rs:
##########
@@ -15,7 +15,7 @@
// specific language governing permissions and limitations
// under the License.
-use crate::{arithmetic_overflow_error, EvalMode};
+use crate::{long_add_overflow_error, EvalMode};
Review Comment:
#6069 is open and approved, and it adds a fourth ANSI site in this file:
`SlidingSumIntegerAccumulator::evaluate` returns
`arithmetic_overflow_error("integer")`. Git merges the two PRs without a
conflict, but this line drops that import, so from reading the merged tree I
expect whichever lands second not to compile. That sliding frame would also
keep reporting `integer overflow` with no `try_add`, because Spark recomputes
each sliding frame through the same `Add`. Could that site use
`long_add_overflow_error()` too, in whichever PR lands second? I have not built
the merge.
--
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]
