[GitHub] [arrow] alamb commented on a change in pull request #8463: ARROW-10163: [Rust] [DataFusion] Add DictionaryArray coercion support

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

alamb commented on a change in pull request #8463:
URL: https://github.com/apache/arrow/pull/8463#discussion_r504827840



##########
File path: rust/datafusion/tests/sql.rs
##########
@@ -1238,3 +1244,59 @@ async fn query_count_distinct() -> Result<()> {
     assert_eq!(expected, actual);
     Ok(())
 }
+

Review comment:
       This test demonstrates `DictionaryArray`s being used in DataFusion

##########
File path: rust/datafusion/src/physical_plan/expressions.rs
##########
@@ -1161,16 +1197,13 @@ fn order_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType>
         return Some(lhs_type.clone());
     }
 
-    match numerical_coercion(lhs_type, rhs_type) {
-        None => {
-            // strings are naturally ordered, and thus ordering can be applied to them.
-            string_coercion(lhs_type, rhs_type)
-        }
-        t => t,
-    }
+    numerical_coercion(lhs_type, rhs_type)

Review comment:
       this is just a refactor, it is not meant to change the semantics

##########
File path: rust/datafusion/src/physical_plan/expressions.rs
##########
@@ -1773,11 +1809,13 @@ mod tests {
 
     // runs an end-to-end test of physical type coercion:
     // 1. construct a record batch with two columns of type A and B
+    //  (*_ARRAY is the Rust Arrow array type, and *_TYPE is the DataType of the elements)
     // 2. construct a physical expression of A OP B
     // 3. evaluate the expression
     // 4. verify that the resulting expression is of type C
+    // 5. verify that the results of evaluation are $VEC
     macro_rules! test_coercion {
-        ($A_ARRAY:ident, $A_TYPE:expr, $A_VEC:expr, $B_ARRAY:ident, $B_TYPE:expr, $B_VEC:expr, $OP:expr, $TYPEARRAY:ident, $TYPE:expr, $VEC:expr) => {{

Review comment:
       I updated the parameter names here to make this macro more consistent with its documentation




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

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