Smallfu666 commented on code in PR #5162:
URL: https://github.com/apache/datafusion-comet/pull/5162#discussion_r3692339352
##########
native/spark-expr/src/math_funcs/negative.rs:
##########
@@ -107,57 +97,51 @@ impl PhysicalExpr for NegativeExpr {
fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let arg = self.arg.evaluate(batch)?;
- // overflow checks only apply in ANSI mode
- // datatypes supported are byte, short, integer, long, float, interval
+ // Overflow checks only apply in ANSI mode, and only the types listed
in the
+ // match below have a Spark overflow message. Everything else (float,
decimal,
+ // `Interval(MonthDayNano)`, ...) falls through to `neg_wrapping`.
match arg {
ColumnarValue::Array(array) => {
- if self.fail_on_error {
- match array.data_type() {
- DataType::Int8 => {
- check_overflow!(array, arrow::array::Int8Array,
i8::MIN, "byte")
- }
- DataType::Int16 => {
- check_overflow!(array, arrow::array::Int16Array,
i16::MIN, "short")
- }
- DataType::Int32 => {
- check_overflow!(array, arrow::array::Int32Array,
i32::MIN, "integer")
- }
- DataType::Int64 => {
- check_overflow!(array, arrow::array::Int64Array,
i64::MIN, "long")
- }
- DataType::Interval(value) => match value {
- arrow::datatypes::IntervalUnit::YearMonth =>
check_overflow!(
- array,
- arrow::array::IntervalYearMonthArray,
- i32::MIN,
- "interval"
- ),
- arrow::datatypes::IntervalUnit::DayTime =>
check_overflow!(
- array,
- arrow::array::IntervalDayTimeArray,
- IntervalDayTime::MIN,
- "interval"
- ),
- arrow::datatypes::IntervalUnit::MonthDayNano => {
- // Overflow checks are not supported
- }
- },
- _ => {
- // Overflow checks are not supported for other
datatypes
- }
- }
+ if !self.fail_on_error {
+ return
Ok(ColumnarValue::Array(neg_wrapping(array.as_ref())?));
}
- let result = neg_wrapping(array.as_ref())?;
- Ok(ColumnarValue::Array(result))
+ // Checked negation of `iN` overflows only at `iN::MIN`, so
the value
+ // Spark reports for byte/short is fixed and can be hardcoded
here.
+ let from_type = match array.data_type() {
+ DataType::Int8 => "-128 caused",
+ DataType::Int16 => "-32768 caused",
Review Comment:
Took it — `6ee228bb8`. Both the array and scalar paths now pass `"byte"` /
`"short"`, which also drops the hardcoded value.
You're right that no single string satisfies every version. I checked the
4.x side directly: in `spark-sql-api_2.13-4.0.2.jar`,
`MathUtils.negateExact(byte)` uses the literal `"byte overflow"`, so with the
shims' `fromType + " overflow"` the message is byte-identical there. On 3.4/3.5
the legacy `_LEGACY_ERROR_TEMP_2043` formatting stays different — the
description now says that explicitly rather than leaving it implied.
I took the second half of your suggestion too: the byte/short `dtype`
placeholders in `unary negative integer overflow test` are real assertions
under `isSpark40Plus` now, for both the array cases and the scalar `cast(… as
byte/short)` ones. That adds `CometExpressionSuite.scala` to the PR. It's 143
tests / 0 failures on both `-Pspark-3.5 -Pscala-2.12` and `-Pspark-4.0
-Pscala-2.13`.
--
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]
