window::shift to work for all array types#388
Conversation
Codecov Report
@@ Coverage Diff @@
## master #388 +/- ##
==========================================
+ Coverage 82.61% 82.65% +0.03%
==========================================
Files 162 162
Lines 44228 44283 +55
==========================================
+ Hits 36538 36600 +62
+ Misses 7690 7683 -7
Continue to review full report at Codecov.
|
arrow/src/compute/kernels/window.rs
Outdated
There was a problem hiding this comment.
If offset == 0, why not return array.clone()? array.slice does incur small overhead as it might have to descend into the array children for nested types (see #389 )
There was a problem hiding this comment.
I guess it's not that easy given Array does not derive Copy
There was a problem hiding this comment.
You're right. This works: Ok(make_array(array.data_ref().clone()))
Going this route avoids:
- checking the offset and length
- recomputing the null count
arrow/src/compute/kernels/window.rs
Outdated
There was a problem hiding this comment.
This looks great! May you please add tests for struct and list types.
jimexist
force-pushed
the
shift-array-all-cases
branch
from
2764be8 to
70ad27c
Compare
jimexist
force-pushed
the
shift-array-all-cases
branch
from
74d44b2 to
7d95d8c
Compare
|
Do you think this one is ready now @nevi-me ? |
* add more doc test for window::shift * use Ok(make_array(array.data_ref().clone())) * shift array for not only primitive cases * include more test cases * add back copied * fix renaming
* add more doc test for window::shift * use Ok(make_array(array.data_ref().clone())) * shift array for not only primitive cases * include more test cases * add back copied * fix renaming Co-authored-by: Jiayu Liu <[email protected]>
6 participants
Which issue does this PR close?
Closes #392 .
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?