You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/10/26 23:28:44 UTC
[GitHub] [arrow-rs] marioloko opened a new pull request, #2943: Added support for LZ4_RAW compression. (#1604)
marioloko opened a new pull request, #2943:
URL: https://github.com/apache/arrow-rs/pull/2943
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #1604
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
* This adds the implementation of LZ4_RAW codec by using lz4 block compression algorithm. (#1604)
* This commit uses https://stackoverflow.com/questions/25740471/lz4-library-decompressed-data-upper-bound-size-estimation formula to estime the size of the uncompressed size. As it is said in thread, this algorithm over-estimates the size, but it is probably the best we can get with the current decompress API. As the size of a arrow LZ4_RAW block is not prepended to the block.
* Other option would be to take the C++ approach to bypass the API https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/compression_lz4.cc#L343. This approach consists on relaying on the output_buffer capacity to guess the uncompress_size. This works as `serialized_reader.rs` already knows the uncompressed_size, as it reads it from the page header, and allocates the output_buffer with a capacity equal to the uncompress_size https://github.com/marioloko/arrow-rs/blob/master/parquet/src/file/serialized_reader.rs#L417. I did not follow this approach because:
1. It is too hacky.
2. It will limit the use cases of the `decompress` API, as the caller will need to know to allocate the right uncompressed_size.
3. It is not compatible with the current test logic followed by the other codecs. However, specific tests can be created if needed.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
The implementation of an LZ4_RAW codec using LZ4 block algorithm.
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
No changes in the API, but parquet files compressed using LZ4_RAW will be supported now.
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold merged pull request #2943: Added support for LZ4_RAW compression. (#1604)
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #2943:
URL: https://github.com/apache/arrow-rs/pull/2943
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2943: Added support for LZ4_RAW compression. (#1604)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2943:
URL: https://github.com/apache/arrow-rs/pull/2943#discussion_r1007274498
##########
parquet/src/compression.rs:
##########
@@ -325,6 +327,65 @@ mod zstd_codec {
#[cfg(any(feature = "zstd", test))]
pub use zstd_codec::*;
+#[cfg(any(feature = "lz4", test))]
+mod lz4_raw_codec {
+ use std::io::{Read, Write};
+
+ use crate::compression::Codec;
+ use crate::errors::Result;
+
+ /// Codec for LZ4 Raw compression algorithm.
+ pub struct LZ4RawCodec {}
+
+ impl LZ4RawCodec {
+ /// Creates new LZ4 Raw compression codec.
+ pub(crate) fn new() -> Self {
+ Self {}
+ }
+ }
+
+ // Compute max LZ4 uncompress size.
+ // Check https://stackoverflow.com/questions/25740471/lz4-library-decompressed-data-upper-bound-size-estimation
+ fn max_uncompressed_size(compressed_size: usize) -> usize {
+ (compressed_size << 8) - compressed_size - 2526
+ }
+
+ impl Codec for LZ4RawCodec {
+ fn decompress(
+ &mut self,
+ input_buf: &[u8],
+ output_buf: &mut Vec<u8>,
+ ) -> Result<usize> {
+ let offset = output_buf.len();
+ let required_len = max_uncompressed_size(input_buf.len());
Review Comment:
Created https://github.com/apache/arrow-rs/issues/2956
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] marioloko commented on a diff in pull request #2943: Added support for LZ4_RAW compression. (#1604)
Posted by GitBox <gi...@apache.org>.
marioloko commented on code in PR #2943:
URL: https://github.com/apache/arrow-rs/pull/2943#discussion_r1007331707
##########
parquet/src/compression.rs:
##########
@@ -325,6 +327,65 @@ mod zstd_codec {
#[cfg(any(feature = "zstd", test))]
pub use zstd_codec::*;
+#[cfg(any(feature = "lz4", test))]
+mod lz4_raw_codec {
+ use std::io::{Read, Write};
+
+ use crate::compression::Codec;
+ use crate::errors::Result;
+
+ /// Codec for LZ4 Raw compression algorithm.
+ pub struct LZ4RawCodec {}
+
+ impl LZ4RawCodec {
+ /// Creates new LZ4 Raw compression codec.
+ pub(crate) fn new() -> Self {
+ Self {}
+ }
+ }
+
+ // Compute max LZ4 uncompress size.
+ // Check https://stackoverflow.com/questions/25740471/lz4-library-decompressed-data-upper-bound-size-estimation
+ fn max_uncompressed_size(compressed_size: usize) -> usize {
+ (compressed_size << 8) - compressed_size - 2526
+ }
+
+ impl Codec for LZ4RawCodec {
+ fn decompress(
+ &mut self,
+ input_buf: &[u8],
+ output_buf: &mut Vec<u8>,
+ ) -> Result<usize> {
+ let offset = output_buf.len();
+ let required_len = max_uncompressed_size(input_buf.len());
Review Comment:
Then I will wait for this PR to merge first,as the enhancement code changes will probably conflict with this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #2943: Added support for LZ4_RAW compression. (#1604)
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2943:
URL: https://github.com/apache/arrow-rs/pull/2943#issuecomment-1293975421
I took the liberty of fixing the clippy lints and adding a basic integration test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2943: Added support for LZ4_RAW compression. (#1604)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2943:
URL: https://github.com/apache/arrow-rs/pull/2943#discussion_r1007328421
##########
parquet/src/compression.rs:
##########
@@ -325,6 +327,65 @@ mod zstd_codec {
#[cfg(any(feature = "zstd", test))]
pub use zstd_codec::*;
+#[cfg(any(feature = "lz4", test))]
+mod lz4_raw_codec {
+ use std::io::{Read, Write};
+
+ use crate::compression::Codec;
+ use crate::errors::Result;
+
+ /// Codec for LZ4 Raw compression algorithm.
+ pub struct LZ4RawCodec {}
+
+ impl LZ4RawCodec {
+ /// Creates new LZ4 Raw compression codec.
+ pub(crate) fn new() -> Self {
+ Self {}
+ }
+ }
+
+ // Compute max LZ4 uncompress size.
+ // Check https://stackoverflow.com/questions/25740471/lz4-library-decompressed-data-upper-bound-size-estimation
+ fn max_uncompressed_size(compressed_size: usize) -> usize {
+ (compressed_size << 8) - compressed_size - 2526
+ }
+
+ impl Codec for LZ4RawCodec {
+ fn decompress(
+ &mut self,
+ input_buf: &[u8],
+ output_buf: &mut Vec<u8>,
+ ) -> Result<usize> {
+ let offset = output_buf.len();
+ let required_len = max_uncompressed_size(input_buf.len());
Review Comment:
Let's do it as a separate PR, as the other codecs may also benefit from it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] marioloko commented on a diff in pull request #2943: Added support for LZ4_RAW compression. (#1604)
Posted by GitBox <gi...@apache.org>.
marioloko commented on code in PR #2943:
URL: https://github.com/apache/arrow-rs/pull/2943#discussion_r1007327690
##########
parquet/src/compression.rs:
##########
@@ -325,6 +327,65 @@ mod zstd_codec {
#[cfg(any(feature = "zstd", test))]
pub use zstd_codec::*;
+#[cfg(any(feature = "lz4", test))]
+mod lz4_raw_codec {
+ use std::io::{Read, Write};
+
+ use crate::compression::Codec;
+ use crate::errors::Result;
+
+ /// Codec for LZ4 Raw compression algorithm.
+ pub struct LZ4RawCodec {}
+
+ impl LZ4RawCodec {
+ /// Creates new LZ4 Raw compression codec.
+ pub(crate) fn new() -> Self {
+ Self {}
+ }
+ }
+
+ // Compute max LZ4 uncompress size.
+ // Check https://stackoverflow.com/questions/25740471/lz4-library-decompressed-data-upper-bound-size-estimation
+ fn max_uncompressed_size(compressed_size: usize) -> usize {
+ (compressed_size << 8) - compressed_size - 2526
+ }
+
+ impl Codec for LZ4RawCodec {
+ fn decompress(
+ &mut self,
+ input_buf: &[u8],
+ output_buf: &mut Vec<u8>,
+ ) -> Result<usize> {
+ let offset = output_buf.len();
+ let required_len = max_uncompressed_size(input_buf.len());
Review Comment:
I can do the changes for that, I guess it is quite straightforward. Should I add the enhancement to this PR? Or create a new one for that specific change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2943: Added support for LZ4_RAW compression. (#1604)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2943:
URL: https://github.com/apache/arrow-rs/pull/2943#discussion_r1007271432
##########
parquet/src/compression.rs:
##########
@@ -325,6 +327,65 @@ mod zstd_codec {
#[cfg(any(feature = "zstd", test))]
pub use zstd_codec::*;
+#[cfg(any(feature = "lz4", test))]
+mod lz4_raw_codec {
+ use std::io::{Read, Write};
+
+ use crate::compression::Codec;
+ use crate::errors::Result;
+
+ /// Codec for LZ4 Raw compression algorithm.
+ pub struct LZ4RawCodec {}
+
+ impl LZ4RawCodec {
+ /// Creates new LZ4 Raw compression codec.
+ pub(crate) fn new() -> Self {
+ Self {}
+ }
+ }
+
+ // Compute max LZ4 uncompress size.
+ // Check https://stackoverflow.com/questions/25740471/lz4-library-decompressed-data-upper-bound-size-estimation
+ fn max_uncompressed_size(compressed_size: usize) -> usize {
+ (compressed_size << 8) - compressed_size - 2526
+ }
+
+ impl Codec for LZ4RawCodec {
+ fn decompress(
+ &mut self,
+ input_buf: &[u8],
+ output_buf: &mut Vec<u8>,
+ ) -> Result<usize> {
+ let offset = output_buf.len();
+ let required_len = max_uncompressed_size(input_buf.len());
Review Comment:
Longer term it would be nice to plumb the decompressed size down, as we do actually know what it is from the page header
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ursabot commented on pull request #2943: Added support for LZ4_RAW compression. (#1604)
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #2943:
URL: https://github.com/apache/arrow-rs/pull/2943#issuecomment-1294058878
Benchmark runs are scheduled for baseline = 880c4d98e6d570db701fb013f3abf5e5e6f42e32 and contender = 4e1247e8c03f36940a912256e2d94f49a1b581df. 4e1247e8c03f36940a912256e2d94f49a1b581df is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/491503e5d9cb4cc1a24e3b3b8104a433...fc86c13e8ee94f979a55990a6f5f1432/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/1e562f125b464b1e8bc3a7d2d5ec5eb0...ad8ddb451cdd4de085478a0809a2902d/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/86ee3ea25536444fbf1a4623e49ac497...471ff975a56c4ecc8167cda813c80ef8/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/8a8b5c483cc54d0496daaf49a05bc0c1...cd800c241512474b995388109d58df19/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org