-
Notifications
You must be signed in to change notification settings - Fork 810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add more tests for window::shift and handle boundary cases #386
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #386 +/- ##
==========================================
+ Coverage 82.62% 82.64% +0.01%
==========================================
Files 162 162
Lines 44275 44310 +35
==========================================
+ Hits 36582 36618 +36
+ Misses 7693 7692 -1 ☔ View full report in Codecov by Sentry. |
efa5781
to
a5a70ff
Compare
arrow/src/compute/kernels/window.rs
Outdated
let nulls = abs(offset as i64) as usize; | ||
let value_len = values.len() as i64; | ||
if offset == 0 { | ||
Ok(values.slice(0, values.len())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as #388
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess it's not that easy - there's no easy way to clone array and likely suboptimal than just taking a slice here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment, LGTM otherwise
64defcc
to
daf1655
Compare
/// let expected: Int32Array = vec![None, Some(4), None].into(); | ||
/// assert_eq!(res.as_ref(), &expected); | ||
/// | ||
/// // shift array 0 element, although not recommended |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one looked good to me. Epic tests @jimexist
FWIW I could not cherry-pick this automatically to
If we want to include it we will have to backport it manually |
* add more doc test for window::shift * handle i64::MIN first * use Ok(make_array(array.data_ref().clone()))
Second chance back porting has worked #429 👍 |
Which issue does this PR close?
Closes #387.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?