Remove redundant result of AggregateFunctionExpr::field (apache#12258)

korowa · Sep 1, 2024 · 016ed03 · 016ed03
1 parent 442d9bf
commit 016ed03
Show file tree

Hide file tree

Showing 6 changed files with 12 additions and 21 deletions.
diff --git a/datafusion/physical-expr/src/aggregate.rs b/datafusion/physical-expr/src/aggregate.rs
@@ -253,12 +253,8 @@ impl AggregateFunctionExpr {
     }
 
     /// the field of the final result of this aggregation.
-    pub fn field(&self) -> Result<Field> {
-        Ok(Field::new(
-            &self.name,
-            self.data_type.clone(),
-            self.is_nullable,
-        ))
+    pub fn field(&self) -> Field {
+        Field::new(&self.name, self.data_type.clone(), self.is_nullable)
     }
 
     /// the accumulator used to accumulate values from the expressions.
@@ -523,9 +519,7 @@ impl AggregateFunctionExpr {
     ///
     /// Note: this is used to use special aggregate implementations in certain conditions
     pub fn get_minmax_desc(&self) -> Option<(Field, bool)> {
-        self.fun
-            .is_descending()
-            .and_then(|flag| self.field().ok().map(|f| (f, flag)))
+        self.fun.is_descending().map(|flag| (self.field(), flag))
     }
 
     /// Returns default value of the function given the input is Null

diff --git a/datafusion/physical-expr/src/window/aggregate.rs b/datafusion/physical-expr/src/window/aggregate.rs
@@ -79,7 +79,7 @@ impl WindowExpr for PlainAggregateWindowExpr {
     }
 
     fn field(&self) -> Result<Field> {
-        self.aggregate.field()
+        Ok(self.aggregate.field())
     }
 
     fn name(&self) -> &str {
@@ -177,7 +177,7 @@ impl AggregateWindowExpr for PlainAggregateWindowExpr {
     ) -> Result<ScalarValue> {
         if cur_range.start == cur_range.end {
             self.aggregate
-                .default_value(self.aggregate.field()?.data_type())
+                .default_value(self.aggregate.field().data_type())
         } else {
             // Accumulate any new rows that have entered the window:
             let update_bound = cur_range.end - last_range.end;

diff --git a/datafusion/physical-expr/src/window/sliding_aggregate.rs b/datafusion/physical-expr/src/window/sliding_aggregate.rs
@@ -81,7 +81,7 @@ impl WindowExpr for SlidingAggregateWindowExpr {
     }
 
     fn field(&self) -> Result<Field> {
-        self.aggregate.field()
+        Ok(self.aggregate.field())
     }
 
     fn name(&self) -> &str {
@@ -183,7 +183,7 @@ impl AggregateWindowExpr for SlidingAggregateWindowExpr {
     ) -> Result<ScalarValue> {
         if cur_range.start == cur_range.end {
             self.aggregate
-                .default_value(self.aggregate.field()?.data_type())
+                .default_value(self.aggregate.field().data_type())
         } else {
             // Accumulate any new rows that have entered the window:
             let update_bound = cur_range.end - last_range.end;

diff --git a/datafusion/physical-optimizer/src/aggregate_statistics.rs b/datafusion/physical-optimizer/src/aggregate_statistics.rs
@@ -183,7 +183,7 @@ fn take_optimizable_min(
                 // MIN/MAX with 0 rows is always null
                 if is_min(agg_expr) {
                     if let Ok(min_data_type) =
-                        ScalarValue::try_from(agg_expr.field().unwrap().data_type())
+                        ScalarValue::try_from(agg_expr.field().data_type())
                     {
                         return Some((min_data_type, agg_expr.name().to_string()));
                     }
@@ -229,7 +229,7 @@ fn take_optimizable_max(
                 // MIN/MAX with 0 rows is always null
                 if is_max(agg_expr) {
                     if let Ok(max_data_type) =
-                        ScalarValue::try_from(agg_expr.field().unwrap().data_type())
+                        ScalarValue::try_from(agg_expr.field().data_type())
                     {
                         return Some((max_data_type, agg_expr.name().to_string()));
                     }

diff --git a/datafusion/physical-plan/src/aggregates/mod.rs b/datafusion/physical-plan/src/aggregates/mod.rs
@@ -821,7 +821,7 @@ fn create_schema(
         | AggregateMode::SinglePartitioned => {
             // in final mode, the field with the final result of the accumulator
             for expr in aggr_expr {
-                fields.push(expr.field()?)
+                fields.push(expr.field())
             }
         }
     }

diff --git a/datafusion/proto/src/physical_plan/mod.rs b/datafusion/proto/src/physical_plan/mod.rs
@@ -1444,11 +1444,8 @@ impl AsExecutionPlan for protobuf::PhysicalPlanNode {
             let agg_names = exec
                 .aggr_expr()
                 .iter()
-                .map(|expr| match expr.field() {
-                    Ok(field) => Ok(field.name().clone()),
-                    Err(e) => Err(e),
-                })
-                .collect::<Result<_>>()?;
+                .map(|expr| expr.name().to_string())
+                .collect::<Vec<_>>();
 
             let agg_mode = match exec.mode() {
                 AggregateMode::Partial => protobuf::AggregateMode::Partial,