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
| pub fn shift(array: &Array, offset: i64) -> Result<ArrayRef> { | ||
| let value_len = array.len() as i64; | ||
| if offset == 0 { | ||
| Ok(array.slice(0, array.len())) |
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
| // Concatenate both arrays, add nulls after if shift > 0 else before | ||
| if offset > 0 { | ||
| concat(&[null_arr.as_ref(), slice.as_ref()]) | ||
| pub fn shift(array: &Array, offset: i64) -> Result<ArrayRef> { |
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
* 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 <Jimexist@users.noreply.github.com>
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?