Repository navigation
fix(spark): match Spark abs overflow errors - #24156
Conversation
github-actions
Bot
added
sqllogictest
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24156 +/- ##
==========================================
- Coverage 81.26% 81.25% -0.01%
==========================================
Files 1112 1112
Lines 391578 391574 -4
Branches 391578 391574 -4
==========================================
- Hits 318197 318190 -7
+ Misses 54677 54673 -4
- Partials 18704 18711 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The change looks good overall. I like that the scalar and array paths now use the same Spark 4.2 ARITHMETIC_OVERFLOW error contract, and the SQLLogicTests cover the signed integral types consistently.
I left one small non-blocking suggestion to simplify the array helper.
| let array = downcast_named_arg!(&input, "abs arg", $ARRAY_TYPE); | ||
| let res: $ARRAY_TYPE = array | ||
| .try_unary(|x| x.checked_abs().ok_or_else(arithmetic_overflow_error)) | ||
| .and_then(|v| Ok(v.with_data_type(input.data_type().clone())))?; |
There was a problem hiding this comment.
Small suggestion: could we use .map(|v| v.with_data_type(...))? here instead of .and_then(|v| Ok(v.with_data_type(...)))?? Since with_data_type is infallible, map feels a little simpler and makes the intent clearer.
There was a problem hiding this comment.
@kosiew addressed this. thanks for checkign!
Compare the stable DataFusion error text after stripping optional backtrace output so the test passes with the CI backtrace feature enabled. Signed-off-by: goutamadwant <[email protected]>
434be0b to
40c26c8
Compare
|
thanks for checking @kosiew addressed the comment - let me know if you have any more comments / suggestions. thanks! |
|
Thanks @goutamadwant for working on this 👍 |
Which issue does this PR close?
Rationale for this change
When ANSI mode is enabled,
datafusion-sparkreports type-specific DataFusion errors for integralabsoverflow. The scalar and array paths also produce different messages. Spark 4.2 uses one canonicalARITHMETIC_OVERFLOWmessage for both paths.What changes are included in this PR?
absarray kernel so Spark-specific error text does not change core DataFusion behavior.ARITHMETIC_OVERFLOWmessage for scalar and array overflows.Are these changes tested?
Yes. The following checks pass:
cargo fmt --all -- --checkcargo clippy -p datafusion-spark --all-targets --all-features -- -D warningscargo test -p datafusion-spark --all-featurescargo test --profile=ci --test sqllogictests -- spark/math/abs.sltcargo test --profile=ci --test sqllogictests -- math.sltRUST_BACKTRACE=1 cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-cli --workspace --lib --tests --bins --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryptionAre there any user-facing changes?
Yes. With
datafusion.execution.enable_ansi_mode = true, Sparkabsoverflow errors now use Spark 4.2's canonicalARITHMETIC_OVERFLOWmessage for scalar and array inputs. There are no public API changes.