[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4352: Improve error handling and add some more types for proper downcasting

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

alamb commented on code in PR #4352:
URL: https://github.com/apache/arrow-datafusion/pull/4352#discussion_r1032783834


##########
datafusion/common/src/scalar.rs:
##########
@@ -720,7 +722,7 @@ fn get_dict_value<K: ArrowDictionaryKeyType>(
     array: &ArrayRef,
     index: usize,
 ) -> (&ArrayRef, Option<usize>) {
-    let dict_array = as_dictionary_array::<K>(array);
+    let dict_array = as_dictionary_array::<K>(array).unwrap();

Review Comment:
   for other reviewers, the rationale here is that arrow cast kernels used previously panic internally if the type is incorrect -- the ones in DataFusion return an error instead



##########
datafusion/physical-expr/src/string_expressions.rs:
##########
@@ -50,43 +53,6 @@ macro_rules! downcast_string_arg {
     }};
 }
 
-macro_rules! downcast_primitive_array_arg {

Review Comment:
   I reviewed the changes in this module carefully and they look good to me -- thank you @retikulum 



##########
datafusion/physical-expr/src/aggregate/median.rs:
##########
@@ -102,12 +103,7 @@ macro_rules! median {
             return Ok(ScalarValue::Null);
         }
         let sorted = sort(&combined, None)?;
-        let array = sorted
-            .as_any()
-            .downcast_ref::<PrimitiveArray<$TY>>()
-            .ok_or(DataFusionError::Internal(
-                "median! macro failed to cast array to expected type".to_string(),
-            ))?;
+        let array = as_primitive_array::<$TY>(&sorted)?;

Review Comment:
   much nicer



##########
datafusion/core/tests/custom_sources.rs:
##########
@@ -162,18 +163,10 @@ impl ExecutionPlan for CustomExecutionPlan {
                     .map(|i| ColumnStatistics {
                         null_count: Some(batch.column(*i).null_count()),
                         min_value: Some(ScalarValue::Int32(aggregate::min(
-                            batch
-                                .column(*i)
-                                .as_any()
-                                .downcast_ref::<PrimitiveArray<Int32Type>>()
-                                .unwrap(),
+                            as_primitive_array::<Int32Type>(batch.column(*i)).unwrap(),

Review Comment:
   👍 



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