alamb commented on code in PR #15776:
URL: https://github.com/apache/datafusion/pull/15776#discussion_r2160311996
##########
datafusion/functions-aggregate/src/correlation.rs:
##########
@@ -372,10 +377,8 @@ impl GroupsAccumulator for CorrelationGroupsAccumulator {
self.sum_xx.resize(total_num_groups, 0.0);
self.sum_yy.resize(total_num_groups, 0.0);
- let array_x = &cast(&values[0], &DataType::Float64)?;
- let array_x = downcast_array::<Float64Array>(array_x);
- let array_y = &cast(&values[1], &DataType::Float64)?;
- let array_y = downcast_array::<Float64Array>(array_y);
+ let array_x = downcast_array::<Float64Array>(&values[0]);
Review Comment:
It looks to me like this code only handles `Float64` but the signature of
the function reports that it accepts any numeric type:
```rust
impl Correlation {
/// Create a new COVAR_POP aggregate function
pub fn new() -> Self {
Self {
signature: Signature::uniform(2, NUMERICS.to_vec(),
Volatility::Immutable),
}
}
}
```
I wonder if you just changed the signature to say the function needs Float64
argument types, woudl that be enough?
DataFusion already has a bunch of coercion rules, see
https://docs.rs/datafusion/latest/datafusion/logical_expr/type_coercion/index.html
for example
--
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]
