[All]Fix compression#385
Open
elenagaljak-db wants to merge 3 commits into
Open
Conversation
| .collect(); | ||
| RecordBatch::try_new(batch.schema(), columns) | ||
| .expect("compacted batch preserves the original schema and row count") | ||
| } |
Contributor
There was a problem hiding this comment.
This must not be forgotten, it will impact perf on SDKs, as will not fixing it. It should be removed as soon as it is fixed in the arrow. Do we know the rough performance impact of this change?
teodordelibasic-db
left a comment
teodordelibasic-db
left a comment
Contributor
There was a problem hiding this comment.
LGTM, let's just revert the Python/Java changes like discussed offline. Also, do we know if fix for this issue is in progress for Arrow?
| stream.ingest_batch(batches.remove(0)).await | ||
| // The IPC reader is zero-copy and slices all columns out of one shared | ||
| // allocation, which makes the Flight encoder massively over-estimate the | ||
| // batch size and over-split it (arrow-rs#9388). Compact to restore accurate |
Contributor
There was a problem hiding this comment.
nit: It seems it's better for these to either be apache/arrow-rs#9388 or the full URL https://github.com/apache/arrow-rs/issues/9388.
| /// each `Buffer::capacity()` reports the whole message-body size. This makes the | ||
| /// Flight encoder over-estimate the batch and over-split it (and defeats IPC | ||
| /// compression). Re-allocating restores accurate size estimates. Workaround for | ||
| /// arrow-rs#9388 / #5352. |
Contributor
There was a problem hiding this comment.
Same applies here.
| match reader.next() { | ||
| None => Ok(batch), | ||
| // Compact so the Flight encoder estimates batch size correctly (see | ||
| // `compact_record_batch` and arrow-rs#9388). |
elenagaljak-db
force-pushed
the
elenagaljak-db-fix_compression
branch
from
06c42bd to
d5791d8
Compare
Signed-off-by: elenagaljak-db <elena.galjak@databricks.com>
elenagaljak-db
force-pushed
the
elenagaljak-db-fix_compression
branch
from
d5791d8 to
77bfec5
Compare
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
3 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.
What changes are proposed in this pull request?
Arrow Flight ingestion was over-splitting every batch decoded from Arrow IPC bytes into many tiny
FlightDatamessages, which inflated message counts and made IPC compression ineffective (compressing each tiny chunk independently yields almost nothing). This patches the Flight encoder to size batches correctly, fixingcompression and message overhead with no extra data copy.
Vendor
arrow-flight58.2.0 intorust/third_party/arrow-flightand change the size calculation insplit_batch_for_grpc_responseto be slice-aware: