-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Avoid a clone when creating BooleanArray from ArrayData
#9159
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
Conversation
scovich
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.
LGTM
| assert_eq!( | ||
| data.buffers().len(), | ||
| buffers.len(), | ||
| 1, | ||
| "BooleanArray data should contain a single buffer only (values buffer)" | ||
| ); | ||
| let values = BooleanBuffer::new(data.buffers()[0].clone(), data.offset(), data.len()); | ||
| let buffer = buffers.pop().expect("checked above"); |
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.
aside: it's a bit annoying that we can't just do let [buffer] = buffers else { ... }
(let [buffer] = buffers[..] else { ... } produces a slice ref that would match but only capture a borrow instead of consuming it like we want here)
etseidl
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.
LGTM2!
|
Avoid all the clones! |
# Which issue does this PR close? - Part of apache#9061 - broken out of apache#9058 # Rationale for this change Let's make arrow-rs the fastest we can and the fewer allocations the better # What changes are included in this PR? Apply pattern from apache#9114 # Are these changes tested? Existing tests # Are there any user-facing changes? No
Which issue does this PR close?
make_array) #9061ArrayData(speed up reading from Parquet reader) #9058Rationale for this change
Let's make arrow-rs the fastest we can and the fewer allocations the better
What changes are included in this PR?
Apply pattern from #9114
Are these changes tested?
Existing tests
Are there any user-facing changes?
No