jayzhan211 commented on PR #10560:
URL: https://github.com/apache/datafusion/pull/10560#issuecomment-2153499258
> This is really cool @jayzhan211 -- other than some documentation and
examples I think this looks great
>
> I left some other comments inline
>
> One thing that I think might be confusing with this API is that this would
not error:
>
> ```rust
> use AggregateUDFExprBuilder;
> let expr = col("a")
> .order_by(col("b"))
> ```
>
> I wonder if we could make it looks more like this:
>
> ```rust
> let expr = col("a")
> .order_by(col("b"))
> .build()?
> ```
>
> To have a change to throw an error
>
> So a more full featured example would look like
>
> ```rust
> // FIRST_VALUE(ORDER BY b FILTER c > 1)
> let expr = first_value("a")
> .order_by(col("b"))
> .filter(col("c").gt(lit(1)) // note no ? here
> .build()? // only ? here
> ```
I'm not sure if we really need the error check since we don't have any now
```rust
pub fn new_udf(
udf: Arc<crate::AggregateUDF>,
args: Vec<Expr>,
distinct: bool,
filter: Option<Box<Expr>>,
order_by: Option<Vec<Expr>>,
null_treatment: Option<NullTreatment>,
) -> Self {
Self {
func_def: AggregateFunctionDefinition::UDF(udf),
args,
distinct,
filter,
order_by,
null_treatment,
}
}
```
What should we do in `build() -> Result<Expr>`?
--
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
