alamb commented on code in PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#discussion_r1626296887


##########
datafusion-examples/examples/advanced_udaf.rs:
##########
@@ -412,7 +412,7 @@ async fn main() -> Result<()> {
     let df = ctx.table("t").await?;
 
     // perform the aggregation
-    let df = df.aggregate(vec![], vec![geometric_mean.call(vec![col("a")])])?;
+    let df = df.aggregate(vec![], 
vec![geometric_mean.call(vec![col("a")]).build()])?;

Review Comment:
   Sorry for the delay in reviewing this @jayzhan211  -- I have been thinking 
about this PR and I am struggling
   
   On one hand, having a builder allows for exposing the ability to provide 
other types of arguments to aggregate functions (e.g. `ORDER BY` or `FILTER`) 
   
   On the other hand, I feel like this API change (requiring a call to 
`build()` after `geometric_mean.call` makes DataFusion  harder to learn / use 
as most calls to aggregate functions are made with just the arguments. 
   
   I was thinking about what an ideal  user experience would be. What do you 
think about something that permits using the existing `expr_fn` API, but also 
allows easier construction of more advanced aggregate calls
   
   What about something like  this (as today, no need to call build)
   
   ```rust
       // geometric_mean(a)
       let agg = geometric_mean.call(vec![col("a")]);
       let df = df.aggregate(vec![], vec![agg])?;
   ```
   
   And then then to create an `ORDER BY` the user could create and use a 
builder like this:
   
   ```rust
       // geometric_mean(a FILTER b > c)
       let agg = geometric_mean.call(vec![col("a")])
         .agg_builder() // creates a builder that wraps an Expr, no error return
         .filter(col("b").gt(col("c"((
         .build()?; // check here if everything was ok
   
       let df = df.aggregate(vec![], vec![agg])?;
   ```
   
   
   We could probably implement a trait for `Expr` to add `agg_builder` (or 
maybe just add it directly)



-- 
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

Reply via email to