andygrove commented on code in PR #5162:
URL: https://github.com/apache/datafusion-comet/pull/5162#discussion_r3723066439
##########
native/spark-expr/src/math_funcs/negative.rs:
##########
@@ -107,57 +97,55 @@ 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))
+ // The shims render this as `{from_type} overflow` under
+ // `ARITHMETIC_OVERFLOW`. For byte/short that is
byte-identical to Spark
+ // 4.x, which routes them through `MathUtils.negateExact`
("byte overflow" /
+ // "short overflow"). Spark 3.4/3.5 instead throw
+ // `_LEGACY_ERROR_TEMP_2043` ("- <sqlValue> caused
overflow."); that error
+ // class is out of reach here, so no string can match every
version.
+ let from_type = match array.data_type() {
+ DataType::Int8 => "byte",
+ DataType::Int16 => "short",
+ DataType::Int32 => "integer",
+ DataType::Int64 => "long",
+ // `neg` checks each `DayTime` component, so either `days`
or `ms` at
+ // `i32::MIN` overflows -- the same failure set as before.
The pre-scan
+ // this replaced only matched the whole
`IntervalDayTime::MIN`, but the
+ // single-component values then overflowed inside
`neg_wrapping`
+ // anyway; routing them here only maps the raw Arrow error
onto Spark's,
+ // and matches the scalar path below.
Review Comment:
Some of the comments seem to refer to the previous implementation - seems
like info that is not useful once this merges?
--
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]
