alamb commented on code in PR #9249:
URL: https://github.com/apache/arrow-datafusion/pull/9249#discussion_r1497059941
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -1689,13 +1685,16 @@ pub fn create_aggregate_expr_with_name_and_maybe_filter(
(agg_expr, filter, order_by)
}
AggregateFunctionDefinition::UDF(fun) => {
+ let ordering_reqs: Vec<PhysicalSortExpr> =
Review Comment:
👨🍳 👌
Very nice
##########
datafusion/physical-plan/src/windows/mod.rs:
##########
@@ -90,8 +90,15 @@ pub fn create_window_expr(
))
}
WindowFunctionDefinition::AggregateUDF(fun) => {
- let aggregate =
- udaf::create_aggregate_expr(fun.as_ref(), args, input_schema,
name)?;
+ // TODO: Ordering not supported for Window UDFs yet
Review Comment:
We should probably file a ticket for this as well -- I can do so as part of
this PR's review
##########
datafusion/expr/src/expr_fn.rs:
##########
@@ -1073,6 +1075,34 @@ pub fn create_udaf(
))
}
+/// Creates a new UDAF with a specific signature, state type and return type.
Review Comment:
Adding this API effectively means that trait based user defined aggregates
(e.g. `AggregateUDFImpl`) can't take advantage of sorting.
I think the hope is to move all aggregate functions to use the new trait
based API eventually, so using a new function seems to be at odds with that goal
https://github.com/apache/arrow-datafusion/blob/edec4189242ab07ac65967490537d77e776aad5c/datafusion/expr/src/udaf.rs#L242
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -256,11 +256,7 @@ fn create_physical_name(e: &Expr, is_first_expr: bool) ->
Result<String> {
"aggregate expression with filter is not supported"
);
}
- if order_by.is_some() {
- return exec_err!(
- "aggregate expression with order_by is not supported"
- );
- }
+
Review Comment:
The comment above
```
// TODO: Add support for filter and order by in AggregateUDF
```
Could be updated to reflect only the filter is not supported
##########
datafusion/expr/src/expr_fn.rs:
##########
@@ -1073,6 +1075,34 @@ pub fn create_udaf(
))
}
+/// Creates a new UDAF with a specific signature, state type and return type.
Review Comment:
What would you think about changing the `AggregateUDFImpl`
```rust
impl AggregateUDFImpl {
...
fn accumulator(&self, arg: &DataType) -> Result<Box<dyn Accumulator>>;
...
}
```
To something like the following
```rust
impl AggregateUDFImpl {
...
fn accumulator(&self, arg: &DataType, sort_exprs: Vec<Expr>) ->
Result<Box<dyn Accumulator>>;
...
}
```
It would be an API change though (but this API is pretty new).
--
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]
