Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938228189
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -336,7 +369,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
None,
)?;
let order_by = (!order_by.is_empty()).then_some(order_by);
- let args = self.function_args_to_expr(args, schema,
planner_context)?;
+ let mut args =
+ self.function_args_to_expr(args, schema, planner_context)?;
+
+ let within_group = self.order_by_to_sort_expr(
+ within_group,
+ schema,
+ planner_context,
+ false,
+ None,
+ )?;
+
+ // add target column expression in within group clause to
function arguments
+ if !within_group.is_empty() {
+ args = within_group
+ .iter()
+ .map(|sort| sort.expr.clone())
+ .chain(args)
+ .collect::<Vec<_>>();
+ }
Review Comment:
The `Signature` of `ApproxPercentileCont` and
`ApproxPercentileContWithWeight` are not changed. Even though the `direct
argument (percentile)` is distinguished from the `aggregated argument (inside
WITHIN GROUP clause)` when calling function, those arguments need to be handled
as a whole during planning, in my opinion. That is because ordered set
aggregate functions are not working only with `direct argument (percentile)`,
without `WITHIN GROUP` clause.
Therefore, the expression in `WITHIN GROUP` clause is added into function
arguments while converting `sql expr` to `logical expr`.
**_If this approach doesn't seem like a good idea, please let me know._**
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
