alamb commented on a change in pull request #1357:
URL: https://github.com/apache/arrow-datafusion/pull/1357#discussion_r757884258
##########
File path: datafusion/src/physical_plan/aggregates.rs
##########
@@ -93,90 +93,111 @@ impl FromStr for AggregateFunction {
}
}
-/// Returns the datatype of the scalar function
-pub fn return_type(fun: &AggregateFunction, arg_types: &[DataType]) ->
Result<DataType> {
+/// Returns the datatype of the aggregation function
+pub fn return_type(
+ fun: &AggregateFunction,
+ input_expr_types: &[DataType],
+) -> Result<DataType> {
// Note that this function *must* return the same type that the respective
physical expression returns
// or the execution panics.
// verify that this is a valid set of data types for this function
- data_types(arg_types, &signature(fun))?;
+ data_types(input_expr_types, &signature(fun))?;
match fun {
AggregateFunction::Count | AggregateFunction::ApproxDistinct => {
Ok(DataType::UInt64)
}
- AggregateFunction::Max | AggregateFunction::Min =>
Ok(arg_types[0].clone()),
- AggregateFunction::Sum => sum_return_type(&arg_types[0]),
- AggregateFunction::Avg => avg_return_type(&arg_types[0]),
+ AggregateFunction::Max | AggregateFunction::Min => {
+ Ok(input_expr_types[0].clone())
+ }
+ AggregateFunction::Sum => sum_return_type(&input_expr_types[0]),
+ AggregateFunction::Avg => avg_return_type(&input_expr_types[0]),
AggregateFunction::ArrayAgg => Ok(DataType::List(Box::new(Field::new(
"item",
- arg_types[0].clone(),
+ input_expr_types[0].clone(),
true,
)))),
}
}
-/// Create a physical (function) expression.
-/// This function errors when `args`' can't be coerced to a valid argument
type of the function.
+/// Create a physical aggregation expression.
+/// This function errors when `input_phy_exprs`' can't be coerced to a valid
argument type of the aggregation function.
pub fn create_aggregate_expr(
fun: &AggregateFunction,
distinct: bool,
- args: &[Arc<dyn PhysicalExpr>],
+ input_phy_exprs: &[Arc<dyn PhysicalExpr>],
Review comment:
I happen to think `input_phy_exprs` is more clear and I prefer it,
however the rest of the code uses `arg` or `args` pretty extensively to refer
to the input arguments of functions/aggregates/ etc as `args`
I do like how the difference between `Expr` and `PhysicalExpr` is now
clearer to the reader without having to look at the definition
--
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]
