timsaucer commented on code in PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#discussion_r1685742655
##########
datafusion/expr/src/expr_fn.rs:
##########
@@ -676,6 +679,266 @@ pub fn interval_month_day_nano_lit(value: &str) -> Expr {
Expr::Literal(ScalarValue::IntervalMonthDayNano(interval))
}
+/// Extensions for configuring [`Expr::AggregateFunction`]
+///
+/// Adds methods to [`Expr`] that make it easy to set optional aggregate
options
+/// such as `ORDER BY`, `FILTER` and `DISTINCT`
+///
+/// # Example
+/// ```no_run
+/// # use datafusion_common::Result;
+/// # use datafusion_expr::{AggregateUDF, col, Expr, lit};
+/// # use sqlparser::ast::NullTreatment;
+/// # fn count(arg: Expr) -> Expr { todo!{} }
+/// # fn first_value(arg: Expr) -> Expr { todo!{} }
+/// # fn main() -> Result<()> {
+/// use datafusion_expr::ExprFunctionExt;
+///
+/// // Create COUNT(x FILTER y > 5)
+/// let agg = count(col("x"))
+/// .filter(col("y").gt(lit(5)))
+/// .build()?;
+/// // Create FIRST_VALUE(x ORDER BY y IGNORE NULLS)
+/// let sort_expr = col("y").sort(true, true);
+/// let agg = first_value(col("x"))
+/// .order_by(vec![sort_expr])
+/// .null_treatment(NullTreatment::IgnoreNulls)
+/// .build()?;
+/// # Ok(())
+/// # }
+/// ```
+pub trait ExprFunctionExt {
Review Comment:
Here is an alternate approach:
https://github.com/apache/datafusion/pull/11582
In that PR instead of using a builder structure, each of these calls like
`partition_by(expr)` instead returns a `Result<Expr>`.
I think this makes it pretty ergonomic, but it is an API change.
From one of the unit tests we have the old method:
```
async fn test_aggregate_ext_filter() {
let agg = first_value_udaf()
.call(vec![col("i")])
.order_by(vec![col("i").sort(true, true)])
.filter(col("i").is_not_null())
.build()
.unwrap()
.alias("val");
```
And the new method:
```
async fn test_aggregate_ext_filter() -> Result<()> {
let agg = first_value_udaf()
.call(vec![col("i")])
.order_by(vec![col("i").sort(true, true)])?
.filter(col("i").is_not_null())?
.alias("val");
```
*Maybe* it doesn't make a lot of difference, which is why I put it on a
different branch. If we want to stick with the builder method, then this PR is
ready to go assuming we come to agreement on if this should be a single trait
or broken into two for Aggregate vs Window.
--
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
