implement eq_dyn, neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
#858
Conversation
jimexist
changed the title
eq_dyn, neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
jimexist
force-pushed
the
impl-eq-neq-dyn
branch
from
84250ae
to
7179a44
Compare
Codecov Report
@@ Coverage Diff @@
## master #858 +/- ##
==========================================
- Coverage 82.66% 82.65% -0.02%
==========================================
Files 168 168
Lines 48172 48196 +24
==========================================
+ Hits 39823 39834 +11
- Misses 8349 8362 +13
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I think this looks really nice @jimexist -- thank you!
FWIW I took a friendly look at what arrow2 does for this situation and I think this PR is not inconsistent. Arrow2 appears to have a function called arithmetic which does the type dispatch on the parameter types and an Operator enum:
https://github.com/jorgecarleitao/arrow2/blob/main/src/compute/arithmetics/mod.rs#L91
While an Operator enum seems like a reasonable thing to consider adding to arrow-rs it is also likely fine to do as a follow on (as we already have different kernels for different operations)
| @@ -1245,11 +1431,17 @@ mod tests { | |||
| /// `EXPECTED` can be either `Vec<bool>` or `Vec<Option<bool>>`. | |||
| /// The main reason for this macro is that inputs and outputs align nicely after `cargo fmt`. | |||
| macro_rules! cmp_i64 { | |||
| ($KERNEL:ident, $A_VEC:expr, $B_VEC:expr, $EXPECTED:expr) => { | |||
| ($KERNEL:ident, $DYN_KERNEL:ident, $A_VEC:expr, $B_VEC:expr, $EXPECTED:expr) => { | |||
jimexist
force-pushed
the
impl-eq-neq-dyn
branch
from
e6c66aa
to
08b53e5
Compare
Co-authored-by: Jiayu Liu <[email protected]>
Which issue does this PR close?
Closes #843
Rationale for this change
to reduce casting on the call sites
What changes are included in this PR?
implement:
eq_dynneq_dynlt_dynlt_eq_dyngt_dyngt_eq_dynAre there any user-facing changes?