[GitHub] [arrow-datafusion] houqp commented on a change in pull request #1357: Change the arg names and make parameters more meaningful

[GitBox <gi...@apache.org>]

houqp commented on a change in pull request #1357:
URL: https://github.com/apache/arrow-datafusion/pull/1357#discussion_r760753124



##########
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>],
     input_schema: &Schema,
     name: impl Into<String>,
 ) -> Result<Arc<dyn AggregateExpr>> {
     let name = name.into();
-    let arg = coerce(args, input_schema, &signature(fun))?;
-    if arg.is_empty() {
+    let coerced_phy_exprs = coerce(input_phy_exprs, input_schema, &signature(fun))?;
+    if coerced_phy_exprs.is_empty() {
         return Err(DataFusionError::Plan(format!(
             "Invalid or wrong number of arguments passed to aggregate: '{}'",
             name,
         )));
     }
-    let arg = arg[0].clone();
+    let first_coerced_phy_expr = coerced_phy_exprs[0].clone();
 
-    let arg_types = args
+    let coerced_exprs_types = coerced_phy_exprs

Review comment:
       ha, ok, so what we had in `physical_plan/functions.rs` was wrong :D




-- 
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@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org