[GitHub] [arrow] yordan-pavlov commented on a change in pull request #8660: ARROW-10173: [Rust][DataFusion] Implement support for direct comparison to scalar values

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

yordan-pavlov commented on a change in pull request #8660:
URL: https://github.com/apache/arrow/pull/8660#discussion_r523241911



##########
File path: rust/datafusion/src/physical_plan/expressions.rs
##########
@@ -1288,18 +1363,84 @@ impl PhysicalExpr for BinaryExpr {
         Ok(self.left.nullable(input_schema)? || self.right.nullable(input_schema)?)
     }
 
-    fn evaluate(&self, batch: &RecordBatch) -> Result<ArrayRef> {
-        let left = self.left.evaluate(batch)?;
-        let right = self.right.evaluate(batch)?;
-        if left.data_type() != right.data_type() {
+    fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+        let left_value = self.left.evaluate(batch)?;
+        let right_value = self.right.evaluate(batch)?;
+        let left_data_type = left_value.data_type();
+        let right_data_type = right_value.data_type();
+
+        if left_data_type != right_data_type {
             return Err(ExecutionError::General(format!(
                 "Cannot evaluate binary expression {:?} with types {:?} and {:?}",
-                self.op,
-                left.data_type(),
-                right.data_type()
+                self.op, left_data_type, right_data_type
             )));
         }
-        match &self.op {
+
+        let scalar_result = match (&left_value, &right_value) {
+            (ColumnarValue::Array(array), ColumnarValue::Scalar(scalar)) => {
+                // if left is array and right is literal - use scalar operations
+                let result: Result<ArrayRef> = match &self.op {
+                    Operator::Lt => binary_array_op_scalar!(array, scalar.clone(), lt),
+                    Operator::LtEq => {
+                        binary_array_op_scalar!(array, scalar.clone(), lt_eq)
+                    }
+                    Operator::Gt => binary_array_op_scalar!(array, scalar.clone(), gt),
+                    Operator::GtEq => {
+                        binary_array_op_scalar!(array, scalar.clone(), gt_eq)
+                    }
+                    Operator::Eq => binary_array_op_scalar!(array, scalar.clone(), eq),
+                    Operator::NotEq => {
+                        binary_array_op_scalar!(array, scalar.clone(), neq)
+                    }
+                    _ => Err(ExecutionError::General(format!(
+                        "Scalar values on right side of operator {} are not supported",
+                        self.op
+                    ))),
+                };
+                Some(result)
+            }
+            (ColumnarValue::Scalar(scalar), ColumnarValue::Array(array)) => {
+                // if right is literal and left is array - reverse operator and parameters
+                let result: Result<ArrayRef> = match &self.op {

Review comment:
       good question; the code blocks are not exactly the same, there is a small difference; notice how in `(ColumnarValue::Array(array), ColumnarValue::Scalar(scalar))` we have `Operator::Lt => binary_array_op_scalar!(array, scalar.clone(), lt)`, but under `(ColumnarValue::Scalar(scalar), ColumnarValue::Array(array))` we have `Operator::Lt => binary_array_op_scalar!(array, scalar.clone(), gt)`;
   this is because there is only one version of arrow comparison kernel functions for scalar comparison where the scalar value can only be on one side of the comparison, for example `pub fn lt_scalar<T>(left: &PrimitiveArray<T>, right: T::Native) -> Result<BooleanArray>`




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