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 20:41:55 UTC

[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6619: Minor: Move get_equal_orderings into `BuiltInWindowFunctionExpr`, remove `BuiltInWindowFunctionExpr::as_any`

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