fix: native ansi-errors raised as spark error

[peterxcli]

Which issue does this PR close?

Closes #5072.

Rationale for this change

Several native ANSI decimal paths returned plain Arrow errors. Those errors bypassed Comet's structured Spark error conversion and surfaced as generic exceptions without Spark's error class, SQLSTATE, or query context.

What changes are included in this PR?

• Return typed SparkError variants for wide-decimal overflow, decimal division by zero, integral division overflow, and decimal-to-decimal cast overflow.
• Preserve query context when CheckOverflow is skipped or when an outer Cast evaluates a failing child expression.
• Remove the decimal-cast raw Arrow error accommodation and add Rust and Spark regressions that compare the Spark error class, SQLSTATE, and query context.

How are these changes tested?

• cd native && cargo test -p datafusion-comet-spark-expr returns_spark_error
• cd native && cargo fmt --all -- --check
• make core
• Spark 4.1.2 with JDK 17: focused CometExpressionSuite tests for ANSI decimal division by zero and wide-decimal overflow, plus the CometCastSuite decimal precision/scale cast test.
• ./mvnw spotless:check

[andygrove]

Thanks for taking this on. I traced through why each piece of the fix is needed and the design is coherent. For a / b the context comes from the injected CheckOverflow, whose Origin is inherited through transformUp in DecimalPrecision.promote, and for a div b it comes from the outer Cast, because CometIntegralDivide builds its inner CheckOverflow proto by hand without an expr_id. That explains why both the planner change and the Cast::evaluate change are required. Deleting the castTest special case and still passing the Spark 3.4/3.5 exact-message assertion is good evidence the value formatting is right for casts. Nice touch keeping the value scan on the error path only.

A few things I would like to work through before merge.

The fused rescale path is still a raw Arrow error

DecimalRescaleCheckOverflow still raises ArrowError::ComputeError on both overflow branches when fail_on_error is set, and it takes no query_context at all:

• native/spark-expr/src/math_funcs/internal/decimal_rescale_check.rs:118 ("Decimal overflow during rescale")
• native/spark-expr/src/math_funcs/internal/decimal_rescale_check.rs:138 ("Decimal overflow: value does not fit in precision")

That looks like the same bug class this PR is fixing everywhere else. The reason it matters for this PR in particular is that planner.rs:531 fuses CheckOverflow(Cast(Decimal128 -> Decimal128)) into that expression, and the fusion check runs before the new cast_array arm can ever be reached. So if that shape is reachable, the decimal-to-decimal cast path from #5072 is only half fixed.

Do you know whether that pattern is reachable on Spark 3.4+? DecimalPrecision.promote only ever wraps a BinaryArithmetic, and arithmetic.scala only ever wraps a divide, so I could not convince myself either way. If it is reachable it would be good to cover it here. If it is not, a tracking issue plus a note on the fusion would be great so this does not get lost.

The reported value diverges from Spark for wide-decimal overflow

At native/spark-expr/src/math_funcs/wide_decimal_binary_expr.rs:353 the value handed to NumericValueOutOfRange is the i256 intermediate after rescaling to s_out, formatted as plain digits. Spark goes through Decimal.$times, which multiplies with MATH_CONTEXT (MathContext(38, HALF_UP)), and the error carries Decimal.toString of the pre-rounding value.

For the repro in #5072, 11000000000000000000 * 11000000000000000000, I believe that means Spark reports 1.2100000000000000000000000000000000000E+38 and Comet reports 121000000000000000000000000000000000000. Since value is substituted into the error-class template on the JVM side, it ends up in the user-facing message. The new test only compares error class and SQLSTATE, so the difference does not show up.

Could you confirm what Spark actually prints for that query? If they do diverge, a comment at the call site plus a note in the test explaining why we only compare class and SQLSTATE would help the next reader, and a tracking issue for the message text would be good.

Query context is looked up and then dropped where it originates

create_binary_expr resolves query_context at native/core/src/execution/planner.rs:899, but the wide-decimal arm at line 936 and the decimal-divide arm at line 962 both drop it. Only the integer and float arm wraps in CheckedBinaryExpr. So the fix relies on an ancestor CheckOverflow or Cast carrying the same context, which is a coupling that is hard to see from either side.

Would it be cleaner to wrap WideDecimalBinaryExpr and the decimal_div / decimal_integral_div ScalarFunctionExpr in CheckedBinaryExpr right there, the way the integer arm already does? That would also cover the shapes where no outer CheckOverflow gets emitted, for example when serializeDataType returns None in CometDivide.convert.

Related, at native/core/src/execution/planner.rs:556: with that change, is the wrap on CheckOverflow's child still needed? As written it goes on unconditionally, including when query_context is None, and CheckOverflow still holds its own query_context for its own errors, so we end up with two mechanisms for one job. Gating on query_context.is_some() at minimum would keep the extra node out of plans that cannot use it.

Shared error-unwrap helper

The ArrowError::ExternalError to DataFusionError::External unwrap at native/spark-expr/src/math_funcs/div.rs:186 is identical to the one at native/spark-expr/src/math_funcs/wide_decimal_binary_expr.rs:284. Dropping it buries the SparkError inside DataFusionError::ArrowError and puts us right back where #5072 started. Would you mind pulling it into a small shared helper next to the other error helpers, with a short comment on why the ExternalError hop through try_binary is necessary? Future kernels that need to raise a SparkError from an Arrow closure would then get it right by default.

format_decimal_str

At native/spark-expr/src/math_funcs/wide_decimal_binary_expr.rs:352, passing the digit count as the precision argument is really a way to defeat the truncation that format_decimal_str applies, which took me a while to work out. A comment saying so would help. More generally, now that this helper is shared between conversion_funcs and math_funcs, would it be worth a non-truncating variant, or moving it somewhere neutral? A formatter whose precision parameter silently drops digits is easy to misuse from a call site that does not already know the value fits.

Tests

At native/spark-expr/src/conversion_funcs/cast.rs:342, a couple of cases I would like to see covered. Negative values exercise the sign handling in format_decimal_str. A scale-up overflow goes through make_upscaler rather than make_downscaler, so it is a different code path inside rescale_decimal. It would also be reassuring to have a case where the offending value is not the first row and there are nulls ahead of it, to pin the flatten().find() behavior.

At native/spark-expr/src/math_funcs/wide_decimal_binary_expr.rs:524, would it be worth a Multiply case alongside the Add one? Multiply with s_out < s1 + s2 goes through div_round_half_up before the overflow check, so the value reported in the error comes out of a different branch than the one this test covers.

At spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:52, any reason for comparing getClass.getName against the string rather than pattern matching on SparkArithmeticException directly? The string form will not match a subclass and will fail quietly if the class ever moves. If it is working around a cross-version compile problem, a comment saying which version would be helpful.

One note on test placement, since I checked: CometSqlFileTestSuite's expect_error only does a substring match on the message and cannot assert error class, SQLSTATE, or query context, so the Scala tests are the right call here.

Cast::evaluate

At native/spark-expr/src/conversion_funcs/cast.rs:786, routing the child's error through the context-wrapping block is what makes the a div b test work, since CometIntegralDivide registers the IntegralDivide's context on the outer Cast proto. That is worth a comment, because in the general case this attaches the cast's own fragment to an error raised by a child, where Spark would report the child's fragment. It only bites when the child raises a bare SparkError with no context of its own, which is a case where today we report nothing at all, so it is an improvement either way. A note saying it is deliberate would stop someone reading it as a bug later.

[peterxcli]

1: The fused rescale path is still a raw Arrow error

Changed DecimalRescaleCheckOverflow to raise SparkError::NumericValueOutOfRange, unwrap Arrow external errors, and propagate query context. Large scale deltas now preserve zero/null behavior and return typed ANSI errors. Added a note that current Spark 3.4–4.2 serialization does not reach this fusion.

2: The reported value diverges from Spark for wide-decimal overflow

Confirmed the message-value difference and documented it at the Rust call site and in the Scala test. The test intentionally compares error class, SQLSTATE, and query context rather than the differing value parameter.
Opened #5211 with the example query and corresponding Spark/Comet messages. Linked it from the Rust call site and Scala regression-test comment.

3: Query context is looked up and then dropped where it originates

Wide-decimal and decimal-division expressions are now wrapped directly when query context exists. The ancestor wrapper remains as a fallback for generated child protos without an expr_id, but context-less and duplicate wrappers are avoided.

4: Shared error-unwrap helper

Added a shared Arrow external-error unwrapping helper and reused it in decimal division, wide-decimal arithmetic, and fused decimal rescaling.

5: format_decimal_str

Removed Comet’s copied formatter and reused Arrow’s public format_decimal_str. Added comments explaining why the actual digit count is passed for already-overflowing values.

6: Tests

Added coverage for negative decimal casts, scale-up overflow, null and valid rows before the offending value, rounded multiplication overflow, fused array/scalar overflow, and large scale deltas.

The SparkArithmeticException class-name match remains because that type is inaccessible outside org.apache.spark in Spark 3.4. Added a comment documenting this cross-version constraint.

7: Cast::evaluate

Added a comment explaining that child errors deliberately inherit the outer Cast query context because CometIntegralDivide creates its inner CheckOverflow without an expression ID.

[andygrove]

Thanks for the thorough follow-up. Everything from the first round landed, and several items went further than I asked. I re-verified the pieces I was unsure about:

• The Arrow dedup is safe. arrow-data-58.4.0/src/decimal.rs:1238 has format_decimal_str delegating to format_decimal_str_internal(.., safe_decimal = true), and that body is character-for-character the function deleted from numeric.rs. No behavior change.
• The new (Decimal128, Decimal128) if Ansi arm in cast_array calls cast_with_options with exactly the same native_cast_options the is_datafusion_spark_compatible fallback used before, so only the error path changed. rescale_decimal is the documented row-level mirror of apply_decimal_cast, and when the scan finds nothing it falls back to error.into(), so a mismatch degrades to the old error rather than producing a wrong one.
• Deleting the castTest decimal special case means cast between decimals with different precision and scale now runs assert(cometMessage == sparkMessage) on 3.4 and 3.5. Only one of the three rows overflows, so the reported value is deterministic across partitions. CometNativeCastSuite is in the [expressions] group in both workflow files and that job passed on 3.4 and 3.5. That is real proof the value formatting matches Spark for the cast path.
• divide_by_zero_error() and integral_divide_overflow_error() both return SparkError, so the ExternalError boxing and the later downcast_ref::<SparkError>() in CheckedBinaryExpr::evaluate line up.
• WideDecimalBinaryExpr is downcast in exactly one place, the CheckOverflow arm, and that arm now looks through the wrapper, so adding CheckedBinaryExpr around it does not break anything else. The double-wrap guards work out too: CometDivide emits CheckOverflow(Divide(...)), the Divide proto carries its own context so create_binary_expr wraps it, and the CheckOverflow arm then sees a CheckedBinaryExpr and leaves it alone.
• The rescale_and_check rework is correct beyond error typing. Scale-up past 10^38 with a zero value gives zero, and scale-down past 10^38 gives zero because i128::MAX is below half the divisor. Both match Spark and both are covered by test_large_scale_delta.

Three things I would still like addressed.

The compatibility guide still says these paths raise CometNativeException

docs/source/user-guide/latest/compatibility/index.md still has this bullet under "ANSI-mode error classes and messages":

Wide-decimal arithmetic overflow, decimal divide-by-zero, and decimal-to-decimal cast overflow raise raw Arrow errors that bypass SparkErrorConverter and surface as CometNativeException rather than SparkArithmeticException with the proper error class and query context (#5072).

This PR closes #5072, so the moment it merges that bullet tells users something that is no longer true. You handled the equivalent case in #5167 by removing the #5073 bullet, so the same treatment fits here.

I would rewrite rather than delete, though. Divide-by-zero and decimal-to-decimal cast overflow are fully resolved, but wide-decimal overflow still reports a different value in the message per #5211. That section's intro explicitly covers "message text", so a narrowed bullet pointing at #5211 would keep the remaining divergence discoverable. Would you mind updating it?

The query context assertions never compare against Spark

spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:236 and the wide-decimal test at line 1450 both assert actual.getQueryContext.exists(_.fragment().contains(...)). They compute expected from Spark but only use it for the error class and SQLSTATE, so nothing checks that Comet's context matches Spark's.

Query context preservation is one of the two things this PR fixes, and it is the part with the most room to drift. Could the tests compare actual.getQueryContext.map(_.fragment) against expected.getQueryContext.map(_.fragment) instead? That would directly pin the a div b case where the context comes from the outer synthesized Cast rather than from the IntegralDivide itself, which is exactly the coupling the Cast::evaluate comment describes. If the fragments do not match for a div b, that is worth knowing before merge rather than after.

arithmeticError is another copy of the traversal #5223 is meant to remove

CometExpressionSuite.scala:50 adds a sixth cause-chain walk alongside the five you inventoried in #5223. That inventory is what a future dedup PR will work from, so a call site added after the issue was filed will get missed. Could you add this one to #5223 so it does not drift?

One note for #5211 rather than for this PR

I agree #5211 is the right home for the value divergence and I am not asking you to fix it here. Worth capturing in the issue though: matching Spark is not just a matter of formatting at the natural scale instead of s_out. Spark's Decimal.$times multiplies with MATH_CONTEXT, which is MathContext(38) with HALF_UP, so the value in Spark's error has already been rounded to 38 significant digits before toPrecision is attempted. Your repro happens to round exactly because 1.21e38 has trailing zeros, but for a product with non-zero low digits Comet's exact i256 would carry more significant digits than Spark reports even after a natural-scale fix. Could you add that to the issue so whoever picks it up does not assume it is a one-line change?

[peterxcli]

@andygrove thanks for another round of review, addressed your review. PTAL!

1. Compatibility guide still says these paths raise CometNativeException

Updated compatibility/index.md to retain only the remaining wide-decimal message-value divergence tracked by #5211.

2. Query-context assertions never compare against Spark

Changed both regressions to compare Spark and Comet fragments exactly in CometExpressionSuite.scala.

This exposed and fixed a serializer bug in QueryPlanSerde.scala: passthrough Alias serialization was overwriting the child’s precise context with the full SQL statement.

3. Add arithmeticError to Deduplicate Throwable cause-chain traversal in Comet tests #5223

#5223: added CometExpressionSuite.arithmeticError; inventory now says six call sites across five suites.

4. Document MathContext(38, HALF_UP) in Wide-decimal overflow reports a different value than Spark #5211

#5211: added Spark’s MathContext(38, HALF_UP) rounding behavior and the non-zero-low-digit implication.