itsjunetime commented on PR #6690:
URL: https://github.com/apache/arrow-rs/pull/6690#issuecomment-2480879299
> Let me see if I can help to find a way to break this PR up into smaller,
more manageable pieces, so that we can get this in / tested in a way that is
reasonable to maintain
One change I could make that may help with breaking this up and also with
@tustvold's concern about the IPC-specific interface would be to maybe
genericize the `get_memory_slice_size_with_alignment` over a `T:
MemoryAccountant` (or something like that), e.g.:
```rust
enum SizeSource {
Buffer(BufferType),
NullBuffer,
ChildData,
// does it need to be more granular? idk
}
trait MemoryAccountant {
fn count_size(&mut self, size: usize, source: SizeSource);
}
impl ArrayData {
fn get_slice_memory_size_with_accountant<A: MemoryAccountant>(
&self,
acc: &mut A
) -> Result<(), ArrowError> {
// ...
}
fn get_slice_memory_size(&self) -> Result<usize, ArrowError> {
struct DefaultAccountant(usize);
impl MemoryAccountant for DefaultAccountant {
fn count_size(&mut self, size: usize, _: SizeSource) {
self.0 += size;
}
}
let mut acc = DefaultAccountant(0);
self.get_slice_memory_size_with_accountant(&mut acc)?;
Ok(acc.0)
}
}
```
This would allows us to use it nicely with the 'alignment' accounting that
we need without being too IPC-specific. It would also allow us to remove the
ugly re-accounting for `RunEndEncoded` buffers that this PR adds in
`get_encoded_arr_batch_size`, which would be nice.
Obviously, I'd be happy to make this change and pull it out to a separate PR
(to make this PR easier to review once that separate PR is merged) if we feel
like this would be a better move.
--
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]
