You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/06/09 19:54:26 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #6619: Minor: Move get_equal_orderings into `BuiltInWindowFunctionExpr`, remove `BuiltInWindowFunctionExpr::as_any`
alamb opened a new pull request, #6619:
URL: https://github.com/apache/arrow-datafusion/pull/6619
# Which issue does this PR close?
This is related to #5781
# Rationale for this change
I would like to ensure that user defined window functions have the same expressive power as built in window functions.
In order to achieve this goal, all the important information about a window function is provided by a trait (rather than by other code special casing based on the type).
# What changes are included in this PR?
1. Move `get_equal_orderings` into `BuiltInWindowFunctionExpr`
2. Remove `BuiltInWindowFunctionExpr::as_any` (which is how I found the special case for RowNumber)
# Are these changes tested?
Covered by existing tests
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
3. 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?
No this only changes internal APIs
Note that my proposal in https://github.com/apache/arrow-datafusion/pull/6617 for user defined window functions is to make `BuiltInWindowFunctionExpr` public. However, even if we choose another option, I think this still PR makes the code more explicit and easier to find functionality related to window functions so I think it is still a good change
<!--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6619: Minor: Move get_equal_orderings into `BuiltInWindowFunctionExpr`, remove `BuiltInWindowFunctionExpr::as_any`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6619:
URL: https://github.com/apache/arrow-datafusion/pull/6619#discussion_r1227348648
##########
datafusion/physical-expr/src/window/row_number.rs:
##########
@@ -61,6 +58,24 @@ impl BuiltInWindowFunctionExpr for RowNumber {
&self.name
}
+ fn add_equal_orderings(&self, builder: &mut OrderingEquivalenceBuilder) {
+ // Only the built-in `RowNumber` window function introduces a new
Review Comment:
👍 done in commit ad165028d5
--
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
[GitHub] [arrow-datafusion] mustafasrepo commented on a diff in pull request #6619: Minor: Move get_equal_orderings into `BuiltInWindowFunctionExpr`, remove `BuiltInWindowFunctionExpr::as_any`
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #6619:
URL: https://github.com/apache/arrow-datafusion/pull/6619#discussion_r1226205915
##########
datafusion/physical-expr/src/window/row_number.rs:
##########
@@ -61,6 +58,24 @@ impl BuiltInWindowFunctionExpr for RowNumber {
&self.name
}
+ fn add_equal_orderings(&self, builder: &mut OrderingEquivalenceBuilder) {
+ // Only the built-in `RowNumber` window function introduces a new
Review Comment:
Maybe we can replace comment
`// Only the built-in RowNumber window function introduces a new` with
`// The built-in RowNumber window function introduces a new`. Since method implements it (and special handling is not done outside), we do not need stress this is the only window function with this property.
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6619: Minor: Move get_equal_orderings into `BuiltInWindowFunctionExpr`, remove `BuiltInWindowFunctionExpr::as_any`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6619:
URL: https://github.com/apache/arrow-datafusion/pull/6619#discussion_r1224709658
##########
datafusion/core/src/physical_plan/windows/mod.rs:
##########
@@ -245,32 +244,14 @@ pub(crate) fn window_ordering_equivalence(
.with_equivalences(input.equivalence_properties())
.with_existing_ordering(input.output_ordering().map(|elem| elem.to_vec()))
.extend(input.ordering_equivalence_properties());
+
for expr in window_expr {
if let Some(builtin_window_expr) =
expr.as_any().downcast_ref::<BuiltInWindowExpr>()
{
- // Only the built-in `RowNumber` window function introduces a new
- // ordering:
- if builtin_window_expr
+ builtin_window_expr
.get_built_in_func_expr()
- .as_any()
- .is::<RowNumber>()
Review Comment:
The point of the PR is to remove this special case for RowNumber (which I did by hoisting it into the trait)
##########
datafusion/physical-expr/src/window/built_in_window_function_expr.rs:
##########
@@ -34,10 +35,6 @@ use std::sync::Arc;
/// `nth_value` need the value.
#[allow(rustdoc::private_intra_doc_links)]
pub trait BuiltInWindowFunctionExpr: Send + Sync + std::fmt::Debug {
- /// Returns the aggregate expression as [`Any`](std::any::Any) so that it can be
- /// downcast to a specific implementation.
- fn as_any(&self) -> &dyn Any;
Review Comment:
If reviewers would prefer I keep this in I can do so -- the reason I think it would be best to avoid is so that other parts of the code can't special case the built in window functions
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6619: Minor: Move get_equal_orderings into `BuiltInWindowFunctionExpr`, remove `BuiltInWindowFunctionExpr::as_any`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6619:
URL: https://github.com/apache/arrow-datafusion/pull/6619#discussion_r1224710392
##########
datafusion/physical-expr/src/window/built_in_window_function_expr.rs:
##########
@@ -34,10 +35,6 @@ use std::sync::Arc;
/// `nth_value` need the value.
#[allow(rustdoc::private_intra_doc_links)]
pub trait BuiltInWindowFunctionExpr: Send + Sync + std::fmt::Debug {
- /// Returns the aggregate expression as [`Any`](std::any::Any) so that it can be
- /// downcast to a specific implementation.
- fn as_any(&self) -> &dyn Any;
Review Comment:
If reviewers would prefer to keep `as_any()` I can do so -- the reason I think it would be best to avoid is so that other parts of the code can't special case the built in window functions
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6619: Minor: Move get_equal_orderings into `BuiltInWindowFunctionExpr`, remove `BuiltInWindowFunctionExpr::as_any`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6619:
URL: https://github.com/apache/arrow-datafusion/pull/6619#discussion_r1227348648
##########
datafusion/physical-expr/src/window/row_number.rs:
##########
@@ -61,6 +58,24 @@ impl BuiltInWindowFunctionExpr for RowNumber {
&self.name
}
+ fn add_equal_orderings(&self, builder: &mut OrderingEquivalenceBuilder) {
+ // Only the built-in `RowNumber` window function introduces a new
Review Comment:
👍 done in ad165028d5
--
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
[GitHub] [arrow-datafusion] alamb merged pull request #6619: Minor: Move get_equal_orderings into `BuiltInWindowFunctionExpr`, remove `BuiltInWindowFunctionExpr::as_any`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6619:
URL: https://github.com/apache/arrow-datafusion/pull/6619
--
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
[GitHub] [arrow-datafusion] alamb commented on pull request #6619: Minor: Move get_equal_orderings into `BuiltInWindowFunctionExpr`, remove `BuiltInWindowFunctionExpr::as_any`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6619:
URL: https://github.com/apache/arrow-datafusion/pull/6619#issuecomment-1588258943
I have reverted the changes to `as_any` in [ef89135](https://github.com/apache/arrow-datafusion/pull/6619/commits/ef89135388600c5da75dcaf2de6ad3674466e0cc)
--
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
[GitHub] [arrow-datafusion] alamb commented on pull request #6619: Minor: Move get_equal_orderings into `BuiltInWindowFunctionExpr`, remove `BuiltInWindowFunctionExpr::as_any`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6619:
URL: https://github.com/apache/arrow-datafusion/pull/6619#issuecomment-1587309700
> By the way regarding the as_any() as fas as I can remember it is not introduces by us. I am not familiar with its purpose. However, even though we no longer have special handling outside and no use-case for it in datafusion code base. I can imagine it may be useful for others. I wonder what others think about it.
@Jimexist (who I think added the first version of window functions and perhaps the trait), @andygrove, @viirya or @Dandandan do you have any thoughts about keeping / removing the `as_any`, or leaving it in?
Given the question, I think the safe thing will be to change this PR to put back the `as_any` and we can then discuss that change separately. I'll plan to restore `as_any` in this PR if I don't hear any other comments
--
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