Skip to content

fix: use Spark type names in ANSI abs overflow errors - #5357

Merged
sunchao merged 1 commit into
apache:mainfrom
Smallfu666:internal/abs-ansi-spark-type-names
Aug 28, 2026
Merged

fix: use Spark type names in ANSI abs overflow errors#5357
sunchao merged 1 commit into
apache:mainfrom
Smallfu666:internal/abs-ansi-spark-type-names

Conversation

@Smallfu666

@Smallfu666 Smallfu666 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #5356.

Rationale for this change

Under ANSI mode, abs on an integer minimum raises ARITHMETIC_OVERFLOW. Comet's
Spark error shim turns the native fromType field into Spark's arithmetic-overflow
message by passing fromType + " overflow" to
QueryExecutionErrors.arithmeticOverflowError. native/spark-expr/src/math_funcs/abs.rs
was passing Arrow-style type names as fromType, hard-coded at each arm, so Comet
reported Int64 overflow where Spark reports long overflow, and Int32 overflow
where Spark reports integer overflow.

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 determines the user-visible Spark exception message.

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. 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 originally reproduced this against 3.4.3, 3.5.8, 4.0.2 and
4.1.2 jars; the same code paths remain in the currently supported 3.5.9 and 4.1.3
sources.

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 Spark and Comet separately and requires both errors to contain the
pattern. It does not by itself prove native coverage. Native reachability for the INT
column case is established by the negative control below: reverting only the production
change while keeping the strengthened fixture makes the Comet run fail on
integer overflow, while Spark is unchanged.

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.

@Smallfu666
Smallfu666 force-pushed the internal/abs-ansi-spark-type-names branch from 4877eb6 to 7935b32 Compare August 14, 2026 07:37
@Smallfu666
Smallfu666 marked this pull request as ready for review August 18, 2026 08:36

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the ANSI abs overflow type names against the canonical Spark 3.5 and 4.0 error paths. I found no P1/P2 issues introduced by this change. A Spark 4.0.4 baseline confirmed the byte, short, integer, and long overflow names.

CI has now started. The rust-test job fails during Clippy 1.98 on chunks_exact_to_as_chunks in spark_bit_array.rs and manual_isolate_lowest_one in mersenne.rs, before tests run. I verified both files are unchanged from this PR's merge base. The full CI matrix has not passed. I did not run the full Comet abs test suite locally.

@Smallfu666
Smallfu666 force-pushed the internal/abs-ansi-spark-type-names branch from 7935b32 to 1ba4e32 Compare August 24, 2026 16:28
@Smallfu666

Copy link
Copy Markdown
Contributor Author

Thanks for the review, and for checking those two files against the merge base.

That clippy failure came from #5400 , which landed after this branch was cut, so I rebased onto current main to pick it up. Workspace clippy is clean locally now.

@Smallfu666
Smallfu666 force-pushed the internal/abs-ansi-spark-type-names branch from 1ba4e32 to ed8e55e Compare August 28, 2026 01:10
`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) <noreply@anthropic.com>
@Smallfu666
Smallfu666 force-pushed the internal/abs-ansi-spark-type-names branch from ed8e55e to 940a0d0 Compare August 28, 2026 04:16
@sunchao
sunchao merged commit 9321f92 into apache:main Aug 28, 2026
71 checks passed
@sunchao

sunchao commented Aug 28, 2026

Copy link
Copy Markdown
Member

Merged, thanks @Smallfu666 for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ANSI abs integer overflow errors use Arrow-style type names instead of Spark's

2 participants