Split out arrow-arith (#2594)#3384
Conversation
| @@ -17,18 +17,19 @@ | |||
|
|
|||
| //! Defines temporal kernels for time and date related functions. | |||
There was a problem hiding this comment.
These aren't technically arithmetic kernels, but they are similar in spirit, and putting them in arrow-cast would bring in a load of additional stuff
| ArrowNativeTypeOp, ArrowNumericType, DataType, Date32Type, Date64Type, | ||
| IntervalDayTimeType, IntervalMonthDayNanoType, IntervalUnit, IntervalYearMonthType, | ||
| }; | ||
| #[cfg(feature = "dyn_arith_dict")] |
There was a problem hiding this comment.
what changed about the feature guards that allows for this simplification?
There was a problem hiding this comment.
Combination of wildcard imports and explicitly scoping a couple of types
| .borrow_mut() | ||
| .zip(left_chunks.borrow_mut().zip(right_chunks.borrow_mut())), | ||
| ) | ||
| .zip((&mut result_chunks).zip((&mut left_chunks).zip(&mut right_chunks))) |
There was a problem hiding this comment.
this doesn't need mut references, is that the difference?
There was a problem hiding this comment.
It avoids needing BorrowMut in scope 😅
| pub mod arity; | ||
| pub mod bitwise; | ||
| pub mod boolean; | ||
| pub mod limit; |
There was a problem hiding this comment.
what is the deal with limit? Is it going away too?
There was a problem hiding this comment.
It is deprecated and can probably be removed at some point - it just calls through to Array::slice
|
Benchmark runs are scheduled for baseline = 13e0b87 and contender = e7fc073. e7fc073 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
| pub mod bitwise; | ||
| pub mod boolean; | ||
| pub mod temporal; |
There was a problem hiding this comment.
Maybe we can have arrow-misc crate to hold such kernels which are not exactly arithmetic or comparison kernels? I'm fine to put them in arrow-arith though.
4 participants
Which issue does this PR close?
Closes #2594
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?