[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5560: refactor: add more error info when array is empty

["Jefffrey (via GitHub)" <gi...@apache.org>]

Jefffrey commented on code in PR #5560:
URL: https://github.com/apache/arrow-datafusion/pull/5560#discussion_r1133331511


##########
datafusion/physical-expr/src/aggregate/median.rs:
##########
@@ -151,7 +151,12 @@ impl Accumulator for MedianAccumulator {
                 // ignore null values
                 .filter(|v| !v.is_null())
                 .cloned(),
-        )?;
+        )
+        .map_err(|_| {
+            DataFusionError::Execution(
+                "median requires at least one non-null value".into(),
+            )
+        })?;

Review Comment:
   Does this have the possibility of incorrectly overwriting other errors that might have occurred in `iter_to_array`?



##########
datafusion/physical-expr/src/aggregate/approx_median.rs:
##########
@@ -67,7 +67,11 @@ impl AggregateExpr for ApproxMedian {
     }
 
     fn create_accumulator(&self) -> Result<Box<dyn Accumulator>> {
-        self.approx_percentile.create_accumulator()
+        self.approx_percentile.create_accumulator().map_err(|_| {
+            DataFusionError::Execution(
+                "approx median requires at least one non-null value".to_string(),
+            )
+        })

Review Comment:
   I'm not sure if this wrapping will prevent having an empty input? Could you add some tests to check this?



-- 
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