Add NaN handling in dyn scalar comparison kernels #2830
Conversation
|
I wonder if it might be cleaner to use a trait for this that can be derived for T::Native, much like we do for arithmetic? Using the dyn downcasting machinery feels like a bit of a hack... |
Hm, okay. Yea, current approach not looks very clear due to downcasting and try_cast. |
arrow/src/datatypes/native.rs
Outdated
| @@ -46,6 +46,7 @@ pub(crate) mod native_op { | |||
| + Div<Output = Self> | |||
| + Rem<Output = Self> | |||
| + Zero | |||
| + num::ToPrimitive | |||
There was a problem hiding this comment.
Because kernels like eq_dyn_scalar have this type binding. In macro dyn_compare_scalar, ToPrimitive api is used on the input scalar.
There was a problem hiding this comment.
But the dyn_scalar kernels don't use ArrowNativeTypeOp?
There was a problem hiding this comment.
I put is_eq, is_ne..APIs into ArrowNativeTypeOp. If I don't add this addition, the compilier complains:
error[E0599]: no method named `to_f64` found for type parameter `T` in the current scope
--> arrow/src/compute/kernels/comparison.rs:1228:50
|
1228 | let right = try_to_type!($RIGHT, to_f64)?;
| ^^^^^^ method not found in `T`
...
1338 | pub fn eq_dyn_scalar<T>(left: &dyn Array, right: T) -> Result<BooleanArray>
| - method `to_f64` not found for this type parameter
...
1340 | T: ArrowNativeTypeOp + num::ToPrimitive,
| ++++++++++++++++++
So I pull the type bound num::ToPrimitive into ArrowNativeTypeOp.
I made all dyn_scalar kernels use ArrowNativeTypeOp now.
There was a problem hiding this comment.
Oh I see, could we possibly have the dyn_scalar kernels just have the constraint on ToPrimitive + ArrowNativeTypeOp instead of unifying them.
This not only avoids this leaking to the arithmetic kernels, which will complicate porting decimals over, but it also seems wrong that the dyn kernels are performing type coercion on the scalar value at all, and I would like to keep the door open to removing this down the line.
There was a problem hiding this comment.
Hmm, found that it is not necessary to add the addition. Removed it now.
There was a problem hiding this comment.
Thank you - FWIW I filed #2837 to deal with the somewhat surprising, at least to me, behaviour of these scalar kernels
|
The failed CI is for labeling only. |
|
Thanks @tustvold for reviewing. |
|
Benchmark runs are scheduled for baseline = f8c4037 and contender = c93ce39. c93ce39 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 #2829.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?