edrevo commented on a change in pull request #532:
URL: https://github.com/apache/arrow-datafusion/pull/532#discussion_r649800622
##########
File path: datafusion/src/physical_plan/windows.rs
##########
@@ -61,24 +63,27 @@ pub struct WindowAggExec {
/// Create a physical expression for window function
pub fn create_window_expr(
fun: &WindowFunction,
+ name: String,
args: &[Arc<dyn PhysicalExpr>],
+ _partition_by: &[Arc<dyn PhysicalExpr>],
+ _order_by: &[PhysicalSortExpr],
+ _window_frame: WindowFrame,
Review comment:
I just realized that this PR changes the behavior when there is a
non-empty partition_by/order_by/window_frame: before (at least in ballista) it
would error, whereas now it is silently ignored. Maybe it is worth erroring is
they aren't empty to make it explicit that there is no support?
##########
File path: datafusion/src/physical_plan/windows.rs
##########
@@ -61,24 +63,27 @@ pub struct WindowAggExec {
/// Create a physical expression for window function
pub fn create_window_expr(
fun: &WindowFunction,
+ name: String,
args: &[Arc<dyn PhysicalExpr>],
+ _partition_by: &[Arc<dyn PhysicalExpr>],
+ _order_by: &[PhysicalSortExpr],
+ _window_frame: WindowFrame,
Review comment:
nit: once this is merged, it would be cool to add a link to this code in
the respective issues that are tracking these missing features to make it
easier for new contributors to start contributing.
##########
File path: datafusion/src/physical_plan/windows.rs
##########
@@ -61,24 +63,27 @@ pub struct WindowAggExec {
/// Create a physical expression for window function
pub fn create_window_expr(
fun: &WindowFunction,
+ name: String,
args: &[Arc<dyn PhysicalExpr>],
+ _partition_by: &[Arc<dyn PhysicalExpr>],
+ _order_by: &[PhysicalSortExpr],
+ _window_frame: WindowFrame,
Review comment:
I just realized that this PR changes the behavior when there is a
non-empty partition_by/order_by/window_frame: before (at least in ballista) it
would error, whereas now it is silently ignored. Maybe it is worth erroring if
they aren't empty to make it explicit that there is no support?
--
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:
us...@infra.apache.org
