alamb commented on code in PR #3405:
URL: https://github.com/apache/arrow-datafusion/pull/3405#discussion_r968289474
##########
datafusion/proto/src/lib.rs:
##########
@@ -1009,6 +1012,7 @@ mod roundtrip_tests {
let test_expr = Expr::AggregateUDF {
fun: Arc::new(dummy_agg.clone()),
args: vec![lit(1.0_f64)],
+ filter: Some(Box::new(lit(true))),
Review Comment:
👍
##########
datafusion/expr/src/expr.rs:
##########
@@ -251,6 +253,8 @@ pub enum Expr {
fun: Arc<AggregateUDF>,
/// List of expressions to feed to the functions as arguments
args: Vec<Expr>,
+ /// Optional filter
Review Comment:
```suggestion
/// Optional filter applied prior to aggregating
```
##########
datafusion/proto/src/to_proto.rs:
##########
@@ -639,17 +644,23 @@ impl TryFrom<&Expr> for protobuf::LogicalExprNode {
.collect::<Result<Vec<_>, Error>>()?,
})),
},
- Expr::AggregateUDF { fun, args } => Self {
- expr_type: Some(ExprType::AggregateUdfExpr(
- protobuf::AggregateUdfExprNode {
- fun_name: fun.name.clone(),
- args: args.iter().map(|expr|
expr.try_into()).collect::<Result<
- Vec<_>,
- Error,
- >>(
- )?,
- },
- )),
+ Expr::AggregateUDF { fun, args, filter } => {
+ Self {
+ expr_type: Some(ExprType::AggregateUdfExpr(
+ Box::new(protobuf::AggregateUdfExprNode {
+ fun_name: fun.name.clone(),
+ args: args.iter().map(|expr|
expr.try_into()).collect::<Result<
+ Vec<_>,
+ Error,
+ >>(
+ )?,
+ filter: match filter {
+ Some(e) =>
Some(Box::new(e.as_ref().try_into()?)),
+ None => None,
+ }
Review Comment:
I think you can write this kind of thing somewhat more concisely via
```suggestion
filter: filter
.map(|filter| Box::new(e.as_ref().try_into()))
.transpose()?
```
##########
datafusion/expr/src/expr.rs:
##########
@@ -231,6 +231,8 @@ pub enum Expr {
args: Vec<Expr>,
/// Whether this is a DISTINCT aggregation or not
distinct: bool,
+ /// Optional filter
Review Comment:
```suggestion
/// Optional filter applied prior to aggregating
```
##########
datafusion/sql/src/planner.rs:
##########
@@ -2053,6 +2053,16 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
Ok(Expr::ScalarFunction { fun, args })
}
+ SQLExpr::AggregateExpressionWithFilter { expr, filter } => {
+ match self.sql_expr_to_logical_expr(*expr, schema, ctes)? {
+ Expr::AggregateFunction {
+ fun, args, distinct, ..
+ } => Ok(Expr::AggregateFunction { fun, args, distinct,
filter: Some(Box::new(self.sql_expr_to_logical_expr(*filter, schema, ctes)?))
}),
+ _ => Err(DataFusionError::Internal("".to_string()))
Review Comment:
The error message here appears to be missing
--
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]
