[GitHub] [arrow-datafusion] jorgecarleitao commented on a change in pull request #334: Add window expression part 1 - logical and physical planning, structure, to/from proto, and explain, for empty over clause only

[GitBox <gi...@apache.org>]

jorgecarleitao commented on a change in pull request #334:
URL: https://github.com/apache/arrow-datafusion/pull/334#discussion_r636596927



##########
File path: datafusion/src/logical_plan/builder.rs
##########
@@ -289,6 +288,52 @@ impl LogicalPlanBuilder {
         }))
     }
 
+    /// Apply a window
+    ///
+    /// NOTE: this feature is under development and this API will be changing
+    ///
+    /// - https://github.com/apache/arrow-datafusion/issues/359 basic structure
+    /// - https://github.com/apache/arrow-datafusion/issues/298 empty over clause
+    /// - https://github.com/apache/arrow-datafusion/issues/299 with partition clause
+    /// - https://github.com/apache/arrow-datafusion/issues/360 with order by
+    /// - https://github.com/apache/arrow-datafusion/issues/361 with window frame
+    pub fn window(
+        &self,
+        window_expr: impl IntoIterator<Item = Expr>,

Review comment:
       I am not sure we should use `IntoIterator<Item = Expr>` for every field with 6 fields. This will produce a version of the compiled function for every combination, which IMO adds an unnecessary compile time and binary size.
   
   `IntoIterator` is more relevant when we want to avoid an extra allocation; these are very small vectors.

##########
File path: datafusion/src/physical_plan/aggregates.rs
##########
@@ -183,7 +182,7 @@ static TIMESTAMPS: &[DataType] = &[
 ];
 
 /// the signatures supported by the function `fun`.
-fn signature(fun: &AggregateFunction) -> Signature {
+pub fn signature(fun: &AggregateFunction) -> Signature {

Review comment:
       `pub(super)` or `pub(crate)` instead?




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org