amitvijapur opened a new pull request, #24409:
URL: https://github.com/apache/datafusion/pull/24409
## Which issue does this PR close?
- Closes #23895.
## Rationale for this change
`pmod` reports a wider decimal type than Spark does. Spark derives the result
type of `pmod` with `Pmod.resultDecimalType`, which applies the `Remainder`
rule to the *declared* argument types:
```text
scale = max(s1, s2)
precision = min(p1 - s1, p2 - s2) + scale
```
For `pmod(decimal(3,1), decimal(2,1))` Spark reports `decimal(2,1)`, but
DataFusion reported `Decimal128(3, 1)`.
The cause is coercion. `SparkPmod` used `Signature::numeric`, which collapses
both arguments to a common decimal before `return_type` runs, so the two
precisions `return_type` saw were already equal and the rule degenerated to
the
input precision.
## What changes are included in this PR?
- `SparkPmod` moves to `Signature::user_defined` with a `coerce_types` that
leaves a decimal/decimal argument pair intact, following the precedent set
by
`try_sum`. Every other argument combination keeps the coercion
`Signature::numeric` performed — including its null handling, where a null
argument is skipped and an all-null call falls back to `Float64` — so only
the
decimal pair changes behaviour.
- `return_type` applies Spark's `Pmod.resultDecimalType` rule for decimal
arguments and is unchanged for everything else.
- Because Spark's result type is *narrower* than the dividend, the operands
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))` returns
`decimal(2,1)`, which cannot hold `99.9`. `spark_pmod` therefore widens the
operands 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
when
the divisor is wider than the dividend:
```sql
-- result type decimal(3,1), true value 9999.8
SELECT pmod(-0.1::decimal(3,1), 9999.9::decimal(5,1));
```
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: false` in both modes, since the computation type is
chosen to fit both operands and a silent NULL there would hide a real bug.
### Scope
Deliberately limited to `pmod` over two `Decimal128` arguments, 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:
- **`SparkMod`** has the same bug, since `Remainder.resultDecimalType` is the
same rule. It is the easier of the two: arrow's `Op::Rem` already computes
`min(p1-s1, p2-s2) + max(s1, s2)`, so `mod` needs only the `coerce_types`
pass-through and the matching `return_type`, with no widen/narrow step.
- **Decimal mixed with integer** still diverges: `pmod(2.5::decimal(3,1), 3)`
reports `Decimal128(21, 1)` where Spark casts INT to `decimal(10,0)` and
reports `decimal(3,1)`.
- **`Decimal256`, `Decimal64` and `Decimal32`** pairs fall through to the
previous behaviour. Spark has no equivalent of the wider types.
## Are these changes tested?
Yes.
`datafusion/sqllogictest/test_files/spark/math/pmod.slt` gains:
- four `arrow_typeof` assertions covering equal scales, differing precisions,
differing scales, and the narrowing case;
- a value test for `pmod(99.9::decimal(3,1), 2.5::decimal(2,1))`, the case
that
would regress if the operands were narrowed before the remainder;
- the overflow case above, asserted as NULL in legacy mode and as an error in
the ANSI block; and
- null-argument cases pinning the coercion parity described above.
`modulus.rs` gains a unit test for `pmod_decimal_result_type` covering the
rule
directly, independent of the planner.
The existing `pmod` and `mod` value tests are unchanged and still pass.
Verified
locally: `cargo test -p datafusion-spark --all-features` (279 passed), all
244
`spark/` sqllogictest files, `cargo clippy --all-targets --all-features -D
warnings`, and `cargo fmt --all --check`.
## Are there any user-facing changes?
Yes, and it is the point of the fix: `pmod` over two decimals now reports the
same 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
