Skip to content

fix(spark): derive pmod's decimal result type from the declared arguments - #24409

Open
amitvijapur wants to merge 1 commit into
apache:mainfrom
amitvijapur:fix/spark-pmod-decimal-result-type
Open

fix(spark): derive pmod's decimal result type from the declared arguments#24409
amitvijapur wants to merge 1 commit into
apache:mainfrom
amitvijapur:fix/spark-pmod-decimal-result-type

Conversation

@amitvijapur

Copy link
Copy Markdown

Which issue does this PR close?

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:

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:

-- 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.

…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
@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) spark labels Aug 16, 2026
pub fn new() -> Self {
Self {
signature: Signature::numeric(2, Volatility::Immutable),
signature: Signature::user_defined(Volatility::Immutable),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

spark sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] pmod returns a wider decimal type than Spark

2 participants