Smallfu666 opened a new pull request, #5357:
URL: https://github.com/apache/datafusion-comet/pull/5357
## Which issue does this PR close?
Closes #5356.
## Rationale for this change
Under ANSI mode, `abs` on an integer minimum raises `ARITHMETIC_OVERFLOW`,
whose template renders
`<message> overflow`. `native/spark-expr/src/math_funcs/abs.rs` was passing
Arrow-style type names
as that message, hard-coded at each arm, so Comet reported `Int64 overflow`
where Spark reports
`long overflow`, and `Int32 overflow` where Spark reports `integer overflow`.
```sql
SET spark.sql.ansi.enabled=true;
SELECT abs(v) FROM t; -- v BIGINT, single row -9223372036854775808
```
Spark: `[ARITHMETIC_OVERFLOW] long overflow.`
Comet before this PR: same error class, `Int64 overflow`.
The message is what the user sees in the exception, and it is what any
assertion on the error
parameters compares against.
## What changes are included in this PR?
The eight `from_type` strings in `abs.rs`, four on the array path through
`ansi_compute_op!` and
four on the scalar path, now carry Spark's name instead of Arrow's:
| Arrow | was | now |
| --- | --- | --- |
| `Int8` | `"Int8"` | `"byte"` |
| `Int16` | `"Int16"` | `"short"` |
| `Int32` | `"Int32"` | `"integer"` |
| `Int64` | `"Int64"` | `"long"` |
The mapping is not uniform across the supported Spark versions, so a three
line comment on
`ansi_compute_op!` records which strings are exact where. Spark's `Abs`
negates through
`MathUtils.negateExact`. For int and long that surfaces the JDK
`ArithmeticException` text, which is
`integer overflow` and `long overflow` on 3.4, 3.5 and 4.x alike. Byte and
short match 4.x only:
3.4 and 3.5 route those two widths to
`QueryExecutionErrors.unaryMinusCauseOverflowError`, raising
`_LEGACY_ERROR_TEMP_2043` (`- <sqlValue> caused overflow.`), a different
error class, so no single
string can satisfy every version. 4.0 sends all four widths through
`MathUtils.negateExact`. I
verified this against the 3.4.3, 3.5.8, 4.0.2 and 4.1.2 jars.
`Decimal128` and `Decimal256` are deliberately left alone. Those guards fire
only at `i128::MIN` and
`i256::MIN`, which no Spark decimal reaches at its maximum precision of 38,
and decimal overflow
goes through a different Spark error class. That belongs in its own change.
## How are these changes tested?
**`abs_ansi.sql`.** The fixture already ran `abs(v)` over a column of each
width at its minimum, but
every assertion was `expect_error(overflow)`, which `"Int64 overflow"`
satisfies exactly as happily
as `"long overflow"`. It could not fail on this bug. The int and long cases
now assert the full
message, `expect_error(integer overflow)` and `expect_error(long overflow)`.
Byte and short stay on the loose pattern, and the fixture says why in a
comment: `ExpectError`
asserts the pattern against Spark's message as well as Comet's, and on 3.4
and 3.5 Spark genuinely
words those two differently.
`ExpectError` runs both engines through `checkSparkAnswerMaybeThrows` and
requires both to throw, so
the column cases exercise the Comet path rather than a constant fold.
Negative control at the Spark level, production reverted and the fixture
kept:
`org.apache.comet.CometSqlFileTestSuite` on Spark 3.5 / Scala 2.12 goes from
**444 run, 444
succeeded** to **444 run, 443 succeeded, 1 failed**, the failure being
`expressions/math/abs_ansi.sql:57`, `SELECT abs(v) FROM ansi_test_abs_int`,
`does not contain
'integer overflow'`. Spark is identical between the two runs, so that
failure is the Comet side of
the assertion.
**Rust.** The ANSI branches had no error assertions at all, so this adds two
tests:
- `test_ansi_abs_min_uses_spark_type_names` downcasts to
`SparkError::ArithmeticOverflow` and
asserts `from_type` for all four widths on both the array and the scalar
path. Asserting the
variant and the field rather than a substring of the rendered string.
- `test_ansi_abs_just_inside_boundary_succeeds` checks `MIN + 1` for each
width still returns `MAX`,
so the guard is not over-broad.
Negative control on the unpatched production code with these tests kept:
**18 passed, 1 failed**,
the failure being `assertion left == right failed, left: "Int8", right:
"byte"`. Note it stops at
the first case in the table, so what is proven red on `main` is the byte
case in Rust and the int
case at the Spark level.
Also run on this branch: the full `datafusion-comet-spark-expr` lib suite,
`cargo fmt --all --
--check`, and `cargo clippy -p datafusion-comet-spark-expr --all-targets --
-D warnings`.
## Note for reviewers
TINYINT and SMALLINT have no Spark-level regression test here, only the Rust
one, for the version
reason above. If you would rather see them covered, the fixture would need
to gate the assertion on
the Spark version, which felt like more machinery than the case is worth.
--
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]
