alamb commented on code in PR #10262:
URL: https://github.com/apache/arrow-rs/pull/10262#discussion_r3540346508
##########
arrow-ipc/src/writer.rs:
##########
@@ -622,14 +622,18 @@ impl IpcDataGenerator {
) -> Result<(Vec<EncodedData>, EncodedData), ArrowError> {
let encoded_dictionaries =
self.encode_all_dicts(batch, dictionary_tracker, write_options,
ipc_write_context)?;
- let mut arrow_data = Vec::new();
+ let mut arrow_data = std::mem::take(&mut ipc_write_context.scratch);
let (ipc_message, _, tail_pad) = self.record_batch_to_bytes(
batch,
write_options,
ipc_write_context,
&mut IpcBodySink::Write(&mut arrow_data),
)?;
arrow_data.extend_from_slice(&PADDING[..tail_pad]);
+ let final_capcity = arrow_data.capacity();
Review Comment:
typo:
```suggestion
let final_capacity = arrow_data.capacity();
```
##########
arrow-ipc/src/writer.rs:
##########
@@ -622,14 +622,18 @@ impl IpcDataGenerator {
) -> Result<(Vec<EncodedData>, EncodedData), ArrowError> {
let encoded_dictionaries =
self.encode_all_dicts(batch, dictionary_tracker, write_options,
ipc_write_context)?;
- let mut arrow_data = Vec::new();
+ let mut arrow_data = std::mem::take(&mut ipc_write_context.scratch);
let (ipc_message, _, tail_pad) = self.record_batch_to_bytes(
batch,
write_options,
ipc_write_context,
&mut IpcBodySink::Write(&mut arrow_data),
)?;
arrow_data.extend_from_slice(&PADDING[..tail_pad]);
+ let final_capcity = arrow_data.capacity();
+ if ipc_write_context.reserve_scratch {
Review Comment:
I think this would be better if it was encapsulated as a method on
ipc_write_context
Perhaps something like
```rust
ipc_write_context.reserve(final_capacity)
```
And then check the flag internally in the method
##########
arrow-ipc/src/compression.rs:
##########
@@ -44,6 +44,13 @@ impl IpcWriteContext {
&mut self.fbb
}
+ /// Set whether the scratch buffer capacity should be reserved after each
encode for reuse
Review Comment:
Can we also document the default value (defaults to false)
##########
arrow-ipc/src/writer.rs:
##########
@@ -622,14 +622,18 @@ impl IpcDataGenerator {
) -> Result<(Vec<EncodedData>, EncodedData), ArrowError> {
let encoded_dictionaries =
self.encode_all_dicts(batch, dictionary_tracker, write_options,
ipc_write_context)?;
- let mut arrow_data = Vec::new();
+ let mut arrow_data = std::mem::take(&mut ipc_write_context.scratch);
Review Comment:
likewise I would recommend making this a method on the IpcWriteContext
structure so it is easier to reason / understand how the different fields are
used
SOmething like
```rust
let mut arrow_data = ipc_write_context.scratch();
```
Then we can contemplate methods like
```rust
ipc_write_context.save_scratch(...)
```
--
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]
