You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mustafasrepo (via GitHub)" <gi...@apache.org> on 2023/11/29 08:42:43 UTC
[PR] MINOR: Change display of Window and Aggregate Functions [arrow-datafusion]
mustafasrepo opened a new pull request, #8353:
URL: https://github.com/apache/arrow-datafusion/pull/8353
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Change display of Window and Aggregate Functions [arrow-datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on code in PR #8353:
URL: https://github.com/apache/arrow-datafusion/pull/8353#discussion_r1420341351
##########
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:
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Change display of Window and Aggregate Functions [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #8353:
URL: https://github.com/apache/arrow-datafusion/pull/8353#discussion_r1409265032
##########
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 will try to dynamic calculation from implementation
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Change display of Window and Aggregate Functions [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Change display of Window and Aggregate Functions [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #8353:
URL: https://github.com/apache/arrow-datafusion/pull/8353#discussion_r1410199833
##########
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 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](https://github.com/apache/arrow-datafusion/blob/06bbe1298fa8aa042b6a6462e55b2890969d884a/datafusion/physical-expr/src/aggregate/mod.rs#L77)). 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?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Change display of Window and Aggregate Functions [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo closed pull request #8353: MINOR: Change display of Window and Aggregate Functions
URL: https://github.com/apache/arrow-datafusion/pull/8353
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Change display of Window and Aggregate Functions [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8353:
URL: https://github.com/apache/arrow-datafusion/pull/8353#issuecomment-1852898150
@mustafasrepo is this PR ready for review again?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Change display of Window and Aggregate Functions [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #8353:
URL: https://github.com/apache/arrow-datafusion/pull/8353#discussion_r1409265032
##########
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 will try to remove dynamic calculation from implementation
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Change display of Window and Aggregate Functions [arrow-datafusion]
Posted by "synnada-ai (via GitHub)" <gi...@apache.org>.
synnada-ai closed pull request #8353: MINOR: Change display of Window and Aggregate Functions
URL: https://github.com/apache/arrow-datafusion/pull/8353
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Change display of Window and Aggregate Functions [arrow-datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #8353:
URL: https://github.com/apache/arrow-datafusion/pull/8353#issuecomment-1948425945
To avoid any confusion, we will resume this work in a different branch/repo (we are just moving code around between repos).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Change display of Window and Aggregate Functions [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on PR #8353:
URL: https://github.com/apache/arrow-datafusion/pull/8353#issuecomment-1853329984
> @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 in.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Change display of Window and Aggregate Functions [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8353:
URL: https://github.com/apache/arrow-datafusion/pull/8353#discussion_r1411293592
##########
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 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
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org