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


##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -485,22 +485,31 @@ fn bitwise_or(left: ArrayRef, right: ArrayRef) -> 
Result<ArrayRef> {
     }
 }
 
-/// Use datafusion build-in expression `concat` to evaluate `StringConcat` 
operator
+/// Use datafusion build-in expression `concat` to evaluate `StringConcat` 
operator.
+/// Besides, any `NULL` exists on lhs or rhs will come out result `NULL`
+/// 1. 'a' || 'b' || 32 = 'ab32'
+/// 2. 'a' || NULL = NULL
 fn string_concat(left: ArrayRef, right: ArrayRef) -> Result<ArrayRef> {
-    // return NULL if left or right is NULL
-    for i in 0..left.len() {
-        if left.is_null(i) || right.is_null(i) {
-            return Ok(new_null_array(&DataType::Utf8, left.len()));
-        }
-    }
-    let result = string_expressions::concat(&[
-        ColumnarValue::Array(left),
-        ColumnarValue::Array(right),
-    ])?;
-    match result {
-        ColumnarValue::Array(array_ref) => Ok(array_ref),
-        scalar_value => Ok(scalar_value.into_array(1)),
-    }
+    let ignore_null = match string_expressions::concat(&[
+        ColumnarValue::Array(left.clone()),
+        ColumnarValue::Array(right.clone()),
+    ])? {
+        ColumnarValue::Array(array_ref) => array_ref,
+        scalar_value => scalar_value.into_array(left.clone().len()),
+    };
+    let ignore_null_array = 
ignore_null.as_any().downcast_ref::<StringArray>().unwrap();
+    let result = (0..ignore_null_array.len())
+        .into_iter()
+        .map(|index| {
+            if left.is_null(index) || right.is_null(index) {
+                None
+            } else {
+                Some(ignore_null_array.value(index))
+            }
+        })
+        .collect::<StringArray>();

Review Comment:
   As a future optimization it might be worth using the `take` kernel  
https://docs.rs/arrow/11.1.0/arrow/compute/kernels/take/fn.take.html which is 
fairly well optimized / tries to avoid copying.
   
   This is good for now though



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