amitvijapur commented on code in PR #24409:
URL: https://github.com/apache/datafusion/pull/24409#discussion_r3797089487
##########
datafusion/spark/src/function/math/modulus.rs:
##########
@@ -182,7 +304,7 @@ impl Default for SparkPmod {
impl SparkPmod {
pub fn new() -> Self {
Self {
- signature: Signature::numeric(2, Volatility::Immutable),
+ signature: Signature::user_defined(Volatility::Immutable),
Review 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.
--
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]
