perf(arrow-ipc): Avoid copies and write dictionary batches directly to writers when possible#10128
Open
JakeDern wants to merge 6 commits into
Open
perf(arrow-ipc): Avoid copies and write dictionary batches directly to writers when possible#10128JakeDern wants to merge 6 commits into
JakeDern wants to merge 6 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
from
July 10, 2026 04:06
78e917b to
2a21b60
Compare
JakeDern
force-pushed
the
ipc-writer-collect-dicts
branch
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 |
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
2 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.