alamb commented on code in PR #16897:
URL: https://github.com/apache/datafusion/pull/16897#discussion_r2229113084
##########
datafusion/physical-expr/src/aggregate.rs:
##########
@@ -616,10 +616,40 @@ impl AggregateFunctionExpr {
/// Returns `Some(Arc<dyn AggregateExpr>)` if re-write is supported,
otherwise returns `None`.
pub fn with_new_expressions(
&self,
- _args: Vec<Arc<dyn PhysicalExpr>>,
- _order_by_exprs: Vec<Arc<dyn PhysicalExpr>>,
+ args: Vec<Arc<dyn PhysicalExpr>>,
+ order_by_exprs: Vec<Arc<dyn PhysicalExpr>>,
) -> Option<AggregateFunctionExpr> {
- None
+ debug_assert_eq!(args.len(), self.args.len());
Review Comment:
Perhaps we can return `None` here if the lengths don't match up so in
production builds we don't get wrong results 🤔 Likewise below with the check on
order sensitivity
##########
datafusion/physical-expr/src/window/window_expr.rs:
##########
@@ -130,6 +130,12 @@ pub trait WindowExpr: Send + Sync + Debug {
/// Get the reverse expression of this [WindowExpr].
fn get_reverse_expr(&self) -> Option<Arc<dyn WindowExpr>>;
+ /// Creates a new instance of the window function evaluator.
Review Comment:
this is a new API too, right?
##########
datafusion/physical-expr/src/aggregate.rs:
##########
@@ -616,10 +616,40 @@ impl AggregateFunctionExpr {
/// Returns `Some(Arc<dyn AggregateExpr>)` if re-write is supported,
otherwise returns `None`.
pub fn with_new_expressions(
&self,
- _args: Vec<Arc<dyn PhysicalExpr>>,
- _order_by_exprs: Vec<Arc<dyn PhysicalExpr>>,
+ args: Vec<Arc<dyn PhysicalExpr>>,
+ order_by_exprs: Vec<Arc<dyn PhysicalExpr>>,
) -> Option<AggregateFunctionExpr> {
- None
+ debug_assert_eq!(args.len(), self.args.len());
+
+ if self.order_sensitivity() != AggregateOrderSensitivity::Insensitive {
+ debug_assert_eq!(order_by_exprs.len(), self.order_bys.len());
+ }
+ let new_order_bys = self
+ .order_bys
+ .iter()
+ .zip(order_by_exprs)
+ .map(|(req, new_expr)| PhysicalSortExpr {
+ expr: new_expr,
+ options: req.options,
+ })
+ .collect();
+
+ Some(AggregateFunctionExpr {
+ fun: self.fun.clone(),
+ args,
+ return_field: Arc::clone(&self.return_field),
+ // TODO: This name and display fields should be updated after
re-write to not mislead
Review Comment:
I am not 100% sure about this TODO - perhaps the `human_display` should be
changed, but maybe not the `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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
