Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938224820
##########
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
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._**
##########
datafusion/functions-aggregate/src/approx_percentile_cont.rs:
##########
@@ -131,6 +147,19 @@ impl ApproxPercentileCont {
args: AccumulatorArgs,
) -> Result<ApproxPercentileAccumulator> {
let percentile = validate_input_percentile_expr(&args.exprs[1])?;
+
+ let is_descending = args
+ .ordering_req
+ .first()
+ .map(|sort_expr| sort_expr.options.descending)
+ .unwrap_or(false);
+
+ let percentile = if is_descending {
Review Comment:
I used floating point subtraction instead of actual sorting in reverse
order, for conciseness.
**_If any slight floating point difference is not permitted (even if this
branch passed the tests), 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: 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
