From 940a0d0bc312907ffbc6264b38f9096f04ecf816 Mon Sep 17 00:00:00 2001 From: Nick Fu Date: Mon, 10 Aug 2026 12:40:33 +0800 Subject: [PATCH] fix: use Spark type names in ANSI abs overflow errors `abs` passed Arrow type names as `from_type`, so an ANSI overflow surfaced as `ARITHMETIC_OVERFLOW` with `"Int64 overflow"` where Spark says `"long overflow"`. `"Int64 overflow"` matches no Spark version, so using the Spark SQL type names is an improvement everywhere, but it is only exact parity on some versions. Spark's wording for this error is not stable across the versions Comet supports. 3.4 and 3.5 route byte and short to `QueryExecutionErrors.unaryMinusCauseOverflowError` and raise `_LEGACY_ERROR_TEMP_2043`, an error class Comet cannot emit. 4.0 and 4.1 send all four widths through `MathUtils.negateExact` and report the JDK `ArithmeticException` text, so `"integer overflow"` and `"long overflow"` are exact there. 4.2 drops the type name and reports a bare `"overflow"`, so Comet remains more specific than Spark on that version. `abs_ansi.sql` already covered all four widths over column inputs, but asserted only the substring `overflow`, which `"Int64 overflow"` satisfies just as well as `"long overflow"`. Because `expect_error` checks the pattern against Spark's message as well as Comet's, and the version markers are file level rather than per query, the fixture is split the way `to_unix_timestamp_ansi_spark34.sql` is: `abs_ansi.sql` carries `MaxSparkVersion: 4.1` and asserts the exact int and long messages, and `abs_ansi_spark42.sql` carries `MinSparkVersion: 4.2` and asserts the loose pattern. Byte and short stay loose on every version for the 3.4/3.5 reason above. The ANSI branches had no Rust-level error assertions either, so this adds one covering the array and scalar paths for all four widths, plus the nearest valid input to each boundary. Those assert the exact Comet type names directly, so that coverage does not depend on the SQL patterns. `Decimal128` and `Decimal256` are left alone. Those guards only fire at `i128::MIN` and `i256::MIN`, which no Spark decimal reaches at its maximum precision of 38. Co-Authored-By: Claude Opus 5 (1M context) --- native/spark-expr/src/math_funcs/abs.rs | 99 +++++++++++++++-- .../sql-tests/expressions/math/abs_ansi.sql | 17 ++- .../expressions/math/abs_ansi_spark42.sql | 103 ++++++++++++++++++ 3 files changed, 207 insertions(+), 12 deletions(-) create mode 100644 spark/src/test/resources/sql-tests/expressions/math/abs_ansi_spark42.sql diff --git a/native/spark-expr/src/math_funcs/abs.rs b/native/spark-expr/src/math_funcs/abs.rs index 5a16398ec49..7b79a15727b 100644 --- a/native/spark-expr/src/math_funcs/abs.rs +++ b/native/spark-expr/src/math_funcs/abs.rs @@ -38,6 +38,7 @@ macro_rules! legacy_compute_op { }}; } +/// `$FROM_TYPE` is the Spark SQL type name carried in the `ArithmeticOverflow` payload. macro_rules! ansi_compute_op { ($ARRAY:expr, $FUNC:ident, $TYPE:ident, $RESULT:ident, $NATIVE:ident, $FROM_TYPE:expr) => {{ let n = $ARRAY.as_any().downcast_ref::<$TYPE>(); @@ -95,7 +96,7 @@ pub fn abs(args: &[ColumnarValue]) -> Result { let result = legacy_compute_op!(array, wrapping_abs, Int8Array, Int8Array); Ok(ColumnarValue::Array(Arc::new(result?))) } else { - ansi_compute_op!(array, abs, Int8Array, Int8Type, i8, "Int8") + ansi_compute_op!(array, abs, Int8Array, Int8Type, i8, "byte") } } DataType::Int16 => { @@ -103,7 +104,7 @@ pub fn abs(args: &[ColumnarValue]) -> Result { let result = legacy_compute_op!(array, wrapping_abs, Int16Array, Int16Array); Ok(ColumnarValue::Array(Arc::new(result?))) } else { - ansi_compute_op!(array, abs, Int16Array, Int16Type, i16, "Int16") + ansi_compute_op!(array, abs, Int16Array, Int16Type, i16, "short") } } DataType::Int32 => { @@ -111,7 +112,7 @@ pub fn abs(args: &[ColumnarValue]) -> Result { let result = legacy_compute_op!(array, wrapping_abs, Int32Array, Int32Array); Ok(ColumnarValue::Array(Arc::new(result?))) } else { - ansi_compute_op!(array, abs, Int32Array, Int32Type, i32, "Int32") + ansi_compute_op!(array, abs, Int32Array, Int32Type, i32, "integer") } } DataType::Int64 => { @@ -119,7 +120,7 @@ pub fn abs(args: &[ColumnarValue]) -> Result { let result = legacy_compute_op!(array, wrapping_abs, Int64Array, Int64Array); Ok(ColumnarValue::Array(Arc::new(result?))) } else { - ansi_compute_op!(array, abs, Int64Array, Int64Type, i64, "Int64") + ansi_compute_op!(array, abs, Int64Array, Int64Type, i64, "long") } } DataType::Float32 => { @@ -207,7 +208,7 @@ pub fn abs(args: &[ColumnarValue]) -> Result { // return the original value Ok(ColumnarValue::Scalar(ScalarValue::Int8(Some(*v)))) } else { - Err(arithmetic_overflow_error("Int8").into()) + Err(arithmetic_overflow_error("byte").into()) } } }, @@ -221,7 +222,7 @@ pub fn abs(args: &[ColumnarValue]) -> Result { // return the original value Ok(ColumnarValue::Scalar(ScalarValue::Int16(Some(*v)))) } else { - Err(arithmetic_overflow_error("Int16").into()) + Err(arithmetic_overflow_error("short").into()) } } }, @@ -235,7 +236,7 @@ pub fn abs(args: &[ColumnarValue]) -> Result { // return the original value Ok(ColumnarValue::Scalar(ScalarValue::Int32(Some(*v)))) } else { - Err(arithmetic_overflow_error("Int32").into()) + Err(arithmetic_overflow_error("integer").into()) } } }, @@ -249,7 +250,7 @@ pub fn abs(args: &[ColumnarValue]) -> Result { // return the original value Ok(ColumnarValue::Scalar(ScalarValue::Int64(Some(*v)))) } else { - Err(arithmetic_overflow_error("Int64").into()) + Err(arithmetic_overflow_error("long").into()) } } }, @@ -312,6 +313,7 @@ pub fn abs(args: &[ColumnarValue]) -> Result { #[cfg(test)] mod tests { use super::*; + use crate::SparkError; use datafusion::common::cast::{ as_decimal128_array, as_decimal256_array, as_float32_array, as_float64_array, as_int16_array, as_int32_array, as_int64_array, as_int8_array, as_uint64_array, @@ -323,6 +325,87 @@ mod tests { } } + fn assert_spark_overflow(err: DataFusionError, expected_from_type: &str) { + if let DataFusionError::External(ref e) = err { + if let Some(SparkError::ArithmeticOverflow { from_type }) = + e.downcast_ref::() + { + assert_eq!(from_type, expected_from_type); + return; + } + } + panic!( + "Expected SparkError::ArithmeticOverflow {{ from_type: {expected_from_type:?} }}, got: {err:?}" + ); + } + + fn abs_ansi(value: ColumnarValue) -> Result { + abs(&[ + value, + ColumnarValue::Scalar(ScalarValue::Boolean(Some(true))), + ]) + } + + /// `abs(MIN)` under ANSI must name the Spark type, so the shims render `"long overflow"` + /// rather than `"Int64 overflow"`. + #[test] + fn test_ansi_abs_min_uses_spark_type_names() { + let arrays: Vec<(ArrayRef, &str)> = vec![ + (Arc::new(Int8Array::from(vec![i8::MIN])), "byte"), + (Arc::new(Int16Array::from(vec![i16::MIN])), "short"), + (Arc::new(Int32Array::from(vec![i32::MIN])), "integer"), + (Arc::new(Int64Array::from(vec![i64::MIN])), "long"), + ]; + for (array, from_type) in arrays { + assert_spark_overflow( + abs_ansi(ColumnarValue::Array(array)).unwrap_err(), + from_type, + ); + } + + for (scalar, from_type) in [ + (ScalarValue::Int8(Some(i8::MIN)), "byte"), + (ScalarValue::Int16(Some(i16::MIN)), "short"), + (ScalarValue::Int32(Some(i32::MIN)), "integer"), + (ScalarValue::Int64(Some(i64::MIN)), "long"), + ] { + assert_spark_overflow( + abs_ansi(ColumnarValue::Scalar(scalar)).unwrap_err(), + from_type, + ); + } + } + + /// The nearest valid input to each overflow boundary still succeeds, so the guard above is + /// not over-broad. + #[test] + fn test_ansi_abs_just_inside_boundary_succeeds() { + for (scalar, expected) in [ + ( + ScalarValue::Int8(Some(i8::MIN + 1)), + ScalarValue::Int8(Some(i8::MAX)), + ), + ( + ScalarValue::Int16(Some(i16::MIN + 1)), + ScalarValue::Int16(Some(i16::MAX)), + ), + ( + ScalarValue::Int32(Some(i32::MIN + 1)), + ScalarValue::Int32(Some(i32::MAX)), + ), + ( + ScalarValue::Int64(Some(i64::MIN + 1)), + ScalarValue::Int64(Some(i64::MAX)), + ), + ] { + let ColumnarValue::Scalar(result) = abs_ansi(ColumnarValue::Scalar(scalar)).unwrap() + else { + panic!("expected scalar result") + }; + assert_eq!(result, expected); + } + } + // Unsigned types, return as is #[test] fn test_abs_u8_scalar() { diff --git a/spark/src/test/resources/sql-tests/expressions/math/abs_ansi.sql b/spark/src/test/resources/sql-tests/expressions/math/abs_ansi.sql index c89a2958c1e..879b24f3f65 100644 --- a/spark/src/test/resources/sql-tests/expressions/math/abs_ansi.sql +++ b/spark/src/test/resources/sql-tests/expressions/math/abs_ansi.sql @@ -18,6 +18,11 @@ -- ANSI mode abs function tests -- Tests that abs throws exceptions for overflow on minimum integer values +-- Spark 4.2 drops the type name from the abs overflow message and reports a bare +-- "[ARITHMETIC_OVERFLOW] overflow.", so the int and long patterns below cannot hold there. +-- abs_ansi_spark42.sql covers 4.2 and later with the loose pattern. +-- MaxSparkVersion: 4.1 + -- Config: spark.sql.ansi.enabled=true -- ============================================================================ @@ -53,11 +58,11 @@ INSERT INTO ansi_test_abs_byte VALUES (-128) -- ============================================================================ -- abs(-2147483648) cannot be represented as int (since INT_MAX = 2147483647) -query expect_error(overflow) +query expect_error(integer overflow) SELECT abs(v) FROM ansi_test_abs_int -- literal -query expect_error(overflow) +query expect_error(integer overflow) SELECT abs(-2147483648) -- ============================================================================ @@ -65,17 +70,21 @@ SELECT abs(-2147483648) -- ============================================================================ -- abs(-9223372036854775808) cannot be represented as long -query expect_error(overflow) +query expect_error(long overflow) SELECT abs(v) FROM ansi_test_abs_long -- literal -query expect_error(overflow) +query expect_error(long overflow) SELECT abs(-9223372036854775808L) -- ============================================================================ -- abs(SHORT_MIN) overflow -- ============================================================================ +-- Byte and short stay on the loose `overflow` pattern. Spark 4.x reports +-- "byte overflow" / "short overflow", but 3.4 and 3.5 raise +-- _LEGACY_ERROR_TEMP_2043 ("- caused overflow.") instead, and +-- expect_error asserts the pattern against Spark's message as well as Comet's. -- abs(-32768) cannot be represented as short query expect_error(overflow) SELECT abs(v) FROM ansi_test_abs_short diff --git a/spark/src/test/resources/sql-tests/expressions/math/abs_ansi_spark42.sql b/spark/src/test/resources/sql-tests/expressions/math/abs_ansi_spark42.sql new file mode 100644 index 00000000000..6e8ec500940 --- /dev/null +++ b/spark/src/test/resources/sql-tests/expressions/math/abs_ansi_spark42.sql @@ -0,0 +1,103 @@ +-- Licensed to the Apache Software Foundation (ASF) under one +-- or more contributor license agreements. See the NOTICE file +-- distributed with this work for additional information +-- regarding copyright ownership. The ASF licenses this file +-- to you under the Apache License, Version 2.0 (the +-- "License"); you may not use this file except in compliance +-- with the License. You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, +-- software distributed under the License is distributed on an +-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +-- KIND, either express or implied. See the License for the +-- specific language governing permissions and limitations +-- under the License. + +-- ANSI mode abs function tests +-- Tests that abs throws exceptions for overflow on minimum integer values + +-- Spark 4.2 reports a bare "[ARITHMETIC_OVERFLOW] overflow." for abs, with no type name, so every +-- pattern here is the loose "overflow". abs_ansi.sql covers 4.1 and earlier, where int and long +-- carry the type name and are asserted exactly. Comet still emits the Spark type names on every +-- version, which the Rust tests in abs.rs assert directly. +-- MinSparkVersion: 4.2 + +-- Config: spark.sql.ansi.enabled=true + +-- ============================================================================ +-- Test data setup +-- ============================================================================ + +statement +CREATE TABLE ansi_test_abs_int(v int) USING parquet + +statement +INSERT INTO ansi_test_abs_int VALUES (-2147483648) + +statement +CREATE TABLE ansi_test_abs_long(v long) USING parquet + +statement +INSERT INTO ansi_test_abs_long VALUES (-9223372036854775808) + +statement +CREATE TABLE ansi_test_abs_short(v short) USING parquet + +statement +INSERT INTO ansi_test_abs_short VALUES (-32768) + +statement +CREATE TABLE ansi_test_abs_byte(v tinyint) USING parquet + +statement +INSERT INTO ansi_test_abs_byte VALUES (-128) + +-- ============================================================================ +-- abs(INT_MIN) overflow +-- ============================================================================ + +-- abs(-2147483648) cannot be represented as int (since INT_MAX = 2147483647) +query expect_error(overflow) +SELECT abs(v) FROM ansi_test_abs_int + +-- literal +query expect_error(overflow) +SELECT abs(-2147483648) + +-- ============================================================================ +-- abs(LONG_MIN) overflow +-- ============================================================================ + +-- abs(-9223372036854775808) cannot be represented as long +query expect_error(overflow) +SELECT abs(v) FROM ansi_test_abs_long + +-- literal +query expect_error(overflow) +SELECT abs(-9223372036854775808L) + +-- ============================================================================ +-- abs(SHORT_MIN) overflow +-- ============================================================================ + +-- abs(-32768) cannot be represented as short +query expect_error(overflow) +SELECT abs(v) FROM ansi_test_abs_short + +-- literal +query expect_error(overflow) +SELECT abs(cast(-32768 as short)) + +-- ============================================================================ +-- abs(BYTE_MIN) overflow +-- ============================================================================ + +-- abs(-128) cannot be represented as tinyint +query expect_error(overflow) +SELECT abs(v) FROM ansi_test_abs_byte + +-- literal +query expect_error(overflow) +SELECT abs(cast(-128 as tinyint))