fix(spark): derive pmod's decimal result type from the declared arguments - #24409
fix(spark): derive pmod's decimal result type from the declared arguments#24409amitvijapur wants to merge 6 commits 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
| pub fn new() -> Self { | ||
| Self { | ||
| signature: Signature::numeric(2, Volatility::Immutable), | ||
| signature: Signature::user_defined(Volatility::Immutable), |
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
There was a problem hiding this comment.
Thanks — I've taken the second half of this. spark_pmod now receives the type
via ScalarFunctionArgs::return_type instead of re-deriving it from the argument
arrays, so the rule is applied in exactly one place (e363e12).
The one_of signature I could not get to work, and I think the reason is
structural rather than something I can order around. NativeType::Null matches
every TypeSignatureClass:
https://github.com/apache/datafusion/blob/main/datafusion/expr-common/src/signature.rs#L460-L462
and is then left at its origin type rather than being cast to the desired one:
https://github.com/apache/datafusion/blob/main/datafusion/expr-common/src/signature.rs#L520
So Coercible([Decimal, Decimal]) accepts a null argument, passes it through as
Null, and Numeric(2) is never reached. Swapping the order does not help,
since Numeric would then match the decimal pair first and unify the two
precisions, which is the bug this PR is fixing.
With the one_of version applied, three cases regress:
SELECT arrow_typeof(pmod(NULL, NULL));
This feature is not implemented: Can't create a zero scalar from data_type "Null"
SELECT pmod(NULL, NULL);
This feature is not implemented: Can't create a zero scalar from data_type "Null"
SELECT arrow_typeof(pmod(2.5::decimal(3,1), NULL));
Execution error: pmod does not support (Decimal128(3, 1), Null)
mod returns Float64 and Decimal128(3, 1) for those, and pmod did too
before this PR, so they looked worth keeping. That is what the coerce_types
version is doing: decimal pairs pass through untouched, and everything else —
nulls included — reuses the same fold TypeSignature::Numeric performs, so the
existing behaviour is preserved rather than reimplemented.
Happy to switch if there is a way to make Coercible decline nulls that I've
missed, or if you'd rather the null cases be handled explicitly in return_type
instead.
There was a problem hiding this comment.
or if you'd rather the null cases be handled explicitly in
return_type
instead.
i think this approach would be good. we do have an issue regarding how null types interact with this coercion api:
so until that is resolved this is probably a good interim fix
There was a problem hiding this comment.
Done in ff84872. one_of with Coercible([Decimal, Decimal]) then Numeric(2), and pmod_numeric_coercion is gone with it.
The null result types are decided in return_type now. I checked these against mod rather than guessing: arrow_typeof(mod(NULL, NULL)) is Float64 and arrow_typeof(mod(2.5::decimal(3,1), NULL)) is Decimal128(3, 1), and pmod now returns the same.
One knock-on: since the null is no longer coerced away, spark_pmod can receive DataType::Null arrays, so it short-circuits to a null array of the result type instead of failing on ScalarValue::new_zero.
pmod(NULL, 3::int) does not plan under Numeric(2). mod rejects it identically, and pmod did too before this PR when it used Signature::numeric(2), so I've covered it with a statement error test rather than treating it as a regression to fix here.
|
Noting the overlap for reviewers: #23898 touches the same two files. That PR corrects So they are complementary rather than competing, but they will conflict |
`spark_pmod` re-derived the Spark decimal result type from the argument arrays, duplicating the rule `return_type` had already applied. Pass the computed type in instead, so it is derived in exactly one place. Per review feedback on apache#24409.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24409 +/- ##
==========================================
+ Coverage 81.22% 81.61% +0.38%
==========================================
Files 1111 1124 +13
Lines 389991 412059 +22068
Branches 389991 412059 +22068
==========================================
+ Hits 316783 336300 +19517
- Misses 54590 55947 +1357
- Partials 18618 19812 +1194 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…explicitly
Replaces the hand-written `coerce_types` with the `one_of` signature suggested
in review:
Coercible([Decimal, Decimal])
Numeric(2)
`Coercible` matches a null argument and passes it through uncoerced (apache#19458),
so a decimal pair still reaches `return_type` with its declared precision and
scale, while everything else falls to `Numeric(2)`. That drops
`pmod_numeric_coercion`, which existed only to reproduce `Numeric`'s fold.
The null cases the coercion API cannot express are handled in `return_type`
instead: two untyped nulls answer `Float64`, and one untyped null keeps the
other side's type. Both match what `mod` returns, verified directly:
SELECT arrow_typeof(mod(NULL, NULL)); -- Float64
SELECT arrow_typeof(mod(2.5::decimal(3,1), NULL)); -- Decimal128(3, 1)
Because the null is no longer coerced away, `spark_pmod` can now receive
`DataType::Null` arrays, so it short-circuits to a null array of the result
type rather than failing to build a zero scalar.
`pmod(NULL, 3::int)` does not plan under `Numeric(2)`. That matches both `mod`
today and `pmod` before this PR, which used `Signature::numeric(2)`, so it is
covered by a `statement error` test rather than treated as a regression.
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
im assuming theres heavy LLM usage at play here; please ensure you disclose this and carefully review its output as im seeing unnecessary comments
| args[0].len(), | ||
| ))); | ||
| } | ||
|
|
There was a problem hiding this comment.
| // Need to handle nulls separately as they are pass through by the signature | |
| if args.iter().any(|arg| arg.data_type() == &DataType::Null) { | |
| return Ok(ColumnarValue::Scalar(ScalarValue::try_new_null( | |
| result_type, | |
| )?)); | |
| } |
There was a problem hiding this comment.
same here; we only really need the line of Need to handle nulls separately as they are pass through by the signature, the rest is unnecessarily verbose
| # An untyped NULL beside a typed non-decimal argument takes the Numeric path, | ||
| # which cannot coerce the pair. `mod` rejects it the same way. | ||
| statement error DataFusion error: Error during planning: Internal error: Function 'pmod' failed to match any signature | ||
| SELECT pmod(NULL, 3::int); |
There was a problem hiding this comment.
this seems quite odd and is worth looking into further in a followup
| # An untyped NULL matches the decimal signature and is passed through | ||
| # uncoerced (apache/datafusion#19458), so these types are decided explicitly | ||
| # rather than by coercion. `mod` answers Float64 for two untyped nulls and | ||
| # keeps the other side's type when only one is null; pmod matches it. | ||
| query T | ||
| SELECT arrow_typeof(pmod(NULL, NULL)); | ||
| ---- | ||
| Float64 | ||
|
|
There was a problem hiding this comment.
| # An untyped NULL matches the decimal signature and is passed through | |
| # uncoerced (apache/datafusion#19458), so these types are decided explicitly | |
| # rather than by coercion. `mod` answers Float64 for two untyped nulls and | |
| # keeps the other side's type when only one is null; pmod matches it. | |
| query T | |
| SELECT arrow_typeof(pmod(NULL, NULL)); | |
| ---- | |
| Float64 | |
| query T | |
| SELECT arrow_typeof(pmod(NULL, NULL)); | |
| ---- | |
| Float64 | |
we dont have to keep repeating this
There was a problem hiding this comment.
bumping to remove this unnecessary comment
Uses `decimal_coercion` directly at the call site and drops `pmod_computation_type`: only a decimal pair can reach that branch, since `Numeric(2)` already gives every other combination a common type. Returns a null scalar rather than building a null array for the null short-circuit. Removes comments that were wrong or unnecessary: a doc paragraph referencing `SparkPmod::coerce_types`, which this PR deletes; a claim that the computation type is wide enough by construction alongside a justification for overflow handling; and a pointer to an ANSI assertion "further down" that sits above it. Drops three sqllogictest cases that were duplicated in two places, and the value assertions that only restated the type assertions beside them.
|
Yes, LLM-assisted, and I should have said so upfront rather than when asked. I use Claude Code for implementation and review its output before pushing. That review wasn't good enough here. You caught a doc paragraph pointing at All eleven points are applied in d336e0e, and the diff is 43 lines smaller for it. On the understanding bar in the contributor guide: what I can justify is the null and coercion interaction. When you suggested What I'd flag as not fully mine: I can't tell you why I kept the Going forward I'll keep comments to what I'd write myself. |
Removes the comment above the decimal coercion branch and the block above the null type assertions in pmod.slt, per review.
3 participants
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.