-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Avoid clones in make_array for StructArray and GenericByteViewArray
#9114
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
| fn from(value: ArrayData) -> Self { | ||
| let views = value.buffers()[0].clone(); | ||
| let views = ScalarBuffer::new(views, value.offset(), value.len()); | ||
| let buffers = value.buffers()[1..].to_vec(); |
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 call to_vec() allocates a new Vec which is unecessary
|
|
||
| impl<T: ByteViewType + ?Sized> From<ArrayData> for GenericByteViewArray<T> { | ||
| fn from(value: ArrayData) -> Self { | ||
| let views = value.buffers()[0].clone(); |
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.
cloneing the buffers is relatively cheap (they are Arcd internally) so avoiding this just makes the code easier to follow, I don't think it will be any significant performance savings
| make_array(cd.slice(parent_offset, parent_len)) | ||
| } else { | ||
| make_array(cd.clone()) | ||
| make_array(cd) |
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.
cd.clone() clones the ArrayData (and allocates a new Vec) for each child, recursively, which is unecessary
| .into_iter() | ||
| .map(|cd| { | ||
| if parent_offset != 0 || parent_len != cd.len() { | ||
| make_array(cd.slice(parent_offset, parent_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.
we can probably avoid an additional allocation for sliced arrays by making a version of slice() that consumes self -- like sliced() perhaps 🤔
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 filed a ticket to track this idea
|
Thanks @mhilton |
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, one nit
Co-authored-by: Jeffrey Vo <[email protected]>
|
BTW I plan to make the same corresponding change for other array types (see #9058) but I was making multiple PRs to reduce the review load |
|
@alamb -- post-merge reply to a resolved comment here (in case github didn't make it easy to find): |
…om `ArrayData` (#9156) # Which issue does this PR close? - part of #9061 - follow on #9114 # Rationale for this change @scovich noted in #9114 (comment) that calling `Vec::remove` does an extra copy and that `Vec::from` doesn't actually reuse the allocation the way I thought it did # What changes are included in this PR? Build the Arc for buffers directly # Are these changes tested? BY existing tests # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. -->
# Which issue does this PR close? - Part of #9061 - broken out of #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 #9114 # Are these changes tested? Existing tests # Are there any user-facing changes? No
…9160) # Which issue does this PR close? - Part of #9061 - broken out of #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 #9114 # Are these changes tested? Existing tests # Are there any user-facing changes? No
…ray` (apache#9114) # Which issue does this PR close? - Part of apache#9061 - broken out of apache#9058 # Rationale for this change The current implementation of `make_array` for StructArray and GenericByteViewArray clones `ArrayData` which allocates a new Vec. This is unnecessary given that `make_array` is passed an owned ArrayData # What changes are included in this PR? 1. Add a new API to ArrayData to break it down into parts (`into_parts`) 2. Use that API to avoid cloning while constructing StructArray and GenericByteViewArray # Are these changes tested? Yes by CI # Are there any user-facing changes? A few fewer allocations when creating arrays --------- Co-authored-by: Jeffrey Vo <[email protected]>
…om `ArrayData` (apache#9156) # Which issue does this PR close? - part of apache#9061 - follow on apache#9114 # Rationale for this change @scovich noted in apache#9114 (comment) that calling `Vec::remove` does an extra copy and that `Vec::from` doesn't actually reuse the allocation the way I thought it did # What changes are included in this PR? Build the Arc for buffers directly # Are these changes tested? BY existing tests # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. -->
# 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
…pache#9160) # 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? - Part of #9061 - broken out of #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 #9114 # Are these changes tested? Existing tests # Are there any user-facing changes? No
# Which issue does this PR close? - Part of #9061 - broken out of #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 #9114 # Are these changes tested? Existing tests # Are there any user-facing changes? No
# Which issue does this PR close? - Part of #9061 - broken out of #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 #9114 # Are these changes tested? Existing tests # Are there any user-facing changes? No
# Which issue does this PR close? - Part of #9061 - broken out of #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 #9114 # Are these changes tested? Existing tests # Are there any user-facing changes? No
# Which issue does this PR close? - Part of #9061 - broken out of #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 #9114 # Are these changes tested? Existing tests # Are there any user-facing changes? No
# Which issue does this PR close? - Part of #9061 - broken out of #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 #9114 # Are these changes tested? Existing tests # Are there any user-facing changes? No
# Which issue does this PR close? - Part of #9061 - broken out of #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 #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
The current implementation of
make_arrayfor StructArray and GenericByteViewArray clonesArrayDatawhich allocates a new Vec. This is unnecessary given thatmake_arrayis passed an owned ArrayDataWhat changes are included in this PR?
into_parts)Are these changes tested?
Yes by CI
Are there any user-facing changes?
A few fewer allocations when creating arrays