fix(spark): derive pmod's decimal result type from the declared arguments - #24409
Open
amitvijapur wants to merge 1 commit into
Open
fix(spark): derive pmod's decimal result type from the declared arguments#24409amitvijapur wants to merge 1 commit into
amitvijapur wants to merge 1 commit into
Conversation
…ents
Spark derives pmod's decimal result with `Pmod.resultDecimalType`, applying
the `Remainder` rule to the declared argument types:
scale = max(s1, s2)
precision = min(p1 - s1, p2 - s2) + scale
`SparkPmod` used `Signature::numeric`, which collapses both arguments to a
common decimal before `return_type` runs. The two precisions it saw were
already equal, so the rule degenerated to the input precision and
`pmod(decimal(3,1), decimal(2,1))` reported `Decimal128(3, 1)` where Spark
reports `decimal(2,1)`.
Leave decimal arguments intact through coercion, as `try_sum` already does,
and apply the rule in `return_type`. Every other argument combination keeps
the coercion `Signature::numeric` performed, null handling included, so only
the decimal pair changes behaviour.
The result type is narrower than the dividend, so the operands cannot be cast
to it up front without overflowing it; `spark_pmod` widens them to a common
computation type instead and narrows the result afterwards. The remainder is
bounded by the divisor rather than by the result type, so that narrowing can
overflow when the divisor is wider than the dividend. Spark wraps decimal
arithmetic in `CheckOverflow(nullOnOverflow = !ansiEnabled)`, so the narrowing
cast returns NULL in legacy mode and raises under ANSI, and the widening cast
raises in either mode because the computation type always fits both operands.
Values that fit the Spark result type are unchanged; values that do not were
previously returned at the wider type and are now NULL or an error, which is
the reported-type bug itself rather than a separate behaviour change.
Github-Issue:apache#23895
Jefffrey
reviewed
Aug 16, 2026
| pub fn new() -> Self { | ||
| Self { | ||
| signature: Signature::numeric(2, Volatility::Immutable), | ||
| signature: Signature::user_defined(Volatility::Immutable), |
Contributor
There was a problem hiding this comment.
would it be valid to do this instead:
signature: Signature::one_of(
vec![
TypeSignature::Coercible(vec![
Coercion::new_exact(TypeSignatureClass::Decimal),
Coercion::new_exact(TypeSignatureClass::Decimal),
]),
TypeSignature::Numeric(2),
],
Volatility::Immutable,
),then in return_type() calculate the widened precision/scale according to spark rules
and then in invoke_with_args() we can retrieve this calculated type via ScalarFunctionArgs::return_type
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
pmodreports a wider decimal type than Spark does. Spark derives the resulttype of
pmodwithPmod.resultDecimalType, which applies theRemainderrule to the declared argument types:
For
pmod(decimal(3,1), decimal(2,1))Spark reportsdecimal(2,1), butDataFusion reported
Decimal128(3, 1).The cause is coercion.
SparkPmodusedSignature::numeric, which collapsesboth arguments to a common decimal before
return_typeruns, so the twoprecisions
return_typesaw were already equal and the rule degenerated to theinput precision.
What changes are included in this PR?
SparkPmodmoves toSignature::user_definedwith acoerce_typesthatleaves a decimal/decimal argument pair intact, following the precedent set by
try_sum. Every other argument combination keeps the coercionSignature::numericperformed — including its null handling, where a nullargument is skipped and an all-null call falls back to
Float64— so only thedecimal pair changes behaviour.
return_typeapplies Spark'sPmod.resultDecimalTyperule for decimalarguments and is unchanged for everything else.
cannot be cast to it before the remainder is taken without overflowing the
dividend —
pmod(99.9::decimal(3,1), 2.5::decimal(2,1))returnsdecimal(2,1), which cannot hold99.9.spark_pmodtherefore widens theoperands to a common computation type, takes the remainder there, and narrows
the result afterwards.
Overflow semantics
The remainder is bounded by the divisor, but the result type only carries
min(p1 - s1, p2 - s2)integer digits, so the narrowing step can overflow whenthe divisor is wider than the dividend:
Spark wraps decimal arithmetic in
CheckOverflow(nullOnOverflow = !ansiEnabled),so the narrowing cast returns NULL in legacy mode and raises under ANSI. The
widening cast uses
safe: falsein both modes, since the computation type ischosen to fit both operands and a silent NULL there would hide a real bug.
Scope
Deliberately limited to
pmodover twoDecimal128arguments, which is what#23895 reports. Three adjacent gaps are left alone and are happy to be follow-ups
if you would rather see them here:
SparkModhas the same bug, sinceRemainder.resultDecimalTypeis thesame rule. It is the easier of the two: arrow's
Op::Remalready computesmin(p1-s1, p2-s2) + max(s1, s2), somodneeds only thecoerce_typespass-through and the matching
return_type, with no widen/narrow step.pmod(2.5::decimal(3,1), 3)reports
Decimal128(21, 1)where Spark casts INT todecimal(10,0)andreports
decimal(3,1).Decimal256,Decimal64andDecimal32pairs fall through to theprevious behaviour. Spark has no equivalent of the wider types.
Are these changes tested?
Yes.
datafusion/sqllogictest/test_files/spark/math/pmod.sltgains:arrow_typeofassertions covering equal scales, differing precisions,differing scales, and the narrowing case;
pmod(99.9::decimal(3,1), 2.5::decimal(2,1)), the case thatwould regress if the operands were narrowed before the remainder;
the ANSI block; and
modulus.rsgains a unit test forpmod_decimal_result_typecovering the ruledirectly, independent of the planner.
The existing
pmodandmodvalue tests are unchanged and still pass. Verifiedlocally:
cargo test -p datafusion-spark --all-features(279 passed), all 244spark/sqllogictest files,cargo clippy --all-targets --all-features -D warnings, andcargo fmt --all --check.Are there any user-facing changes?
Yes, and it is the point of the fix:
pmodover two decimals now reports thesame result type Spark does. Values that fit the Spark result type are
unchanged. Values that do not fit were previously returned at the wider type and
are now NULL (legacy) or an error (ANSI), matching Spark. No public API changes.