alamb commented on code in PR #8353:
URL: https://github.com/apache/arrow-datafusion/pull/8353#discussion_r1409218555
##########
datafusion/physical-expr/src/aggregate/mod.rs:
##########
@@ -102,6 +102,21 @@ pub trait AggregateExpr: Send + Sync + Debug +
PartialEq<dyn Any> {
"AggregateExpr: default name"
}
+ /// Returns Aggregate Fucntion Name
+ fn func_name(&self) -> &str;
+
+ /// Human readable name such as `"MIN(c2)"` or `"RANK()"`. The default
+ /// implementation returns `"FUNCTION_NAME(args)"`
+ fn display_name(&self) -> String {
Review Comment:
I am confused by this API change -- the AggregateFunction now has `name()`,
`func_name()` **and** `display_name()`
I thought the reason for having `name()` was so that that the code that
created the AggregateExpr would calculate the name once and then pass it along.
The creation of the name happened *once* on construction rather than on demand
For example:
https://github.com/apache/arrow-datafusion/blob/4c914ea1e5d3dc61f3552adcffb8356c90d73bac/datafusion/proto/src/physical_plan/mod.rs#L464-L470
This PR seems to have added a parallel API for creating display names for
aggregate functions which I think is quite confusing
##########
datafusion/core/src/physical_optimizer/enforce_sorting.rs:
##########
@@ -906,17 +906,17 @@ mod tests {
let physical_plan = bounded_window_exec("non_nullable_col",
sort_exprs, filter);
- let expected_input = ["BoundedWindowAggExec: wdw=[count: Ok(Field {
name: \"count\", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered:
false, metadata: {} }), frame: WindowFrame { units: Range, start_bound:
Preceding(NULL), end_bound: CurrentRow }], mode=[Sorted]",
+ let expected_input = ["BoundedWindowAggExec:
wdw=[COUNT(non_nullable_col@1) ORDER BY [non_nullable_col@1 ASC NULLS LAST],
frame: WindowFrame { units: Range, start_bound: Preceding(NULL), end_bound:
CurrentRow }], mode=[Sorted]",
Review Comment:
that is certainly much nicer 👍
##########
datafusion/core/src/physical_optimizer/enforce_sorting.rs:
##########
@@ -906,17 +906,17 @@ mod tests {
let physical_plan = bounded_window_exec("non_nullable_col",
sort_exprs, filter);
- let expected_input = ["BoundedWindowAggExec: wdw=[count: Ok(Field {
name: \"count\", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered:
false, metadata: {} }), frame: WindowFrame { units: Range, start_bound:
Preceding(NULL), end_bound: CurrentRow }], mode=[Sorted]",
+ let expected_input = ["BoundedWindowAggExec:
wdw=[COUNT(non_nullable_col@1) ORDER BY [non_nullable_col@1 ASC NULLS LAST],
frame: WindowFrame { units: Range, start_bound: Preceding(NULL), end_bound:
CurrentRow }], mode=[Sorted]",
Review Comment:
that is certainly much nicer 👍
##########
datafusion/physical-expr/src/aggregate/mod.rs:
##########
@@ -102,6 +102,21 @@ pub trait AggregateExpr: Send + Sync + Debug +
PartialEq<dyn Any> {
"AggregateExpr: default name"
}
+ /// Returns Aggregate Fucntion Name
Review Comment:
```suggestion
/// Returns Aggregate Function Name
```
--
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]
