Jefffrey commented on code in PR #24409:
URL: https://github.com/apache/datafusion/pull/24409#discussion_r3889566681
##########
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:
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
##########
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:
bumping to remove this unnecessary comment
--
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]
