Use downcast_primitive_array in arithmetic kernels
#2640
Conversation
|
Did a benchmark run and no obvious difference found. |
| @@ -225,6 +225,255 @@ macro_rules! downcast_primitive_array { | |||
| $($p => $fallback,)* | |||
| } | |||
| }; | |||
|
|
|||
| (($values1:ident, $values2:ident) => $e:block $($p:pat => $fallback:expr $(,)*)*) => { | |||
There was a problem hiding this comment.
This has an implicit assumption that $values1 and $values2 have the same type, not only is this potentially surprising as an API, but I think it changes the behaviour of the kernels which will now panic where previously they would return an error?
Adding $values2.data_type() to the match might work, but this still feels a bit confusing as an API? 🤔
I wonder if we could instead do something like this
downcast_primitive_array!(
left => {
let right = as_primitive_array(right);
multiply(left, right).map(|a| Arc::new(a) as ArrayRef)
}
_ => Err(ArrowError::CastError(format!(
"Unsupported data type {}, {}",
left.data_type(), right.data_type()
)))
)
And rely on the fact the generic kernels constrain them to be the same type. I don't know, perhaps this is hack...
There was a problem hiding this comment.
Ah, good point. But I guess the suggested one will also panic on let right = as_primitive_array(right); if right is not same type?
Let me do a test.
There was a problem hiding this comment.
Hmm, math_op doesn't constrain two sides of op should be same type. So
downcast_primitive_array!(
left => {
let right = as_primitive_array(right);
math_op(left, right, |a, b| a + b).map(|a| Arc::new(a) as ArrayRef)
}
_ => Err(ArrowError::CastError(format!(
"Unsupported data type {}, {}",
left.data_type(), right.data_type()
)))
)will not constrain the right side type.
There was a problem hiding this comment.
suggested one will
Yeah, you will still need to check the type, it is what it is
doesn't constrain the type
That's why i suggested using the generic kernel not math_op directly 😃
There was a problem hiding this comment.
Oh oh right. 😄
Good news is it works.
But as Float16Type doesn't implement ArrowNumericType, I need to remove Float16Type pattern from downcast_primitive_array! to make it work.
For simd feature, seems f16 related APIs are not available so it appears not easy to let Float16Type implement ArrowNumericType.
Currently I leave single argument downcast_primitive_array! untouched and stick with two argument one and make it constrain the two arguments must be same type.
|
Btw very cool to see these being used already ❤️ |
|
Benchmark runs are scheduled for baseline = b46fc92 and contender = 30ab9bb. 30ab9bb is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2639.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?