Expose IPC decompressed size and bound LZ4 output - #10959
Conversation
|
run benchmark arrow-ipc |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing ipc-decompression-size-upper-bound (eb92502) to b84fc5c (merge-base) diff Run configurationrun benchmark arrow-ipcBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow-ipc File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
Rich-T-kid
left a comment
Rich-T-kid
left a comment
There was a problem hiding this comment.
thank you for the in depth PR description.
can you add test for this if it doesn't already exist
'
Without the LZ4 bound, an untrusted compressed stream could cause Arrow's output Vec to grow beyond an accepted advertised length before decompression eventually failed.
'
| /// Returns an error if the input buffer is shorter than the 8-byte prefix. | ||
| #[inline] | ||
| fn read_uncompressed_size(buffer: &[u8]) -> Result<i64, ArrowError> { | ||
| pub fn read_uncompressed_size(buffer: &[u8]) -> Result<i64, ArrowError> { |
There was a problem hiding this comment.
I'm not sure that we should make this public.
do you have examples of when it would be useful to use this directly?
There was a problem hiding this comment.
@Rich-T-kid Thank you for the prompt review. I have added a test that returns an error on an underreported size for LZ4.
To answer your question, this function can be very useful for downstream users. It allows effective resource governance, where instead of unconditionally doing decompress and possibly getting an OOM issue during the allocation, you check if resources (memory in this case) in your system are available and make a decision based on that. That is also why LZ4 fix is important, since it gives us a much stronger guarantee.
I don't see any other way to achieve this currently that is exposed through the public API.
There was a problem hiding this comment.
makes sense, I'm interested in what Jefffrey thinks.
There was a problem hiding this comment.
where instead of unconditionally doing decompress and possibly getting an OOM issue during the allocation, you check if resources (memory in this case) in your system are available and make a decision based on that.
this is interesting, you may be interested in this issue #10392
There was a problem hiding this comment.
Thanks for pointing out the issue, though the proposition is more about memory optimization if some kind of projection is done (which I agree can be significant for some workloads), the issue I raised is more concerned with having a guaranteed upper bound on memory allocation and knowing it before allocation happens.
Thanks for the review, I totally understand general hesitation when expansion of the public API surface area is considered. In this case, though, I believe the decompressed size is more than internal detail, as downstream users can generally make decisions based on it.
|
run benchmark ipc_reader |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing ipc-decompression-size-upper-bound (eb92502) to b84fc5c (merge-base) diff Run configurationrun benchmark ipc_readerBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench ipc_reader File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing ipc-decompression-size-upper-bound (eb92502) to b84fc5c (merge-base) diff Run configurationrun benchmark ipc_readerCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Rich-T-kid
left a comment
Rich-T-kid
left a comment
There was a problem hiding this comment.
Thank you for adding a test. This PR looks good to me!
ideally we keep our api surface as small as possible but this seems small and focused so it looks fine to me.
thanks @azecevic000
There was a problem hiding this comment.
while we're here can we avoid this panic? it should be pretty straightforward to error here.
|
|
||
| #[test] | ||
| #[cfg(feature = "lz4")] | ||
| fn test_lz4_decompression_rejects_output_exceeding_advertised_size() { |
| /// Returns an error if the input buffer is shorter than the 8-byte prefix. | ||
| #[inline] | ||
| fn read_uncompressed_size(buffer: &[u8]) -> Result<i64, ArrowError> { | ||
| pub fn read_uncompressed_size(buffer: &[u8]) -> Result<i64, ArrowError> { |
There was a problem hiding this comment.
makes sense, I'm interested in what Jefffrey thinks.
| /// Returns an error if the input buffer is shorter than the 8-byte prefix. | ||
| #[inline] | ||
| fn read_uncompressed_size(buffer: &[u8]) -> Result<i64, ArrowError> { | ||
| pub fn read_uncompressed_size(buffer: &[u8]) -> Result<i64, ArrowError> { |
There was a problem hiding this comment.
where instead of unconditionally doing decompress and possibly getting an OOM issue during the allocation, you check if resources (memory in this case) in your system are available and make a decision based on that.
this is interesting, you may be interested in this issue #10392
4 participants
Rationale for this change
Compressed Arrow IPC buffers advertise their uncompressed length in an 8-byte prefix.
decompress_to_bufferinterprets0as empty,-1as uncompressed, and a positive value as the expected decompressed length. For a positive value, IPC decompression already requires an exact match: the common codec path rejects a returned buffer whose length differs from the advertised length.The codecs previously differed in how they used this value while decompressing. ZSTD passes it as the output capacity to an API that returns an error if the decompressed data exceeds that capacity. LZ4, however, used it only as the initial
Veccapacity before an unboundedread_to_end, allowing Arrow's outputVecto grow beyond the advertised length before the final check.This change gives both codecs the same output-size invariants:
Vec.Without the LZ4 bound, an untrusted compressed stream could cause Arrow's output
Vecto grow beyond an accepted advertised length before decompression eventually failed. With the bound, the potentially unbounded output accumulation is limited to the advertised length.Downstream IPC consumers may also need to inspect the advertised length before decompression so that they can reject an unreasonable value before allocating the output buffer. Exposing
read_uncompressed_sizelets them do so without duplicating Arrow's interpretation of the IPC prefix.What changes are included in this PR?
read_uncompressed_sizefromarrow-ipcand document the-1,0, and positive-length semantics.The advertised length remains the exact required output length on successful return and bounds the number of decompressed bytes accumulated in Arrow's output
Vec. It does not bound the compressed input, allocator overhead, or codec working memory.Are these changes tested?
No new tests are introduced. Existing tests cover successful LZ4 and ZSTD round trips and rejection of prefixes shorter than 8 bytes. The new LZ4 over-limit rejection is not directly covered.
Are there any user-facing changes?
Yes.
arrow_ipc::read_uncompressed_sizeis a new public, non-breaking API that lets callers inspect the IPC compression prefix before decompression.Malformed LZ4 input whose decompressed output exceeds its advertised length is now rejected before Arrow's output buffer grows beyond that length. Valid IPC input is unaffected.