Refactor aggregate function handling #8358
Conversation
github-actions
bot
added
sql
Weijun-H
force-pushed
the
udf-aggregation
branch
2 times, most recently
from
cb469eb
to
95a868f
Compare
There was a problem hiding this comment.
Thank you @Weijun-H -- this looks great. I had some code style suggestion that could reduce duplication but I don't think they are necessary as the duplication already exists and we could potentially do it as a follow on (though this PR would be stronger 🦾 if they were done too)
It looks like there are some conflicts to resolve
But all in all, great work and thank you
| let mut names = Vec::with_capacity(args.len()); | ||
| for e in args { | ||
| names.push(create_physical_name(e, false)?); | ||
| } |
There was a problem hiding this comment.
I know it is the same as was here, but I think you can write this more concisely like
| let mut names = Vec::with_capacity(args.len()); | |
| for e in args { | |
| names.push(create_physical_name(e, false)?); | |
| } | |
| let names = args.iter() | |
| .map(|e| create_physical_name(e, false)) | |
| .collect::<Result<Vec<_>>()? |
| )?), | ||
| None => None, | ||
| }; | ||
| let order_by = match order_by { |
There was a problem hiding this comment.
I think it would be possible to do the order_by and filter conversions once rather than for both arms
datafusion/expr/src/expr.rs
Outdated
| @@ -477,11 +475,36 @@ impl Sort { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | |||
| /// Defines which implementation of a function for DataFusion to call. | |||
There was a problem hiding this comment.
| /// Defines which implementation of a function for DataFusion to call. | |
| /// Defines which implementation of an aggregate function DataFusion should call. |
datafusion/expr/src/expr.rs
Outdated
| fun: aggregate_function::AggregateFunction, | ||
| name: Arc<str>, | ||
| }, | ||
| /// Resolved to a user defined function |
There was a problem hiding this comment.
| /// Resolved to a user defined function | |
| /// Resolved to a user defined aggregate function |
datafusion/expr/src/expr.rs
Outdated
| }) => match func_def { | ||
| AggregateFunctionDefinition::BuiltIn { fun: _, name: _ } | ||
| | AggregateFunctionDefinition::Name(_) => { | ||
| let mut name = create_function_name(func_def.name(), *distinct, args)?; |
There was a problem hiding this comment.
You might be able to combine the arms together here (the AggregateFunctionDefinition::UDF appears to do the same thing, though I realize it is not entirely the same)
datafusion/expr/src/expr_schema.rs
Outdated
| Expr::AggregateFunction(AggregateFunction { func_def, args, .. }) => { | ||
| match func_def { | ||
| AggregateFunctionDefinition::BuiltIn { fun, .. } => { | ||
| let data_types = args |
There was a problem hiding this comment.
The data_types calculation could be hoisted above as well
Weijun-H
force-pushed
the
udf-aggregation
branch
from
95a868f
to
99273de
Compare
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| /// Defines which implementation of an aggregate function DataFusion should call. | ||
| pub enum AggregateFunctionDefinition { | ||
| BuiltIn(aggregate_function::AggregateFunction), |
|
Since this is an API change I will leave it open for another day before merging. Really nice work @Weijun-H |
Weijun-H
force-pushed
the
udf-aggregation
branch
from
1fb0b0c
to
3da4d54
Compare
Weijun-H
force-pushed
the
udf-aggregation
branch
from
3da4d54
to
441ca13
Compare
|
Thanks again @Weijun-H -- this is really nice |
Which issue does this PR close?
Closes #8346
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?