Jefffrey commented on code in PR #19463:
URL: https://github.com/apache/datafusion/pull/19463#discussion_r2642434464
##########
datafusion/functions-aggregate/src/correlation.rs:
##########
@@ -85,8 +86,11 @@ impl Correlation {
/// Create a new CORR aggregate function
pub fn new() -> Self {
Self {
- signature: Signature::exact(
- vec![DataType::Float64, DataType::Float64],
+ signature: Signature::coercible(
+ vec![
+ Coercion::new_exact(TypeSignatureClass::Numeric),
+ Coercion::new_exact(TypeSignatureClass::Numeric),
+ ],
Review Comment:
I am not sure of this approach of accepting all numeric types at the
signature level only to cast it internally 🤔
##########
datafusion/functions-aggregate/src/approx_percentile_cont.rs:
##########
@@ -133,18 +133,30 @@ impl Default for ApproxPercentileCont {
impl ApproxPercentileCont {
/// Create a new [`ApproxPercentileCont`] aggregate function.
pub fn new() -> Self {
- let mut variants = Vec::with_capacity(NUMERICS.len() * (INTEGERS.len()
+ 1));
- // Accept any numeric value paired with a float64 percentile
- for num in NUMERICS {
- variants.push(TypeSignature::Exact(vec![num.clone(),
DataType::Float64]));
- // Additionally accept an integer number of centroids for T-Digest
- for int in INTEGERS {
- variants.push(TypeSignature::Exact(vec![
- num.clone(),
- DataType::Float64,
- int.clone(),
- ]))
- }
+ let percentile_coercion = Coercion::new_implicit(
+ TypeSignatureClass::Native(logical_float64()),
+ vec![
+ TypeSignatureClass::Integer,
+ TypeSignatureClass::Float,
+ TypeSignatureClass::Decimal,
+ ],
+ NativeType::Float64,
+ );
+
+ // Value must be numeric (excluding Decimal) as the accumulator
currently only
+ // supports integers and floats. Percentile may be any numeric literal
that can
+ // be coerced to Float64 (to support parse_float_as_decimal).
+ let mut variants = Vec::with_capacity(4);
+ for value_class in [TypeSignatureClass::Integer,
TypeSignatureClass::Float] {
+ variants.push(TypeSignature::Coercible(vec![
+ Coercion::new_exact(value_class.clone()),
+ percentile_coercion.clone(),
+ ]));
+ variants.push(TypeSignature::Coercible(vec![
+ Coercion::new_exact(value_class),
+ percentile_coercion.clone(),
Review Comment:
I'm hesitant to accept a signature like this, since I don't like the look of
this and it would need to be refactored anyway
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -673,12 +673,12 @@ fn get_valid_types(
default_casted_type.default_cast_for(current_type)?;
new_types.push(casted_type);
} else {
- return internal_err!(
- "Expect {} but received NativeType::{}, DataType: {}",
- param.desired_type(),
- current_native_type,
- current_type
- );
+ // No valid coercion for this signature given the current
type.
+ // Return an empty set so the higher-level signature
matching
+ // logic can continue checking other variants (e.g.
`OneOf`)
+ // and/or produce a consistent "Failed to coerce arguments"
+ // error message.
+ return Ok(vec![]);
Review Comment:
We should not be changing this here; I think we need more consideration
before changing 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]
