Rich-T-kid commented on PR #10383:
URL: https://github.com/apache/arrow-rs/pull/10383#issuecomment-5018890719
Spent a few hours debugging this. going to leave this comment sharing what I
found in case anyone has ideas on how to improve this. The PR is mostly ready
for review so mark it as open.
Previously, the size estimate used for chunking was often much larger than
the actual memory the RecordBatch held. Since n_batches = size /
target_size_chunks, that inflated size produced more chunks than necessary.
for example
Take a RecordBatch of size `100MB` containing uniform (fixed-width) arrays.
It's sliced in half, and only one half is encoded and sent over the wire via
Arrow Flight. Assume `max_flight_data_size` is the default, 2MB.
The current implementation looks at the full `100MB` and splits toward a
goal of `2MB` chunks, producing 50 batches. The split itself is done by row
count, slicing rows off the batch (zero-copy, just moving pointers in the
underlying buffers).
This happens to work today because `100MB` isn't an accurate estimate of the
space the batch actually takes up. It over-estimates the size per row, so the
row count assigned to each chunk ends up holding less than 2MB of real data.
Same number of chunks, but less data crammed into each one, that's why the
output stays under the limit.
This works in the test's favor right now because a sliced RecordBatch's
reported size is roughly 2x larger than what it actually takes up once
IPC-encoded. That ~2x gap is the only reason the test currently passes.
```
Encoding 10240 with a maximum size of 5000
max_flight_size : 5000
total-size : 381056
number of batches : 77
rows per batch : 132
variable width : bufffer_len 1320
variable width : bufffer_len 792
record batch size before encoding ipc: 2380
post record batch encoding size:
ipc_message : 312,
arrow_data: 4600 ,
total: 4912
```
With the switch to get_slice_memory_size, the size returned now reflects the
actual memory the sliced batch takes up, so the calculation correctly figures
out almost the exact number of rows needed to fill a max_flight_data_size chunk.
This becomes a problem when IPC encoding significantly inflates the size of
the RecordBatch. Since the chunk size is calculated before encoding, that
inflation isn't accounted for, and the resulting chunk ends up exceeding
max_flight_data_size after all
```
Encoding 10240 with a maximum size of 5000
max_flight_size : 5000
variable width : bufffer_len 102400
variable width : bufffer_len 61440
total-size : 184324
number of batches : 37
rows per batch : 276
variable width : bufffer_len 2760
variable width : bufffer_len 1656
record batch size before encoding ipc: 4972
post record batch encoding size:
ipc_message : 312,
arrow_data: 9560 ,
total: 9872
```
off the top of my head it seems theres two viable solutions
1. Bump the `allow_range` in the test to avoid failing on this case. Not
ideal, this just kicks the can down the road. For users with a `max_grpc_size`
config that isn't` usize::MAX`, sending a record batch chunk that's too large
could still result in panics.
2. Trim the target record batch chunk size to account for IPC encoding
overhead. This needs to be done carefully. The goal of the original change was
to size chunks based on how much data the RecordBatch is actually holding, so
we need to balance that against making sure the IPC-encoded message itself
doesn't grow too large.
--
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]
