alamb commented on code in PR #7614:
URL: https://github.com/apache/arrow-rs/pull/7614#discussion_r2133082449
##########
arrow-select/src/coalesce.rs:
##########
@@ -264,42 +264,62 @@ fn gc_string_view_batch(batch: &RecordBatch) ->
RecordBatch {
})
.sum();
let actual_buffer_size = s.get_buffer_memory_size();
+ let buffers = s.data_buffers();
// Re-creating the array copies data and can be time consuming.
// We only do it if the array is sparse
if actual_buffer_size > (ideal_buffer_size * 2) {
// We set the block size to `ideal_buffer_size` so that the
new StringViewArray only has one buffer, which accelerate later concat_batches.
// See https://github.com/apache/arrow-rs/issues/6094 for more
details.
- let mut builder = StringViewBuilder::with_capacity(s.len());
- if ideal_buffer_size > 0 {
- builder = builder.with_fixed_block_size(ideal_buffer_size
as u32);
- }
-
- for v in s.iter() {
- builder.append_option(v);
- }
-
- let gc_string = builder.finish();
-
- debug_assert!(gc_string.data_buffers().len() <= 1); // buffer
count can be 0 if the `ideal_buffer_size` is 0
+ let mut buffer: Vec<u8> =
Vec::with_capacity(ideal_buffer_size);
+
+ let views: Vec<u128> = s
+ .views()
+ .as_ref()
+ .iter()
+ .cloned()
+ .map(|v| {
+ let mut b: ByteView = ByteView::from(v);
+
+ if b.length > 12 {
+ let offset = buffer.len() as u32;
+ buffer.extend_from_slice(
+ buffers[b.buffer_index as usize]
+ .get(b.offset as usize..b.offset as usize
+ b.length as usize)
+ .expect("Invalid buffer slice"),
+ );
+ b.offset = offset;
+ b.buffer_index = 0; // Set buffer index to 0, as
we only have one buffer
+ }
+
+ b.into()
+ })
+ .collect();
+
+ let buffers = if buffer.is_empty() {
Review Comment:
I think the only way this could happen is if `ideal_buffer_size` is zero,
right? In that case we could make a more efficient inner loop that simply
copied the views without checking length, etc
```rust
if ideal_buffer_size == 0 {
// just reuse views as is...
}
```
##########
arrow-select/src/coalesce.rs:
##########
@@ -242,14 +242,14 @@ impl BatchCoalescer {
/// However, after a while (e.g., after `FilterExec` or `HashJoinExec`) the
/// `StringViewArray` may only refer to a small portion of the buffer,
/// significantly increasing memory usage.
-fn gc_string_view_batch(batch: &RecordBatch) -> RecordBatch {
- let new_columns: Vec<ArrayRef> = batch
- .columns()
- .iter()
+fn gc_string_view_batch(batch: RecordBatch) -> RecordBatch {
+ let (schema, columns, num_rows) = batch.into_parts();
+ let new_columns: Vec<ArrayRef> = columns
Review Comment:
I don't know if it matters but in
https://github.com/apache/arrow-rs/pull/7620 I simply modified the existing Vec
rather than collecting into a new one to save maybe an allocation
```rust
for mut c in columns.iter_mut() {
...
}
```
--
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]
