andygrove commented on code in PR #5162:
URL: https://github.com/apache/datafusion-comet/pull/5162#discussion_r3690870327
##########
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:
Nit, not blocking. Worth knowing that these two strings cannot match every
supported Spark version, because Spark changed both the wording and the error
class here:
- Spark 3.4 and 3.5 throw
`QueryExecutionErrors.unaryMinusCauseOverflowError`, which is error class
`_LEGACY_ERROR_TEMP_2043` with the template `- <sqlValue> caused overflow.`, so
the message is `[_LEGACY_ERROR_TEMP_2043] - -128 caused overflow.`
- Spark 4.0 onward routes byte and short through `MathUtils.negateExact`,
which throws `arithmeticOverflowError("byte overflow")`, so the message is
`[ARITHMETIC_OVERFLOW] byte overflow. If necessary set "spark.sql.ansi.enabled"
to "false" to bypass this error.`
`SparkError::ArithmeticOverflow` always renders as `ARITHMETIC_OVERFLOW`,
since the shims build the message as `fromType + " overflow"`, so the 3.x class
is out of reach no matter what string goes here. That makes `"byte"` and
`"short"` the better choice: byte-identical to Spark 4.x, and no worse than the
current values on 3.x. It also drops the need to hardcode the value, and the
`-128` in the comment above.
If you take it, the matching test expectations move to `"byte"` / `"short"`,
and the `dtype = ""` placeholders for the byte and short cases in
`CometExpressionSuite`'s `unary negative integer overflow test` could become
real assertions under `isSpark40Plus`. Happy for this to land as is either way.
--
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]
