alamb commented on code in PR #20366:
URL: https://github.com/apache/datafusion/pull/20366#discussion_r2988268272
##########
datafusion/functions-aggregate/src/percentile_cont.rs:
##########
@@ -76,9 +76,9 @@ const INTERPOLATION_PRECISION: f64 = 1_000_000.0;
create_func!(PercentileCont, percentile_cont_udaf);
/// Computes the exact percentile continuous of a set of numbers
-pub fn percentile_cont(order_by: Sort, percentile: Expr) -> Expr {
- let expr = order_by.expr.clone();
- let args = vec![expr, percentile];
+pub fn percentile_cont(expr: Expr, percentile: Expr, asc: bool) -> Expr {
Review Comment:
What is the rationale for this change? It seems like the new API doesn't
permit specifying nulls first / nulls last 🤔
##########
docs/source/library-user-guide/upgrading/54.0.0.md:
##########
@@ -91,6 +91,95 @@ fn apply_expressions(
Ok(tnr)
}
+### Ordered-set aggregate DataFrame API simplified
+
+The DataFrame APIs for the following ordered-set aggregate functions have
changed:
+
+- `percentile_cont`
+- `approx_percentile_cont`
+- `approx_percentile_cont_with_weight`
+
+Previously, these functions required a `Sort` expression as input.
+They now accept the value expression `expr` directly along with an `asc: bool`
+parameter to specify sort direction.
+
+This change simplifies the API and removes the need to manually construct
+a `Sort` expression.
Review Comment:
It seems to me like it makes the API more complicated as there are now more
parameters ?
--
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]
