You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Rich-T-kid (via GitHub)" <gi...@apache.org> on 2026/07/19 16:45:49 UTC
[PR] creating unit test to support switch to get_slice_memory_Size() [arrow-rs]
Rich-T-kid opened a new pull request, #10383:
URL: https://github.com/apache/arrow-rs/pull/10383
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.
-->
- Closes #NNN.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
If this PR claims a performance improvement, please include evidence such as benchmark results.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
If there are any breaking changes to public APIs, please call them out.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] creating unit test to support switch to get_slice_memory_Size() [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf/bug: use accurate memory size when determining flight batch chunks [arrow-rs]
Posted by "adriangbot (via GitHub)" <gi...@apache.org>.
adriangbot commented on PR #10383:
URL: https://github.com/apache/arrow-rs/pull/10383#issuecomment-5465569083
Hi @Rich-T-kid, you asked to view the benchmark queue (https://github.com/apache/arrow-rs/pull/10383#issuecomment-5465568961).
| Comment | Repo | PR | User | Benchmarks | Status |
| --- | --- | --- | --- | --- | --- |
| [#5465508360](https://github.com/apache/arrow-rs/pull/10862#issuecomment-5465508360) | apache/arrow-rs | #10862 | Rich-T-kid | ["arrow_writer"] | running |
| [#5465529064](https://github.com/apache/arrow-rs/pull/10913#issuecomment-5465529064) | apache/arrow-rs | #10913 | Rich-T-kid | ["zip_kernels"] | running |
---
[File an issue](https://github.com/adriangb/datafusion-benchmarking/issues) against this benchmark runner
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] creating unit test to support switch to get_slice_memory_Size() [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on PR #10383:
URL: https://github.com/apache/arrow-rs/pull/10383#issuecomment-5017583897
to keep this PR lean im going to resolve the inaccurate results produced by `get_slice_memory_size` in a seperate PR and then rebase this.
tracking this in https://github.com/apache/arrow-rs/issues/10385
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf/bug: use accurate memory size when determining flight batch chunks [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on PR #10383:
URL: https://github.com/apache/arrow-rs/pull/10383#issuecomment-5664955413
closing in favor of #11048
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf/bug: use accurate memory size when determining flight batch chunks [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid closed pull request #10383: perf/bug: use accurate memory size when determining flight batch chunks
URL: https://github.com/apache/arrow-rs/pull/10383
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] creating unit test to support switch to get_slice_memory_Size() [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on PR #10383:
URL: https://github.com/apache/arrow-rs/pull/10383#issuecomment-5017572359
```rust
fn compare_column_sizes(batch: &RecordBatch) {
let schema = batch.schema();
for (i, col) in batch.columns().iter().enumerate() {
let name = schema.field(i).name();
let data = col.to_data();
let slice_size = data.get_slice_memory_size().unwrap();
let buffer_size = col.get_buffer_memory_size();
if slice_size != buffer_size {
println!(
"[{name}] data_type={:?} slice_size={slice_size} buffer_size={buffer_size} diff={}",
data.data_type(),
buffer_size as i64 - slice_size as i64
);
} else {
println!(
"[{name}] data_type={:?} sizes match ({slice_size})",
data.data_type()
);
}
}
}
```
is resulting in
```rust
[s1] data_type=Utf8 slice_size=14336 buffer_size=20544 diff=6208
[i1] data_type=Int16 sizes match (2048)
[s2] data_type=Utf8 slice_size=10240 buffer_size=12352 diff=2112
[i2] data_type=Int64 sizes match (8192)
```
the calculations for the string arrays are not accurate. I could fix the breaking test `encode::tests::flight_data_size_even` by making the allowed coverage larger but I think updating `get_slice_memory_size` to be more accurate is better
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf/bug: use accurate memory size when determining flight batch chunks [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on PR #10383:
URL: https://github.com/apache/arrow-rs/pull/10383#issuecomment-5665192181
closing in favor of https://github.com/apache/arrow-rs/pull/11048
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf/bug: use accurate memory size when determining flight batch chunks [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on PR #10383:
URL: https://github.com/apache/arrow-rs/pull/10383#issuecomment-5073852014
still a wip, added a couple test for the reproducers mentioned in #9388
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf/bug: use accurate memory size when determining flight batch chunks [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on PR #10383:
URL: https://github.com/apache/arrow-rs/pull/10383#issuecomment-5404172599
related - https://github.com/apache/arrow-rs/issues/4724
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] creating unit test to support switch to get_slice_memory_Size() [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on PR #10383:
URL: https://github.com/apache/arrow-rs/pull/10383#issuecomment-5018914700
I may also just be missing something, the comment above `split_batch_for_grpc_response` doesnt it make it seem like the under-accounting to reserver space for arrow-ipc is intentional
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf/bug: use accurate memory size when determining flight batch chunks [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid closed pull request #10383: perf/bug: use accurate memory size when determining flight batch chunks
URL: https://github.com/apache/arrow-rs/pull/10383
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf/bug: use accurate memory size when determining flight batch chunks [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on PR #10383:
URL: https://github.com/apache/arrow-rs/pull/10383#issuecomment-5665196742
closing in favor of https://github.com/apache/arrow-rs/pull/11048
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf/bug: use accurate memory size when determining flight batch chunks [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on PR #10383:
URL: https://github.com/apache/arrow-rs/pull/10383#issuecomment-5465568961
show benchmark queue
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org