[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6785: Consistently coerce dictionaries for arithmetic

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

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


##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -2968,16 +2962,10 @@ mod tests {
         op: Operator,
         expected: BooleanArray,
     ) -> Result<()> {
-        let left_type = left.data_type();
-        let right_type = right.data_type();
-        let (lhs, rhs) = get_input_types(left_type, &op, right_type)?;
-
-        let left_expr = try_cast(col("a", schema)?, schema, lhs)?;
-        let right_expr = try_cast(col("b", schema)?, schema, rhs)?;
-        let arithmetic_op = binary_simple(left_expr, op, right_expr, schema);
+        let op = binary_op(col("a", schema)?, op, col("b", schema)?, schema)?;

Review Comment:
   this is a much nicer formulation



##########
datafusion/expr/src/type_coercion/binary.rs:
##########
@@ -226,13 +226,13 @@ fn math_decimal_coercion(
     use arrow::datatypes::DataType::*;
 
     match (lhs_type, rhs_type) {
-        (Dictionary(key_type, value_type), _) => {
+        (Dictionary(_, value_type), _) => {

Review Comment:
   maybe it is worth a comment here explaining we are expanding out dictionaries to the underlying type and leave a comment pointing back at this PR / a summary of https://github.com/apache/arrow-datafusion/pull/6785#issuecomment-1612957624
   
   I think that is critical context



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