Jefffrey commented on code in PR #24988:
URL: https://github.com/apache/datafusion/pull/24988#discussion_r3944496441
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -840,12 +848,7 @@ fn get_valid_types(
}
let logical_data_type: NativeType = valid_type.clone().into();
- // Fallback to default type if we don't know which type to coerced
to
- // f64 is chosen since most of the math functions utilize
Signature::numeric,
- // and their default type is double precision
- if logical_data_type == NativeType::Null {
Review Comment:
just moved this up; this would only happen if all input types were null type
##########
datafusion/sqllogictest/test_files/spark/math/mod.slt:
##########
@@ -102,6 +102,21 @@ SELECT MOD(NULL::int, NULL::int) as mod_null_3;
----
NULL
+query I
+SELECT MOD(NULL, 3);
+----
+NULL
Review Comment:
currently failing on main:
```sql
1. query failed: DataFusion error: Error during planning: For function 'mod'
Null and Int64 are not coercible to a common numeric type. No function matches
the given name and argument types 'mod(Null, Int64)'. You might need to add
explicit type casts.
Candidate functions:
mod(Numeric(2))
[SQL] SELECT MOD(NULL, 3);
```
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -816,13 +816,21 @@ fn get_valid_types(
TypeSignature::Numeric(number) => {
function_length_check(function_name, current_types.len(),
*number)?;
+ let non_nulls = current_types
+ .iter()
+ .filter(|&t| NativeType::from(t) != NativeType::Null)
+ .collect::<Vec<_>>();
+ let mut valid_type = non_nulls
+ .first()
+ .copied()
+ .cloned()
+ // Fallback to default type if we don't know which type to
coerced to
+ // f64 is chosen since most of the math functions utilize
Signature::numeric,
+ // and their default type is double precision
+ .unwrap_or(DataType::Float64);
// Find common numeric type among given types except string
- let mut valid_type = current_types.first().unwrap().to_owned();
Review Comment:
if first argument was null type, `valid_type` is set to it, and below when
we call `binary_numeric_coercion` it'll fail since it expects two numeric type
arguments:
https://github.com/apache/datafusion/blob/5b389eb23cb4793d2550d87c249febc2e03e85f6/datafusion/expr-common/src/type_coercion/binary.rs#L1047-L1054
rather than fixing inside `binary_numeric_coercion`, decided it should be
better to ensure we ignore null types initially, then run coercion on any
non-null type, which is essentially what was happening in main (so long as the
first type wasnt null)
--
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]
