fix: use Spark type names in ANSI abs overflow errors - #5357
Conversation
4877eb6 to
7935b32
Compare
sunchao
left a comment
There was a problem hiding this comment.
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.
`abs` passed Arrow type names as `from_type`, so an ANSI overflow surfaced as `ARITHMETIC_OVERFLOW` with `"Int64 overflow"` where Spark says `"long overflow"`. Spark's `Abs` negates through `MathUtils.negateExact`. For int and long that reports the JDK `ArithmeticException` text on every supported version, so `"integer overflow"` and `"long overflow"` are exact on 3.4, 3.5 and 4.x. Byte and short match 4.x only, since 3.4 and 3.5 route those two widths to `QueryExecutionErrors.unaryMinusCauseOverflowError`, which raises `_LEGACY_ERROR_TEMP_2043` instead. 4.0 sends all four widths through `MathUtils.negateExact`. `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"`. The int and long cases now assert the full Spark message. Byte and short stay loose, because `expect_error` checks the pattern against Spark's message too and 3.4 and 3.5 genuinely word those differently. 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. `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>
7935b32 to
1ba4e32
Compare
|
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. |
Which issue does this PR close?
Closes #5356.
Rationale for this change
Under ANSI mode,
abson an integer minimum raisesARITHMETIC_OVERFLOW. Comet'sSpark error shim turns the native
fromTypefield into Spark's arithmetic-overflowmessage by passing
fromType + " overflow"toQueryExecutionErrors.arithmeticOverflowError.native/spark-expr/src/math_funcs/abs.rswas passing Arrow-style type names as
fromType, hard-coded at each arm, so Cometreported
Int64 overflowwhere Spark reportslong overflow, andInt32 overflowwhere Spark reports
integer overflow.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_typestrings inabs.rs, four on the array path throughansi_compute_op!andfour on the scalar path, now carry Spark's name instead of Arrow's:
Int8"Int8""byte"Int16"Int16""short"Int32"Int32""integer"Int64"Int64""long"The mapping is not uniform across the supported Spark versions. Spark's
Absnegatesthrough
MathUtils.negateExact. For int and long that surfaces the JDKArithmeticExceptiontext, which isinteger overflowandlong overflowon 3.4, 3.5and 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 and4.1.2 jars; the same code paths remain in the currently supported 3.5.9 and 4.1.3
sources.
Decimal128andDecimal256are deliberately left alone. Those guards fire only ati128::MINandi256::MIN, which no Spark decimal reaches at its maximum precision of 38, and decimal overflowgoes through a different Spark error class. That belongs in its own change.
How are these changes tested?
abs_ansi.sql. The fixture already ranabs(v)over a column of each width at its minimum, butevery assertion was
expect_error(overflow), which"Int64 overflow"satisfies exactly as happilyas
"long overflow". It could not fail on this bug. The int and long cases now assert the fullmessage,
expect_error(integer overflow)andexpect_error(long overflow).Byte and short stay on the loose pattern, and the fixture says why in a comment:
ExpectErrorasserts the pattern against Spark's message as well as Comet's, and on 3.4 and 3.5 Spark genuinely
words those two differently.
ExpectErrorruns Spark and Comet separately and requires both errors to contain thepattern. 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.CometSqlFileTestSuiteon Spark 3.5 / Scala 2.12 goes from 444 run, 444succeeded 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 ofthe assertion.
Rust. The ANSI branches had no error assertions at all, so this adds two tests:
test_ansi_abs_min_uses_spark_type_namesdowncasts toSparkError::ArithmeticOverflowandasserts
from_typefor all four widths on both the array and the scalar path. Asserting thevariant and the field rather than a substring of the rendered string.
test_ansi_abs_just_inside_boundary_succeedschecksMIN + 1for each width still returnsMAX,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 atthe first case in the table, so what is proven red on
mainis the byte case in Rust and the intcase at the Spark level.
Also run on this branch: the full
datafusion-comet-spark-exprlib suite,cargo fmt --all -- --check, andcargo 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.