MINOR: Change display of Window and Aggregate Functions #8353
Conversation
# Conflicts: # datafusion/sqllogictest/test_files/window.slt
github-actions
bot
added
physical-expr
# Conflicts: # datafusion/physical-expr/src/window/nth_value.rs # datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs # datafusion/sqllogictest/test_files/window.slt
There was a problem hiding this comment.
Thank you @mustafasrepo -- the output of this PR looks much nicer, but I am confused about the seemingly parallel API.
| @@ -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]", | |||
There was a problem hiding this comment.
that is certainly much nicer 👍
|
|
||
| /// Human readable name such as `"MIN(c2)"` or `"RANK()"`. The default | ||
| /// implementation returns `"FUNCTION_NAME(args)"` | ||
| fn display_name(&self) -> String { |
There was a problem hiding this 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
This PR seems to have added a parallel API for creating display names for aggregate functions which I think is quite confusing
There was a problem hiding this comment.
I will try to remove dynamic calculation from implementation
There was a problem hiding this comment.
I gave this a try. However, replacing name with current display name did not work. It seems like during schema creation aggregate exprs uses .name for the field (See link). Hence plan depends on the .name string for successful execution. Otherwise subsequent projection after (window or aggregation) may refer to invalid fields.
Other option might be adding a display_name member to the each aggregate expression (similar to .name method). This display_name member will not be set from argument but will calculate at the initialization(Users will not have control over it). Then we will use .display_name(&self) -> &str API to return this member. With this way func_name argument will be removed. However, we will still have parallel .display_name on top .name which might be confusing. However, I think this approach is better than the current one.
What do you think?
There was a problem hiding this comment.
I was thinking that we could add a name field to WindowFunction upon creation to mirror what happens with AggregateFunction. That way the code would be consistent and we can use the same pattern that works for AggregateFunction
There was a problem hiding this comment.
The fact that we have name, func_name and display_name is an indication that we can do a better design. @mustafasrepo and I will discuss to see what we can do here.
Co-authored-by: Andrew Lamb <[email protected]>
|
@mustafasrepo is this PR ready for review again? |
Not really, I didn't have time to experiment with alternative solutions. For now, I am converting this PR to draft. Once it is ready, I will un-draft it. |
|
To avoid any confusion, we will resume this work in a different branch/repo (we are just moving code around between repos). |
Which issue does this PR close?
Closes #.
Rationale for this change
As discussed in link. Currently, it is not obvious which aggregate or window functions are reversed. The reason is that, display of window expressions are static (comes from logical expression).
This PR changes display implementation of window expression so that they do not use static logical plan window expressions. With this new PR, display String is constructed using (function name, partition by (if available), order by expressions (if available)).
What changes are included in this PR?
Displays of
WindowAggExec,BoundedWindowAggExec,AggregateExecare changed in the tests.Are these changes tested?
Existing tests should work.
Are there any user-facing changes?