alamb commented on code in PR #7773:
URL: https://github.com/apache/arrow-rs/pull/7773#discussion_r2167492034
##########
arrow-array/benches/view_types.rs:
##########
@@ -43,6 +43,12 @@ fn criterion_benchmark(c: &mut Criterion) {
hint::black_box(sliced.gc());
});
});
+
+ c.bench_function("view types slice", |b| {
Review Comment:
Nice! Is there any chance you can pull this benchmark into its own PR (to
make it easier for me to run benchmarks to compare the performance of this PR
and main)?
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -162,7 +164,7 @@ use super::ByteArrayType;
pub struct GenericByteViewArray<T: ByteViewType + ?Sized> {
data_type: DataType,
views: ScalarBuffer<u128>,
- buffers: Vec<Buffer>,
+ buffers: Arc<[Buffer]>,
Review Comment:
Would it be possible to use `ViewBuffers` here to keep the encapsulation?
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -734,12 +743,20 @@ where
}
impl<T: ByteViewType + ?Sized> From<GenericByteViewArray<T>> for ArrayData {
- fn from(mut array: GenericByteViewArray<T>) -> Self {
+ fn from(array: GenericByteViewArray<T>) -> Self {
let len = array.len();
- array.buffers.insert(0, array.views.into_inner());
+ let new_buffers = {
+ let mut buffers = Vec::with_capacity(array.buffers.len() + 1);
+ buffers.push(array.views.into_inner());
+ for buffer in array.buffers.iter() {
+ buffers.push(buffer.clone());
+ }
Review Comment:
Since this gets an owned array, I think you can avoid cloning the buffer
here too (`for buffer in array.buffers ...`).
Also, It probably doesnt matter but you can probably use extend here and
skip some capacity checks when appending as well
```suggestion
buffers.extend(array.buffers)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
