alamb commented on code in PR #20366:
URL: https://github.com/apache/datafusion/pull/20366#discussion_r2886493872
##########
datafusion/functions-aggregate/src/approx_percentile_cont_with_weight.rs:
##########
@@ -45,18 +45,18 @@ create_func!(
/// Computes the approximate percentile continuous with weight of a set of
numbers
pub fn approx_percentile_cont_with_weight(
- order_by: Sort,
+ expr: Expr,
Review Comment:
does this change mean we lose the ability to specify the `nulls_first` and
`nulls_last` option? If so, maybe the function should take in `SortOptions`
directly 🤔
##########
docs/source/library-user-guide/upgrading/53.0.0.md:
##########
@@ -398,3 +398,124 @@ Now:
+-------+
0 row(s) fetched.
```
+
+### 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.
+
+**Who is affected:**
+
+- Users constructing ordered-set aggregate expressions using the DataFrame API.
+
+SQL syntax is unaffected.
+
+**Migration guide:**
+
+- Replace calls that construct a `Sort` expression (for example
+ `col("foo").sort(true, false)`) with the value expression directly and
+ provide the `asc` boolean parameter instead.
+
+#### percentile_cont
+
+**Before:**
+
+```rust,ignore
+pub fn percentile_cont(order_by: Sort, percentile: Expr) -> Expr
+
+let expr = percentile_cont(col("foo").sort(true, false), lit(0.5));
+```
+
+**After:**
+
+```rust,ignore
+pub fn percentile_cont(expr: Expr, percentile: Expr, asc: bool) -> Expr
+
+let expr = percentile_cont(col("foo"), lit(0.5), true);
+```
+
+#### approx_percentile_cont
+
+**Before:**
+
+```rust,ignore
+pub fn approx_percentile_cont(
Review Comment:
The same comment applies to the other examples above and below. However, in
those cases since it is only a single line I don't think it is as noticable
##########
docs/source/library-user-guide/upgrading/53.0.0.md:
##########
@@ -398,3 +398,124 @@ Now:
+-------+
0 row(s) fetched.
```
+
+### 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.
+
+**Who is affected:**
+
+- Users constructing ordered-set aggregate expressions using the DataFrame API.
+
+SQL syntax is unaffected.
+
+**Migration guide:**
+
+- Replace calls that construct a `Sort` expression (for example
+ `col("foo").sort(true, false)`) with the value expression directly and
+ provide the `asc` boolean parameter instead.
+
+#### percentile_cont
+
+**Before:**
+
+```rust,ignore
+pub fn percentile_cont(order_by: Sort, percentile: Expr) -> Expr
+
+let expr = percentile_cont(col("foo").sort(true, false), lit(0.5));
+```
+
+**After:**
+
+```rust,ignore
+pub fn percentile_cont(expr: Expr, percentile: Expr, asc: bool) -> Expr
+
+let expr = percentile_cont(col("foo"), lit(0.5), true);
+```
+
+#### approx_percentile_cont
+
+**Before:**
+
+```rust,ignore
+pub fn approx_percentile_cont(
Review Comment:
I think reiterating the function signature here is
1. somewhat redundant and makes for a hard to read upgrade guide.
2. obscures the change that a user must make to their code
I think it would be better I think if we removed the `pub fn
approx_percentile_cont...` from here and the **AFTER** section below
--
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]
