Add dyn boolean kernels #1131
Add dyn boolean kernels #1131
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1131 +/- ##
=======================================
Coverage 82.33% 82.33%
=======================================
Files 169 169
Lines 50055 50115 +60
=======================================
+ Hits 41213 41264 +51
- Misses 8842 8851 +9
Continue to review full report at Codecov.
|
|
It's great, all the logic operations are in the pull request. |
| lt_bool_scalar(left, right) | ||
| } | ||
| _ => Err(ArrowError::ComputeError( | ||
| "Kernel only supports BooleanArray".to_string(), |
There was a problem hiding this comment.
It's better to add the operation name in the error message.
There was a problem hiding this comment.
Same comments for the below functions
| @@ -3599,4 +3674,59 @@ mod tests { | |||
| BooleanArray::from(vec![Some(false), Some(true), Some(false)]) | |||
| ); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_lt_dyn_bool_scalar() { | |||
There was a problem hiding this comment.
Can you add the null element in your test?
There was a problem hiding this comment.
sure - but can you expand on why? is there a specific concern on this test with nulls?
There was a problem hiding this comment.
NULL > FALSE -> NULL.
If left or right is NULL, the result must be NULL.
We should make sure this case is right.
|
|
||
| /// 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> { |
There was a problem hiding this comment.
I think we should update this to
| pub fn lt_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanArray> { | |
| 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 #1127
I am happy to make the change as a follow on PR as well.
There was a problem hiding this comment.
since this PR is for bool i think best to update the signatures in a follow on PR. to confirm though, this would be for all dyn scalar kernels right? scalar, utf8, and bool?
There was a problem hiding this comment.
since this PR is for bool i think best to update the signatures in a follow on PR. to confirm though, this would be for all dyn scalar kernels right? scalar, utf8, and bool?
Yes
However, I made the change for scalar and utf8 already in #1127 -- so how about I merge this PR and then I can make the updates to that PR as well
There was a problem hiding this comment.
sure sounds good
| } | ||
|
|
||
| /// Perform `left < right` operation on an array and a numeric scalar | ||
| /// value. Supports BooleanArrays, and DictionaryArrays that have string values |
There was a problem hiding this 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?
|
Thanks again @matthewmturner |
Which issue does this PR close?
Closes #1113 task dyn bool kernels
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?