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


##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -642,7 +662,7 @@ macro_rules! binary_array_op_dyn_scalar {
 
         let result: Result<Arc<dyn Array>> = match right {
             ScalarValue::Boolean(b) => compute_bool_op_dyn_scalar!($LEFT, b, 
$OP, $OP_TYPE),
-            ScalarValue::Decimal128(..) => compute_decimal_op_scalar!($LEFT, 
right, $OP, Decimal128Array),
+            ScalarValue::Decimal128(..) => 
compute_decimal_op_dyn_scalar!($LEFT, right, $OP, $OP_TYPE),

Review Comment:
   ❤️  you are starting to figure out the thicket that is `binary.rs` 👍 



##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -120,13 +120,33 @@ impl std::fmt::Display for BinaryExpr {
     }
 }
 
+macro_rules! compute_decimal_op_dyn_scalar {
+    ($LEFT:expr, $RIGHT:expr, $OP:ident, $OP_TYPE:expr) => {{
+        let ll = $LEFT.as_any().downcast_ref::<Decimal128Array>().unwrap();
+        if let ScalarValue::Decimal128(Some(_), _, _) = $RIGHT {
+            Ok(Arc::new(paste::expr! {[<$OP _decimal_scalar>]}(
+                ll,
+                $RIGHT.try_into()?,
+            )?))
+        } else {
+            // when the $RIGHT is a NULL, generate a NULL array of $OP_TYPE 
type
+            Ok(Arc::new(new_null_array($OP_TYPE, $LEFT.len())))
+        }
+    }};
+}
+
 macro_rules! compute_decimal_op_scalar {
     ($LEFT:expr, $RIGHT:expr, $OP:ident, $DT:ident) => {{
-        let ll = $LEFT.as_any().downcast_ref::<$DT>().unwrap();
-        Ok(Arc::new(paste::expr! {[<$OP _decimal_scalar>]}(
-            ll,
-            $RIGHT.try_into()?,
-        )?))
+        let ll = $LEFT.as_any().downcast_ref::<Decimal128Array>().unwrap();
+        if let ScalarValue::Decimal128(Some(_), _, _) = $RIGHT {
+            Ok(Arc::new(paste::expr! {[<$OP _decimal_scalar>]}(
+                ll,
+                $RIGHT.try_into()?,
+            )?))
+        } else {

Review Comment:
   Should we assert here that `$RIGHT` is still a decimal ? As written the code 
could convert `ScalarValue::Int8(Some(4))` into NULL, for example
   
   But I don't know if such a `ScalarValue` could be passed



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to