Support dictionary array for divide kernel #1983
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1983 +/- ##
==========================================
- Coverage 83.62% 83.43% -0.19%
==========================================
Files 222 222
Lines 57763 57979 +216
==========================================
+ Hits 48304 48375 +71
- Misses 9459 9604 +145
Continue to review full report at Codecov.
|
|
The clippy errors seems not caused by this change. I will fix them separately if it is reproducible locally. |
|
cc @sunchao This is more complicated. |
| typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict) | ||
| } | ||
| _ => typed_math_op!(left, right, |a, b| a / b, math_checked_divide_op), |
There was a problem hiding this comment.
I extracted the called op function as a parameter.
| }, | ||
| ); | ||
| } else { | ||
| Ok(T::default_value()) |
There was a problem hiding this comment.
curious why we are using default value here
There was a problem hiding this comment.
This follows the divide kernel did before. I think for the case (left or right is None), the value is not important, so it just put a default value there.
| right: impl Iterator<Item = Option<T::Native>>, | ||
| op: F, | ||
| len: usize, | ||
| null_bit_buffer: Option<Buffer>, |
There was a problem hiding this comment.
nit: maybe we can just pass a boolean flag here
There was a problem hiding this comment.
We still need null_bit_buffer.
| T::Native: One + Zero, | ||
| F: Fn(T::Native, T::Native) -> T::Native, | ||
| { | ||
| math_dict_divide_checked_op!(left, right, op, PrimitiveArray<T>) |
There was a problem hiding this comment.
why we need a macro for this? can we just inline it?
| /// then the result is also null. If any right hand value is zero then the result of this | ||
| /// operation will be `Err(ArrowError::DivideByZero)`. | ||
| pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> { | ||
| match left.data_type() { |
There was a problem hiding this comment.
what if right is not a dictionary? are we going to support it too?
There was a problem hiding this comment.
Yea, I will implement these supports later.
|
Thanks @sunchao |
Which issue does this PR close?
Closes #1982.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?