-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix: Allow broadcasting in ranges
#11900
Conversation
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.
Instead of broadcasting the input arrays and increasing memory, can we add branches where we resolve the broadcasting optimally. This is how we deal with broadcasting in most places and makes it most efficient.
You can do that quite elegantly by creating a function that accepts two different iterators. Where for the broadcasting branches one iterator is the iterator over the array and the other is tstd::iter::repeat
.
Right, I just stole the existing logic from |
Any news on this one? |
36b0f2d
to
4acdc4d
Compare
aa6ea75
to
11f184a
Compare
let start = start.downcast_iter().next().unwrap(); | ||
let end = end.downcast_iter().next().unwrap(); | ||
|
||
// First do a pass to determine the required value capacity. |
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 removed this pass and set a default capacity of 5 times the length of the column, similar to the other ranges
functions. If this capacity calculation must be preserved, I have to make some adjustments as it relied on fully materializing start/end.
11f184a
to
80200bb
Compare
80200bb
to
79fdc5b
Compare
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.
Almost there. :)
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.
Nice one @stinodego. It looks great and nice that we are optimal now in all branches. :)
Closes #11892
Also changes the way capacity is allocated for
int_ranges
.