perf(arrow-ipc): Avoid copies and write dictionary batches directly to writers when possible - #10128
Open
JakeDern wants to merge 4 commits into
Open
perf(arrow-ipc): Avoid copies and write dictionary batches directly to writers when possible#10128JakeDern wants to merge 4 commits into
JakeDern wants to merge 4 commits into
Conversation
Contributor
Author
|
Pretty good improvement - ~42% for the dictionary case and ~20% for delta dictionary cases. Not 100% sure why less improvement on the delta side yet, but I think this is worth it to take on its own and can investigate further later. Perf results from #10122: perf results from this branch: |
Contributor
Author
|
CC: @alamb and @Rich-T-kid - I think we got pretty good results here! I also tried to clean up a few things here and there where I could like removing some unnecessary parameter drilling. This has the benchmarks from #10122 as well, will rebase once that goes in. |
Contributor
|
I can take a look at this early next week. |
Contributor
|
started taking a look, results look good! 🚀 |
Rich-T-kid
reviewed
Jun 17, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
I think this PR looks mostly fine. it would be nice to include a round trip test similar to ##10097 that validate that nothing is being broken.
left a couple non-blocking comments. Ill try and take a second pass through tommorow if I can get a chance/ if this is still open
JakeDern
force-pushed
the
ipc-writer-collect-dicts
branch
2 times, most recently
from
July 10, 2026 04:07
2a21b60 to
f9a0328
Compare
Contributor
Author
|
Took a little longer than I'd have liked, but I got back to rebasing this and fixing it up. @Rich-T-kid or @alamb would definitely appreciate another look.
We do have some round trip tests for the writer, but if there's anything specific missing here, I'm happy to take a stab at adding it! |
JakeDern
marked this pull request as ready for review
July 10, 2026 04:58
Contributor
|
Ill try and take another look next week |
Contributor
|
cc @Phoenix500526 in case your interested @JakeDern could you rebase this branch? I think there are outdated changed in your diff |
Contributor
|
This may also be related #9775 |
Contributor
|
run benchmark arrow_writer |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing ipc-writer-collect-dicts (fd9d7c2) to fed7862 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
JakeDern
force-pushed
the
ipc-writer-collect-dicts
branch
from
July 21, 2026 05:15
fd9d7c2 to
2bda004
Compare
Contributor
Author
I think we're all fixed up now - Nice to see so much IPC activity! |
Contributor
@alamb I think you meant to run |
Contributor
|
run benchmark ipc_writer |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing ipc-writer-collect-dicts (2bda004) to 98998a8 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
|
|
Rich-T-kid
suggested changes
Jul 24, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
The benchmarks look very good and the PR looks nice. I left a couple of suggestions.
I'd like to take a second look as well! could you update the description to explain the flow between calls a bit more? The arrow-ipc reader/writer code takes a lot of context to keep in mind. 😮💨
Rich-T-kid
approved these changes
Jul 28, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
Thanks @JakeDern this looks good to me. I left a couple comments, mostly nits.
🚀
Contributor
|
also with @Phoenix500526's #10277 being merged yesterday we may have to rebase this again |
JakeDern
force-pushed
the
ipc-writer-collect-dicts
branch
from
July 29, 2026 03:52
dcfe3f9 to
e946870
Compare
Contributor
Author
|
Alright @Rich-T-kid, one more rebase for the books! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
This is a follow on to #10044, applying basically the same optimization for dictionary batches.
This needs to wait for #10122 before merge.
What changes are included in this PR?
Are these changes tested?
Yes, existing unit tests should cover the change.
Are there any user-facing changes?
No.