-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Avoid a clone when creating ListViewArray from ArrayData
#9193
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
f7d8a97 to
72d0fcc
Compare
| ))); | ||
| } | ||
|
|
||
| let values = data.child_data()[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.
here is a clone of ArrayData (which requires an extra Vec allocaton) which is no longer needed
|
@brancz sorry I meant to tag you on this PR given your effort working on ListViewArray recently |
|
I'm confused, this looks to be the same change as the one for |
That is a great catch -- thank you -- fixed |
| )) | ||
| })?; | ||
|
|
||
| let values = data.child_data()[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.
This saves a real clone of ArrayData (and thus an allocation)
| let value_sizes = ScalarBuffer::new(sizes_buffer, offset, len); | ||
|
|
||
| Ok(Self { | ||
| data_type: data.data_type().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.
This saves a DataType::drop and a clone of NullBuffer -- probably not a huge deal but unecessary
|
Looks good 🚀 |
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