Jefffrey commented on code in PR #24409:
URL: https://github.com/apache/datafusion/pull/24409#discussion_r3886717921
##########
datafusion/spark/src/function/math/modulus.rs:
##########
@@ -101,23 +105,102 @@ pub fn spark_mod(
Ok(ColumnarValue::Array(result))
}
+/// Spark derives the decimal result type of `pmod` with
`Pmod.resultDecimalType`,
+/// which follows the `Remainder` rule:
+///
+/// ```text
+/// scale = max(s1, s2)
+/// precision = min(p1 - s1, p2 - s2) + scale
+/// ```
+///
+/// The rule is applied to the *declared* argument types. Collapsing both
+/// arguments to a common decimal first would make the two precisions equal and
+/// the rule would degenerate to the input precision, which is why
+/// [`SparkPmod::coerce_types`] leaves decimal arguments intact.
+fn pmod_decimal_result_type(p1: u8, s1: i8, p2: u8, s2: i8) -> DataType {
+ let scale = s1.max(s2);
+ let whole_digits = (i32::from(p1) - i32::from(s1)).min(i32::from(p2) -
i32::from(s2));
+ let precision =
+ (whole_digits + i32::from(scale)).clamp(1,
i32::from(DECIMAL128_MAX_PRECISION));
+ DataType::Decimal128(precision as u8, scale)
+}
+
+/// The type `pmod` computes in, which is not always the type it returns.
+///
+/// Spark's result type is narrower than the dividend, so the operands cannot
be
+/// cast to it before the remainder is taken without overflowing the dividend.
+/// The computation therefore runs in a common type wide enough for both, and
+/// the result is narrowed afterwards.
+fn pmod_computation_type(lhs: &DataType, rhs: &DataType) -> Result<DataType> {
Review Comment:
just use
[`decimal_coercion`](https://docs.rs/datafusion/latest/datafusion/logical_expr/type_coercion/binary/fn.decimal_coercion.html)
directly, as i think thats the only reason this is needed. other numeric types
are coerced by the signature i believe
and can just inline where its needed
##########
datafusion/spark/src/function/math/modulus.rs:
##########
@@ -101,23 +105,102 @@ pub fn spark_mod(
Ok(ColumnarValue::Array(result))
}
+/// Spark derives the decimal result type of `pmod` with
`Pmod.resultDecimalType`,
+/// which follows the `Remainder` rule:
+///
+/// ```text
+/// scale = max(s1, s2)
+/// precision = min(p1 - s1, p2 - s2) + scale
+/// ```
+///
+/// The rule is applied to the *declared* argument types. Collapsing both
+/// arguments to a common decimal first would make the two precisions equal and
+/// the rule would degenerate to the input precision, which is why
+/// [`SparkPmod::coerce_types`] leaves decimal arguments intact.
+fn pmod_decimal_result_type(p1: u8, s1: i8, p2: u8, s2: i8) -> DataType {
+ let scale = s1.max(s2);
+ let whole_digits = (i32::from(p1) - i32::from(s1)).min(i32::from(p2) -
i32::from(s2));
+ let precision =
+ (whole_digits + i32::from(scale)).clamp(1,
i32::from(DECIMAL128_MAX_PRECISION));
+ DataType::Decimal128(precision as u8, scale)
+}
+
+/// The type `pmod` computes in, which is not always the type it returns.
+///
+/// Spark's result type is narrower than the dividend, so the operands cannot
be
+/// cast to it before the remainder is taken without overflowing the dividend.
+/// The computation therefore runs in a common type wide enough for both, and
+/// the result is narrowed afterwards.
+fn pmod_computation_type(lhs: &DataType, rhs: &DataType) -> Result<DataType> {
+ match binary_numeric_coercion(lhs, rhs) {
+ Some(computation_type) => Ok(computation_type),
+ None => exec_err!("pmod does not support ({lhs}, {rhs})"),
+ }
+}
+
/// Spark-compatible `pmod` function
/// In ANSI mode, division by zero throws an error.
/// In legacy mode, division by zero returns NULL (Spark behavior).
pub fn spark_pmod(
args: &[ColumnarValue],
enable_ansi_mode: bool,
+ result_type: &DataType,
) -> Result<ColumnarValue> {
assert_eq_or_internal_err!(args.len(), 2, "pmod expects exactly two
arguments");
let args = ColumnarValue::values_to_arrays(args)?;
- let left = &args[0];
- let right = &args[1];
+
+ // A null argument is passed through uncoerced by `Coercible` (#19458), so
+ // it still carries `DataType::Null` here. Every operation below needs a
+ // concrete numeric type, and the answer is null regardless.
+ if args.iter().any(|arg| arg.data_type() == &DataType::Null) {
+ return Ok(ColumnarValue::Array(new_null_array(
+ result_type,
+ args[0].len(),
+ )));
+ }
+
+ let (left, right): (ArrayRef, ArrayRef) =
+ if args[0].data_type() == args[1].data_type() {
+ (Arc::clone(&args[0]), Arc::clone(&args[1]))
+ } else {
+ let computation_type =
+ pmod_computation_type(args[0].data_type(),
args[1].data_type())?;
+ // The computation type is wide enough for both operands by
+ // construction, so widening must not silently null on overflow the
+ // way arrow's default (`safe: true`) cast would.
Review Comment:
if its wide enough by construction how can it overflow?
##########
datafusion/spark/src/function/math/modulus.rs:
##########
@@ -101,23 +105,102 @@ pub fn spark_mod(
Ok(ColumnarValue::Array(result))
}
+/// Spark derives the decimal result type of `pmod` with
`Pmod.resultDecimalType`,
+/// which follows the `Remainder` rule:
+///
+/// ```text
+/// scale = max(s1, s2)
+/// precision = min(p1 - s1, p2 - s2) + scale
+/// ```
+///
+/// The rule is applied to the *declared* argument types. Collapsing both
+/// arguments to a common decimal first would make the two precisions equal and
+/// the rule would degenerate to the input precision, which is why
+/// [`SparkPmod::coerce_types`] leaves decimal arguments intact.
+fn pmod_decimal_result_type(p1: u8, s1: i8, p2: u8, s2: i8) -> DataType {
+ let scale = s1.max(s2);
+ let whole_digits = (i32::from(p1) - i32::from(s1)).min(i32::from(p2) -
i32::from(s2));
+ let precision =
+ (whole_digits + i32::from(scale)).clamp(1,
i32::from(DECIMAL128_MAX_PRECISION));
+ DataType::Decimal128(precision as u8, scale)
+}
+
+/// The type `pmod` computes in, which is not always the type it returns.
+///
+/// Spark's result type is narrower than the dividend, so the operands cannot
be
+/// cast to it before the remainder is taken without overflowing the dividend.
+/// The computation therefore runs in a common type wide enough for both, and
+/// the result is narrowed afterwards.
+fn pmod_computation_type(lhs: &DataType, rhs: &DataType) -> Result<DataType> {
+ match binary_numeric_coercion(lhs, rhs) {
+ Some(computation_type) => Ok(computation_type),
+ None => exec_err!("pmod does not support ({lhs}, {rhs})"),
+ }
+}
+
/// Spark-compatible `pmod` function
/// In ANSI mode, division by zero throws an error.
/// In legacy mode, division by zero returns NULL (Spark behavior).
pub fn spark_pmod(
args: &[ColumnarValue],
enable_ansi_mode: bool,
+ result_type: &DataType,
) -> Result<ColumnarValue> {
assert_eq_or_internal_err!(args.len(), 2, "pmod expects exactly two
arguments");
let args = ColumnarValue::values_to_arrays(args)?;
- let left = &args[0];
- let right = &args[1];
+
+ // A null argument is passed through uncoerced by `Coercible` (#19458), so
+ // it still carries `DataType::Null` here. Every operation below needs a
+ // concrete numeric type, and the answer is null regardless.
+ if args.iter().any(|arg| arg.data_type() == &DataType::Null) {
+ return Ok(ColumnarValue::Array(new_null_array(
+ result_type,
+ args[0].len(),
+ )));
+ }
+
Review Comment:
```suggestion
// 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,
)?));
}
```
##########
datafusion/spark/src/function/math/modulus.rs:
##########
@@ -101,23 +105,102 @@ pub fn spark_mod(
Ok(ColumnarValue::Array(result))
}
+/// Spark derives the decimal result type of `pmod` with
`Pmod.resultDecimalType`,
+/// which follows the `Remainder` rule:
+///
+/// ```text
+/// scale = max(s1, s2)
+/// precision = min(p1 - s1, p2 - s2) + scale
+/// ```
+///
+/// The rule is applied to the *declared* argument types. Collapsing both
+/// arguments to a common decimal first would make the two precisions equal and
+/// the rule would degenerate to the input precision, which is why
+/// [`SparkPmod::coerce_types`] leaves decimal arguments intact.
Review Comment:
```suggestion
/// The rule is applied to the input argument types.
```
not necessary, not to mention its out of date
##########
datafusion/spark/src/function/math/modulus.rs:
##########
@@ -203,13 +298,30 @@ impl ScalarUDFImpl for SparkPmod {
"pmod expects exactly two arguments"
);
- // Return the same type as the first argument for simplicity
- // Arrow's rem function handles type promotion internally
- Ok(arg_types[0].clone())
+ match (&arg_types[0], &arg_types[1]) {
+ (DataType::Decimal128(p1, s1), DataType::Decimal128(p2, s2)) => {
+ Ok(pmod_decimal_result_type(*p1, *s1, *p2, *s2))
+ }
+ // `Coercible` matches a null argument and passes it through
+ // uncoerced (#19458), so an untyped NULL reaches here rather than
+ // being folded by `Numeric(2)`. `mod` answers `Float64` for two
+ // untyped nulls and the other side's type when only one is null;
+ // `pmod` did too, so that behaviour is kept explicitly here.
Review Comment:
```suggestion
// Need to handle nulls explicitly, see:
https://github.com/apache/datafusion/issues/19458
// We align with the behaviour of `mod`
```
##########
datafusion/spark/src/function/math/modulus.rs:
##########
@@ -203,13 +298,30 @@ impl ScalarUDFImpl for SparkPmod {
"pmod expects exactly two arguments"
);
- // Return the same type as the first argument for simplicity
- // Arrow's rem function handles type promotion internally
- Ok(arg_types[0].clone())
+ match (&arg_types[0], &arg_types[1]) {
+ (DataType::Decimal128(p1, s1), DataType::Decimal128(p2, s2)) => {
+ Ok(pmod_decimal_result_type(*p1, *s1, *p2, *s2))
+ }
+ // `Coercible` matches a null argument and passes it through
+ // uncoerced (#19458), so an untyped NULL reaches here rather than
+ // being folded by `Numeric(2)`. `mod` answers `Float64` for two
+ // untyped nulls and the other side's type when only one is null;
+ // `pmod` did too, so that behaviour is kept explicitly here.
+ (DataType::Null, DataType::Null) => Ok(DataType::Float64),
+ (DataType::Null, other) | (other, DataType::Null) =>
Ok(other.clone()),
+ // Arrow's rem function handles type promotion for the rest
Review Comment:
```suggestion
```
this was incorrect
##########
datafusion/sqllogictest/test_files/spark/math/pmod.slt:
##########
@@ -305,6 +338,70 @@ SELECT pmod(-10.0::decimal(3,1), 3.0::decimal(2,1)) as
pmod_decimal_4;
----
2
+# The decimal result type follows Spark's Pmod.resultDecimalType, which applies
+# the Remainder rule to the declared argument types:
+# scale = max(s1, s2)
+# precision = min(p1 - s1, p2 - s2) + scale
+query T
+SELECT arrow_typeof(pmod(2.5::decimal(3,1), 1.2::decimal(2,1)));
+----
+Decimal128(2, 1)
+
+# The divisor bounds the result, so the narrower argument decides the precision
+query T
+SELECT arrow_typeof(pmod(10.0::decimal(5,2), 3.0::decimal(4,1)));
+----
+Decimal128(5, 2)
+
+# Differing scales: the wider scale wins
+query T
+SELECT arrow_typeof(pmod(1.234::decimal(6,3), 2.1::decimal(4,1)));
+----
+Decimal128(6, 3)
+
+# The dividend does not fit the result type, so it must not be narrowed before
+# the remainder is taken
+query T
+SELECT arrow_typeof(pmod(99.9::decimal(3,1), 2.5::decimal(2,1)));
+----
+Decimal128(2, 1)
+
+query R
+SELECT pmod(99.9::decimal(3,1), 2.5::decimal(2,1));
+----
+2.4
+
+# A remainder is bounded by the divisor, not by the result type, so a divisor
+# wider than the dividend can produce a value the result type cannot hold.
Spark
+# wraps decimal arithmetic in CheckOverflow(nullOnOverflow = !ansiEnabled), so
+# this is NULL in legacy mode and an error under ANSI (asserted further down).
Review Comment:
> and an error under ANSI (asserted further down).
where?
##########
datafusion/sqllogictest/test_files/spark/math/pmod.slt:
##########
@@ -128,6 +132,35 @@ SELECT pmod(NULL::int, NULL::int) as pmod_null_3;
----
NULL
+# 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 R
+SELECT pmod(NULL, NULL);
+----
+NULL
+
+query T
+SELECT arrow_typeof(pmod(2.5::decimal(3,1), NULL));
+----
+Decimal128(3, 1)
+
+query R
+SELECT pmod(2.5::decimal(3,1), NULL);
+----
+NULL
+
+# 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);
Review Comment:
this seems quite odd and is worth looking into further in a followup
##########
datafusion/sqllogictest/test_files/spark/math/pmod.slt:
##########
@@ -305,6 +338,70 @@ SELECT pmod(-10.0::decimal(3,1), 3.0::decimal(2,1)) as
pmod_decimal_4;
----
2
+# The decimal result type follows Spark's Pmod.resultDecimalType, which applies
+# the Remainder rule to the declared argument types:
+# scale = max(s1, s2)
+# precision = min(p1 - s1, p2 - s2) + scale
+query T
+SELECT arrow_typeof(pmod(2.5::decimal(3,1), 1.2::decimal(2,1)));
+----
+Decimal128(2, 1)
+
+# The divisor bounds the result, so the narrower argument decides the precision
+query T
+SELECT arrow_typeof(pmod(10.0::decimal(5,2), 3.0::decimal(4,1)));
+----
+Decimal128(5, 2)
+
+# Differing scales: the wider scale wins
+query T
+SELECT arrow_typeof(pmod(1.234::decimal(6,3), 2.1::decimal(4,1)));
+----
+Decimal128(6, 3)
+
+# The dividend does not fit the result type, so it must not be narrowed before
+# the remainder is taken
+query T
+SELECT arrow_typeof(pmod(99.9::decimal(3,1), 2.5::decimal(2,1)));
+----
+Decimal128(2, 1)
+
+query R
+SELECT pmod(99.9::decimal(3,1), 2.5::decimal(2,1));
+----
+2.4
+
+# A remainder is bounded by the divisor, not by the result type, so a divisor
+# wider than the dividend can produce a value the result type cannot hold.
Spark
+# wraps decimal arithmetic in CheckOverflow(nullOnOverflow = !ansiEnabled), so
+# this is NULL in legacy mode and an error under ANSI (asserted further down).
+query T
+SELECT arrow_typeof(pmod(-0.1::decimal(3,1), 9999.9::decimal(5,1)));
+----
+Decimal128(3, 1)
+
+query R
+SELECT pmod(-0.1::decimal(3,1), 9999.9::decimal(5,1));
+----
+NULL
+
+# An untyped NULL pair falls back to Float64, as it did under
Signature::numeric
Review Comment:
```suggestion
```
##########
datafusion/spark/src/function/math/modulus.rs:
##########
@@ -203,13 +298,30 @@ impl ScalarUDFImpl for SparkPmod {
"pmod expects exactly two arguments"
);
- // Return the same type as the first argument for simplicity
- // Arrow's rem function handles type promotion internally
- Ok(arg_types[0].clone())
+ match (&arg_types[0], &arg_types[1]) {
+ (DataType::Decimal128(p1, s1), DataType::Decimal128(p2, s2)) => {
+ Ok(pmod_decimal_result_type(*p1, *s1, *p2, *s2))
+ }
+ // `Coercible` matches a null argument and passes it through
+ // uncoerced (#19458), so an untyped NULL reaches here rather than
+ // being folded by `Numeric(2)`. `mod` answers `Float64` for two
+ // untyped nulls and the other side's type when only one is null;
+ // `pmod` did too, so that behaviour is kept explicitly here.
+ (DataType::Null, DataType::Null) => Ok(DataType::Float64),
+ (DataType::Null, other) | (other, DataType::Null) =>
Ok(other.clone()),
+ // Arrow's rem function handles type promotion for the rest
+ _ => Ok(arg_types[0].clone()),
+ }
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
- spark_pmod(&args.args, args.config_options.execution.enable_ansi_mode)
+ // The Spark result type was already derived in `return_type`, so it is
+ // read back here rather than recomputed from the argument arrays.
Review Comment:
```suggestion
```
this is not necessary
##########
datafusion/sqllogictest/test_files/spark/math/pmod.slt:
##########
@@ -128,6 +132,35 @@ SELECT pmod(NULL::int, NULL::int) as pmod_null_3;
----
NULL
+# 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
+
Review Comment:
```suggestion
query T
SELECT arrow_typeof(pmod(NULL, NULL));
----
Float64
```
we dont have to keep repeating this
--
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]
