Perform type coercion for corr aggregate function during physical planning #15776
Conversation
github-actions
bot
added
logical-expr
kumarlokesh
force-pushed
the
avoid-explicit-cast-in-corr-aggregate-fn
branch
from
41587c4 to
1933efe
Compare
kumarlokesh
force-pushed
the
avoid-explicit-cast-in-corr-aggregate-fn
branch
from
1933efe to
d8a7641
Compare
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
|
PR is active. |
| @@ -1598,8 +1600,16 @@ pub fn create_aggregate_expr_with_name_and_maybe_filter( | |||
| physical_name(e)? | |||
| }; | |||
|
|
|||
| let physical_args = | |||
| // Create base physical expressions (without coercion) | |||
There was a problem hiding this comment.
I think typically coercsion is applied ealier in the planning proesses, not after physiscal expressions have been crated
Did you consider applying the coercsion along with the other coercion ?
| 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]); |
There was a problem hiding this comment.
It looks to me like this code only handles Float64 but the signature of the function reports that it accepts any numeric type:
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
|
I am sorry for the delayed review |
|
Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look |
Which issue does this PR close?
corraggregate function #13721.Rationale for this change
What changes are included in this PR?
apply_aggregate_coercionthat applies typecoercion during physical planning based on the logical type coercion rules
CorrelationGroupsAccumulator.update_batchmethodAre these changes tested?
Are there any user-facing changes?