Remove deprecated comparison kernels #4739
Conversation
|
@tustvold like you suggested, this PR updates all tests in place for the convenience of review. take a look when you have time. |
zinking
changed the title
| vec![6, 7, 8, 9, 10, 6, 7, 8, 9, 10], | ||
| vec![false, false, true, false, false, false, false, true, false, false] | ||
| ); | ||
| macro_rules! test_array_scalar_op { |
There was a problem hiding this comment.
I know there were test macros before, but perhaps we could take the opportunity to remove them. I find it really hard to follow what these tests are doing now...
| let a: BooleanArray = | ||
| vec![Some(true), Some(false), Some(false), Some(true), Some(true), None] | ||
| .into(); | ||
| let b: BooleanArray = | ||
| vec![Some(true), Some(true), Some(false), Some(false), None, Some(false)] | ||
| .into(); | ||
|
|
||
| let res: Vec<Option<bool>> = lt_bool(&a, &b).unwrap().iter().collect(); | ||
|
|
||
| assert_eq!( | ||
| res, | ||
| vec![Some(false), Some(true), Some(false), Some(false), None, None] | ||
| ) | ||
| test_boolean_array_array_op!(crate::cmp::lt, "lt") |
There was a problem hiding this comment.
I think it would be much easier to follow this if it simply was something like
let a = BooleanArray::from(vec![...]);
let b = BooleanArray::from(vec![...]);
let expected = BooleanArray::from(vec![...]);
assert_eq(lt(&a, &b).unwrap(), expected);
| @@ -2643,122 +2646,103 @@ mod tests { | |||
| ); | |||
| } | |||
|
|
|||
| fn test_primitive_dyn_scalar<T: ArrowPrimitiveType>(array: PrimitiveArray<T>) { | |||
There was a problem hiding this comment.
I much prefer generics to macros, they are easier to read, debug and maintain...
|
I'm okay with removing macros as long as codes are not duplicated everywhere. I did try to come up with generics to make a corresponding scalar in |
|
Marking this as a draft to make clear it isn't awaiting review, feel free to unmark when you would like me to take another look |
Which issue does this PR close?
Closes #4733 Remove Deprecated Comparison Kernels
Rationale for this change
What changes are included in this PR?
this is the first part of the change, which updates all tests of the old kernel tests
Are there any user-facing changes?
NO