perf(arrow-ipc): Avoid copies and write dictionary batches directly to writers when possible#10128
Draft
JakeDern wants to merge 19 commits into
Draft
perf(arrow-ipc): Avoid copies and write dictionary batches directly to writers when possible#10128JakeDern wants to merge 19 commits into
JakeDern wants to merge 19 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
| // Collect all dicts that need a dictionary message i.e. ones that were | ||
| // either not in the tracker previously or ones that were but need a | ||
| // replacement or delta. | ||
| #[allow(clippy::too_many_arguments)] |
Contributor
There was a problem hiding this comment.
Is it possible to remove the lint bypass? I know its not tied directly to this PR but it'd be nice to incrementally clean up this file.
| compression_context, | ||
| )?); | ||
| } | ||
| let mut fbb = FlatBufferBuilder::new(); |
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.