-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: [9018]Fixed RunArray slice offsets(row, cast, eq) #9213
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
base: main
Are you sure you want to change the base?
Conversation
14f94b2 to
da01470
Compare
da01470 to
5842759
Compare
5842759 to
862378d
Compare
Jefffrey
left a comment
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 looks quite good; thanks for the comments explaining the logic in the equal implementation 👍
| let r_runs = r_end_phys - r_start_phys + 1; | ||
|
|
||
| if l_runs == r_runs { | ||
| // When the boundaries align perfectly, we don't need the complex stepping loop that calculates overlaps. |
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.
Thanks for these comments, they really helped my understanding of this code 👍
862378d to
9647a93
Compare
brancz
left a comment
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.
Very cool! I'm generally happy with this implementation, but without having thought about it too much, I think this implementation may not be very auto-vectorizable by the compiler. Maybe it's impossible, or even not important, since ranges are already covered, but perhaps something to think about for future optimizations.
Which issue does this PR close?
RunArrays #9018 .Rationale for this change
The existing implementation had issue for row or cast ops for RunArray. The equality implementation also did not support logical index-based comparisons.
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
No.