You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "timsaucer (via GitHub)" <gi...@apache.org> on 2024/07/19 13:10:06 UTC
[PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
timsaucer opened a new pull request, #11550:
URL: https://github.com/apache/datafusion/pull/11550
## Which issue does this PR close?
Closes #6747.
## Rationale for this change
There was excellent work done on https://github.com/apache/datafusion/pull/10560 that makes a much more easy to use interface for developers to set parameters on aggregate functions. This PR is designed to bring the same interface to window functions.
Currently it can be very clunky to write a window function for a relatively simple query. For example to create a lag function you would do:
```
let lag_expr = Expr::WindowFunction(WindowFunction::new(
WindowFunctionDefinition::BuiltInWindowFunction(
BuiltInWindowFunction::Lag,
),
vec![col("array_col")],
vec![],
vec![],
WindowFrame::new(None),
None,
));
```
With this PR it becomes a simpler
```
let lag_expr = lag(col("array_col"));
```
Even more powerful is that you can now convert window function expressions into their builder to set additional properties
```
let lag_expr = lag(col("array_col")).null_treatment(NullTreatment::IgnoreNulls).build()?;
```
## What changes are included in this PR?
This PR refactors `AggregateExt` to be `ExprFunctionExt` and adds in additional functions for `partition_by` and `window_frame`. It also adds in a variety of useful helper functions for things like `lead`, `lag`, `row_number` etc.
## Are these changes tested?
All existing CI tests pass. Some were updated to use the new interface, but all of the functionality remains the same.
## Are there any user-facing changes?
`WindowFunction::new` now only requires two parameters, the function definition and arguments. Other parameters will need to be set using the builder methods provided. For example, if the user was previously calling
```
let max1 = Expr::WindowFunction(expr::WindowFunction::new(
WindowFunctionDefinition::AggregateFunction(AggregateFunction::Max),
vec![col("name")],
vec![],
vec![age_asc.clone(), name_desc.clone()],
WindowFrame::new(Some(false)),
None,
));
```
They would change this to
```
let max1 = Expr::WindowFunction(expr::WindowFunction::new(
WindowFunctionDefinition::AggregateFunction(AggregateFunction::Max),
vec![col("name")]))
.order_by(vec![age_asc.clone(), name_desc.clone()])
.build()
.unwrap();
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#discussion_r1688043042
##########
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`]
Review Comment:
Updated
##########
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(())
+/// # }
+/// ```
Review Comment:
Added
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2241088854
marking as draft as it sounds like @timsaucer has some additional ideas
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2243056293
I guess the chain of `?` is probably the reason why I choose builder, so we have `build()` for checking validity at once.
But I would like to optimize for `user experience` on these APIs, if directly mutating `Expr` is actually easier and straightforward to use. We should enable it.
How about we keep both 😆 🤔
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2247727501
I removed `first_value` and `last_value` so that it doesn't create confusion. Ready for review.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#discussion_r1688132417
##########
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 {
+ /// Add `ORDER BY <order_by>`
+ ///
+ /// Note: `order_by` must be [`Expr::Sort`]
+ fn order_by(self, order_by: Vec<Expr>) -> ExprFuncBuilder;
+ /// Add `FILTER <filter>`
+ fn filter(self, filter: Expr) -> ExprFuncBuilder;
+ /// Add `DISTINCT`
+ fn distinct(self) -> ExprFuncBuilder;
+ /// Add `RESPECT NULLS` or `IGNORE NULLS`
+ fn null_treatment(
+ self,
+ null_treatment: impl Into<Option<NullTreatment>>,
+ ) -> ExprFuncBuilder;
+ // Add `PARTITION BY`
+ fn partition_by(self, partition_by: Vec<Expr>) -> ExprFuncBuilder;
+ // Add appropriate window frame conditions
+ fn window_frame(self, window_frame: WindowFrame) -> ExprFuncBuilder;
+}
+
+#[derive(Debug, Clone)]
+pub enum ExprFuncKind {
+ Aggregate(AggregateFunction),
+ Window(WindowFunction),
+}
+
+/// Implementation of [`ExprFunctionExt`].
+///
+/// See [`ExprFunctionExt`] for usage and examples
+#[derive(Debug, Clone)]
+pub struct ExprFuncBuilder {
+ fun: Option<ExprFuncKind>,
Review Comment:
> Since we are sticking with the builder approach, don't we need this in case someone attempts to create a builder from an Expr that is not aggregate or window?
>
> ```
> col("a").order_by(vec![col("b")])
> ```
>
> This is a wrong use but won't be caught until we call `.build()` so up to that point don't we need the `fun` field to be None?
Makes sense, it is for incorrect usage
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2246554340
> Question: it looks like we have some functions that have both aggregate functions and window functions defined. Specifically looking at `first_value` and `last_value`. My thought is that the current PR just keeps with the existing pattern but I think we should merge these two, right? Should all aggregate functions be viable window functions?
>
> Right now I have `datafusion_expr::window_function::first_value()` will return a window function expression of type `BuiltInWindowFunction::FirstValue` and `datafusion_functions_aggregate::first_last::first_value()` will return an aggregate expression of type `AggregateUDF` and `datafusion_functions_aggregate::first_last::FirstValue`.
>
> My thinking is that we should stick with this PR for now but open an issue to converge all of these. I would expect the following psuedocode to be equivalent
>
> ```
> let window_expr = datafusion_expr::window_function::first_value(col("a"));
> let agg_expr = datafusion_functions_aggregate::first_last::first_value(col("a"));
> ```
>
> Please correct me if I'm missing something here.
I hadn't look into the detail of window_first implementation, but I guess we could reuse the function.
I guess things get easier after udwf is introduced https://github.com/apache/datafusion/issues/8709.
We could keep them separate for now.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2239199650
I don't think I have permission on this repo to add labels to this PR so I wasn't able to add one for having user facing changes
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#discussion_r1686928326
##########
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`]
Review Comment:
We could perhaps update this comment to refer to Window functions as well
##########
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(())
+/// # }
+/// ```
Review Comment:
I think it would also be awesome to add an example here for a window function too (with partition by)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2243508166
> I had tried to directly modify Expr before, but ends up builder API. I couldn't remember the reason
> My guess is that it has to do with all of the other functions returning Expr rather than Result<Expr> and to fit neatly in with the rest of the code base
I think another reason was that it might be easier to understand what APIs could be called wnen
for example, if we created `Expr::order_by` there are many Exprs this doesn't make sense for (like `Expr::Like` for example). By making a builder then the APIs used for creating aggregate expressions can be in a single trait and documented with examples, etc
> But I would like to optimize for user experience on these APIs, if directly mutating Expr is actually easier and straightforward to use
I agree with this (though I do think if we change the API we should try to avoid any more churn than necessary).
I would tend to favor @timsaucer 's opinion on what is easier / more intuitive. I think I am too close to DataFusion / Rust these days to really know what is easier from a "someone who is new to the datafusion codebase" perspective
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#discussion_r1687794069
##########
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 {
+ /// Add `ORDER BY <order_by>`
+ ///
+ /// Note: `order_by` must be [`Expr::Sort`]
+ fn order_by(self, order_by: Vec<Expr>) -> ExprFuncBuilder;
+ /// Add `FILTER <filter>`
+ fn filter(self, filter: Expr) -> ExprFuncBuilder;
+ /// Add `DISTINCT`
+ fn distinct(self) -> ExprFuncBuilder;
+ /// Add `RESPECT NULLS` or `IGNORE NULLS`
+ fn null_treatment(
+ self,
+ null_treatment: impl Into<Option<NullTreatment>>,
+ ) -> ExprFuncBuilder;
+ // Add `PARTITION BY`
+ fn partition_by(self, partition_by: Vec<Expr>) -> ExprFuncBuilder;
+ // Add appropriate window frame conditions
+ fn window_frame(self, window_frame: WindowFrame) -> ExprFuncBuilder;
+}
+
+#[derive(Debug, Clone)]
+pub enum ExprFuncKind {
+ Aggregate(AggregateFunction),
+ Window(WindowFunction),
+}
+
+/// Implementation of [`ExprFunctionExt`].
+///
+/// See [`ExprFunctionExt`] for usage and examples
+#[derive(Debug, Clone)]
+pub struct ExprFuncBuilder {
+ fun: Option<ExprFuncKind>,
Review Comment:
I guess fun is the `must have` field, option is not neccessary. `fun: ExprFuncKind`
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2245192874
Question: it looks like we have some functions that have both aggregate functions and window functions defined. Specifically looking at `first_value` and `last_value`. My thought is that the current PR just keeps with the existing pattern but I think we should merge these two, right? Should all aggregate functions be viable window functions?
Right now I have `datafusion_expr::window_function::first_value()` will return a window function expression of type `BuiltInWindowFunction::FirstValue` and `datafusion_functions_aggregate::first_last::first_value()` will return an aggregate expression of type `AggregateUDF` and `datafusion_functions_aggregate::first_last::FirstValue`.
My thinking is that we should stick with this PR for now but open an issue to converge all of these. I would expect the following psuedocode to be equivalent
```
let window_expr = datafusion_expr::window_function::first_value(col("a"));
let agg_expr = datafusion_functions_aggregate::first_last::first_value(col("a"));
```
Please correct me if I'm missing something here.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#discussion_r1690275071
##########
datafusion/expr/src/expr.rs:
##########
@@ -769,7 +769,52 @@ impl fmt::Display for WindowFunctionDefinition {
}
}
+impl From<aggregate_function::AggregateFunction> for WindowFunctionDefinition {
+ fn from(value: aggregate_function::AggregateFunction) -> Self {
+ Self::AggregateFunction(value)
+ }
+}
+
+impl From<BuiltInWindowFunction> for WindowFunctionDefinition {
+ fn from(value: BuiltInWindowFunction) -> Self {
+ Self::BuiltInWindowFunction(value)
+ }
+}
+
+impl From<Arc<crate::AggregateUDF>> for WindowFunctionDefinition {
+ fn from(value: Arc<crate::AggregateUDF>) -> Self {
+ Self::AggregateUDF(value)
+ }
+}
+
+impl From<Arc<WindowUDF>> for WindowFunctionDefinition {
+ fn from(value: Arc<WindowUDF>) -> Self {
+ Self::WindowUDF(value)
+ }
+}
+
/// Window function
+///
Review Comment:
❤️
##########
datafusion/expr/src/lib.rs:
##########
@@ -16,6 +16,8 @@
// under the License.
// Make cheap clones clear: https://github.com/apache/datafusion/issues/11143
#![deny(clippy::clone_on_ref_ptr)]
+// TODO When the deprecated trait AggregateExt is removed, remove this unstable feature.
Review Comment:
It seems like this feature requires nightly rust, which isn't possible for DataFusion I don't think. I'll push a commit to this PR that removes the attempt at backwards compatibility and just updated the description
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2240823467
Actually maybe hold off on reviewing. I have an idea for the morning that might make it simpler
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2242694088
Copying @jayzhan211 's comment from
> I had tried to directly modify Expr before, but ends up builder API. I couldn't remember the reason
My *guess* is that it has to do with all of the other functions returning `Expr` rather than `Result<Expr>` and to fit neatly in with the rest of the code base, you wanted one point of checking? Aside from that for Aggregate I don't see a strong need to have the builder. Actually, window functions have a better argument for them with the fact that you need to (sometimes) build the window frame if it is not specified.
The reasons I can see sticking with a builder function instead of updating the `Expr` directly:
1. Minimal change to the API. It *should* only require updating the import since all of the signatures in `AggregateExt` are identical in `ExprFunctionExt` and there are only new methods added in. We could even keep `AggregateExt` and mark it deprecated for a release or two.
2. Builder allows checking for errors at one point instead of needing to do `?` after every operation. Right now there are two types of errors in `AggregateExt`, calling one of the functions on an Expr that is not an aggregate function or setting and `order_by` expression that is not a `Sort`.
3. As mentioned above, it actually does make setting up the window functions cleaner. With the window functions, if the user specifies a window frame we want to use that. If not, we want to create it based on a few criteria. This is easier with a builder function so we can perform this operation once. In my alternate approach, I update `WindowsFunction` to make the frame optional. Then at the point of consuming the window function data I perform a `get_frame_or_default()` that I added in. However these uses I don't expect to be something the end user experiences and is mostly upon the DataFusion code to maintain.
The reasons I can see preferring to work directly with the `Expr`:
1. I think this does create a better user experience in the long run. We can perform our checks along the way, which I think is more idiomatic. Plus it relieves the user of needing to understand that we're going from an expression into a builder of an expression.
2. The code to do this is in most ways simpler. We can remove the trait entirely, and simply have functions that are implemented on `Expr`. The notable exception to the simplification is the window frame on window functions mentioned above.
Laying it out like this, I can see a good argument for sticking with the builder function if that's the way you two (and others) would like to go.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#discussion_r1687978397
##########
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 {
+ /// Add `ORDER BY <order_by>`
+ ///
+ /// Note: `order_by` must be [`Expr::Sort`]
+ fn order_by(self, order_by: Vec<Expr>) -> ExprFuncBuilder;
+ /// Add `FILTER <filter>`
+ fn filter(self, filter: Expr) -> ExprFuncBuilder;
+ /// Add `DISTINCT`
+ fn distinct(self) -> ExprFuncBuilder;
+ /// Add `RESPECT NULLS` or `IGNORE NULLS`
+ fn null_treatment(
+ self,
+ null_treatment: impl Into<Option<NullTreatment>>,
+ ) -> ExprFuncBuilder;
+ // Add `PARTITION BY`
+ fn partition_by(self, partition_by: Vec<Expr>) -> ExprFuncBuilder;
+ // Add appropriate window frame conditions
+ fn window_frame(self, window_frame: WindowFrame) -> ExprFuncBuilder;
+}
+
+#[derive(Debug, Clone)]
+pub enum ExprFuncKind {
+ Aggregate(AggregateFunction),
+ Window(WindowFunction),
+}
+
+/// Implementation of [`ExprFunctionExt`].
+///
+/// See [`ExprFunctionExt`] for usage and examples
+#[derive(Debug, Clone)]
+pub struct ExprFuncBuilder {
+ fun: Option<ExprFuncKind>,
Review Comment:
Since we are sticking with the builder approach, don't we need this in case someone attempts to create a builder from an Expr that is not aggregate or window?
```
col("a").order_by(vec![col("b")])
```
This is a wrong use but won't be caught until we call `.build()` so up to that point don't we need the `fun` field to be None?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2243520969
Thanks @timsaucer and @jayzhan211 -- this is looking pretty sweet
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2240809551
> I don't think I have permission on this repo to add labels to this PR so I wasn't able to add one for having user facing changes
Thanks @timsaucer -- I added the `api-change` label. This PR looks amazing. I plan to review it soon (I have a bunch of PRs to review tomorrow). Looking forward to it ❤️
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#discussion_r1686922713
##########
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:
> The one problem I see there is that we would end up with Expr implementing two traits with identical function calls, which would be less user friendly.
I agree that is not a good user experience
> 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.
I wonder if we could do something like this:
```rust
trait WindowExt: AggregateExt {
...
}
```
which is kind of like Rust's inheritance -- it basically says that anything that implements `WindowExt` also implements `AggregateExt`
And then the builder could be extended for window specific functions (and perhaps `build` made to error if the underlying aggregate function couldn't be used as a window function)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent API to set parameters of aggregate and window functions (`AggregateExt` --> `ExprFunctionExt`) [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #11550:
URL: https://github.com/apache/datafusion/pull/11550
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent API for parameters of aggregate functions and window functions (remove `AggregateExt` add `ExprFunctionExt`) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#discussion_r1690279970
##########
datafusion-examples/examples/simple_udwf.rs:
##########
@@ -118,12 +118,12 @@ async fn main() -> Result<()> {
df.show().await?;
// Now, run the function using the DataFrame API:
- let window_expr = smooth_it.call(
- vec![col("speed")], // smooth_it(speed)
- vec![col("car")], // PARTITION BY car
- vec![col("time").sort(true, true)], // ORDER BY time ASC
- WindowFrame::new(None),
- );
+ let window_expr = smooth_it
Review Comment:
this is so much nicer
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent API for parameters of aggregate functions and window functions (remove `AggregateExt` add `ExprFunctionExt`) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2248725172
I am so excited for this PR -- thank you @timsaucer -- I pushed a few more commits to improve the docs a bit more and get the CI passing. I think this is a really nice improvement to the usability of DataFusion
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent API to set parameters of aggregate and window functions (`AggregateExt` --> `ExprFunctionExt`) [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2249081403
Looks great! Thanks @timsaucer and @alamb
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2245490998
From ^ comment I think things like the helper function `first_value()` and such are adding technical debt. I'm going to dig in tomorrow. If we're updating the window functions now we might as well clean this up at the same time.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2243519481
I looked at https://github.com/apache/datafusion/pull/11582 and after reading the rest of the commentary on this PR I think this PR's proposal is best approach
Thus I suggest:
1. Get this PR ready for review
2. Add some sort of backwards compatibility thing to help migration
Backwards compatibilty: basically give a deprecation message that tells users how to upgrade
For example, something like
```rust
// Compatibility: ensure existing users of AggregateExt get a message about ExprFunctionExt
// Note the syntax isn't quite right for the deprecated macro
#[deprecated(version = 40, message=:"ExprFunctionExt instead")]
trait AggregateExt: ExprFunctionExt {}
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#discussion_r1685420813
##########
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:
The one problem I see there is that we would end up with Expr implementing two traits with identical function calls, which would be less user friendly.
```
pub trait AggregateExt {
fn order_by(self, order_by: Vec<Expr>) -> AggregateBuilder;
...
}
pub trait WindowExt {
fn order_by(self, order_by: Vec<Expr>) -> WindowBuilder;
...
}
impl AggregateExt for Expr {
fn order_by(self, order_by: Vec<Expr>) -> AggregateBuilder {
...
}
}
impl WindowExt for Expr {
fn order_by(self, order_by: Vec<Expr>) -> WindowBuilder {
...
}
}
```
Then when the user goes to create their aggregate or window I would expect an error on having multiple methods in scope or the user would have to specify which trait they're working with. Right?
On a slightly separate topic, One thing I was trying to figure out this morning was if we need a builder structure. Would it be simpler to have each of these return an Expr? I can see one issue in that we would want to generate some errors if the user called one of these functions on an Expr that is inappropriate. I was thinking we could do something along the lines of
```
impl Expr {
pub fn order_by(mut self, order_by: impl Into<Option<Vec<Expr>>>) -> Result<Expr> {
match &mut self {
Expr::AggregateFunction(udaf) => {
udaf.order_by = order_by.into();
}
Expr::WindowFunction(udwf) => {
udwf.order_by = order_by.into()
}
_ => {
return plan_err!("Unable to set `order_by` on an Expression that is not an AggregateFunction or WindowFunction.");
}
}
Ok(self)
}
...
}
```
Then the user can do something like
```
let agg = first_value(col("x")).order_by(vec![sort_expr])?
```
But maybe that's less ergonomic than keeping with the builder structure.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Consistent approach to setting parameters on aggregate functions and window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11550:
URL: https://github.com/apache/datafusion/pull/11550#discussion_r1685381472
##########
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:
Rather than trying to unify treatment of aggregate and window functions (which are almost, but not quite the same), it might be worth considering creating a `WindowExt` that mirrors closely the `AggregateExt`
While they would likely have substantial duplication I think the code might be simpler to reason about and use
Also, it would not be an API change, which is an added bonus
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org