[GitHub] [arrow-rs] alamb commented on a change in pull request #1131: Add dyn boolean kernels

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

alamb commented on a change in pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#discussion_r779111464



##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -1448,7 +1448,82 @@ pub fn eq_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanAr
             eq_bool_scalar(left, right)
         }
         _ => Err(ArrowError::ComputeError(
-            "Kernel only supports BooleanArray".to_string(),
+            "eq_dyn_bool_scalar only supports BooleanArray".to_string(),
+        )),
+    };
+    result
+}
+
+/// Perform `left < right` operation on an array and a numeric scalar
+/// value. Supports BooleanArrays, and DictionaryArrays that have string values
+pub fn lt_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanArray> {

Review comment:
       I think we should update this to 
   
   ```suggestion
   pub fn lt_dyn_bool_scalar(left: &dyn Array, right: bool) -> Result<BooleanArray> {
   ```
   
   Rather than taking an owned `Arc` to be consistent with the work in https://github.com/apache/arrow-rs/pull/1127
   
   I am happy to make the change as a follow on PR as well. 

##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -1448,7 +1448,82 @@ pub fn eq_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanAr
             eq_bool_scalar(left, right)
         }
         _ => Err(ArrowError::ComputeError(
-            "Kernel only supports BooleanArray".to_string(),
+            "eq_dyn_bool_scalar only supports BooleanArray".to_string(),
+        )),
+    };
+    result
+}
+
+/// Perform `left < right` operation on an array and a numeric scalar
+/// value. Supports BooleanArrays, and DictionaryArrays that have string values

Review comment:
       I don't think these kernels actually support `DictionaryArray` do they?  I also don't think that it is needed -- maybe we can just update the docstrings?




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