Hadoop LZ4 Support for LZ4 Codec #3013
Conversation
* Added `CodecOptions` struct to hold `Codec` configuration. * Added `backward_compatible_lz4` option in `CodecOptions`. * Added `CodecOptions` to `ReadOptions` to be able to configure `SerializedFileReader`. * Added `SerializedRowGroupReaderOptionsBuilder` with `CodecOptions` to be able to configure `SerializedRowGroupReader`, with extensible interface. * Added `SerializedPageReaderOptionsBuilder` with `CodecOptions` to be able to configure `SerializedPageReader`, with extensible interface. * Added `new_with_config` to `SerializedPageReader` API to be able to configure `SerializedFileReader` without breaking `new` API. * `CodecOptions` implements `CopyTrait` as it is composed by `Copy` types. If in the future it contains a non `Copy` type, maybe is better to create `CodecOptionsPtr = Arc<CodecOptions>`. * `CodecOptions` is only added in the read path, in the write path the default values are taken, as the options currently only affect the read path and have no effect on write path. If required to add to write path maybe it will be nice to add into `WriteProperties`.
* Added compression and decompression for LZ4_HADOOP.
* Added a test for two parquet files with the same content, both with LZ4 CompressionCodec, but one using the LZ4_HADOOP (no-fallback) algorithm and the other LZ4_RAW algorithm (fallback to last level). * Refactor `LZ4HadoopCodec::compress` function to make it easier to understand.
|
Another thing I notice with using pub fn new_with_options(chunk_reader: R, options: ReadOptions) -> Result<Self> {
let metadata = footer::parse_metadata(&chunk_reader)?;
let mut predicates = options.predicates;Of course these options are not used anymore at this point, so instead of cloning the vector of predicates we can just take it. pub fn new_with_options(chunk_reader: R, options: ReadOptions) -> Result<Self> {
let metadata = footer::parse_metadata(&chunk_reader)?;
let mut predicates = std::men::take(&mut options.predicates); |
|
If we want to hide the This problem also implies implementing setters multiple times, but we can use the same @tustvold what do you think? |
|
Sorry for the delay, I'm currently on holiday. Will try to find time to review today |
|
Oh, do not worry, enjoy your holidays! We review this when you are back 😄 |
This commits hides `CodecOptions` from the public API. The changes are the following: - Added a new structs to public API `ReaderProperties`, `ReaderPropertiesBuilder` and `ReaderPropertiesPtr` to store inmutable reader config, as it is the case of `CodecOptions`. - Removed `SerializedRowGroupReaderOptions`, `SerializedRowGroupReaderOptionsBuilder`, `SerializedPageReaderOptionsBuilder` and `SerializedPageReaderOptions`. They are not required anymore as `SerializedRowGroupReader` and `SerializedRowGroupReaderOptions` use `ReaderPropertiesPtr` for configuration. - `SerializedRowGroupReader::new_with_options` renamed to `SerializedRowGroupReader::new_with_properties`. - `SerializedPageReader::new_with_options` renamed to `SerializedPageReader::new_with_properties`. - Test added for `ReaderPropertiesBuilder`.
|
With this last change we remove Commit description:This commits hides
|
|
I think that now it is everything is ready to merge. Take a look when you have time and we discuss if any change has to be done. About the failing test above I think it is not caused by my commit, as I have not modify IPC. In fact, in my local Mac the test is passing. |
| @@ -2422,6 +2422,76 @@ mod tests { | |||
| assert_eq!(a.values(), &[42.000000, 7.700000, 42.125000, 7.700000]); | |||
| } | |||
|
|
|||
| // This test is to ensure backward compatibility, it test 2 files containing the LZ4 CompressionCodec | |||
| } | ||
|
|
||
| /// Try to decompress the buffer as if it was compressed with the Hadoop Lz4Codec. | ||
| /// Adapted from pola-rs [compression.rs:try_decompress_hadoop](https://pola-rs.github.io/polars/src/parquet2/compression.rs.html#225) |
|
Thanks again 😄 |
|
Benchmark runs are scheduled for baseline = 488eff0 and contender = 4f525fe. 4f525fe is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
Marking as api-change even though backwards compatible as does change the output, and so should be highlighted in changelog |
Which issue does this PR close?
Closes #2988 .
Rationale for this change
What changes are included in this PR?
It adds support for
LZ4_HADOOPcompression forCompressionCodec::LZ4, with fallback toLZ4_FRAMEandLZ4_RAWalgorithm for backward compatibility with olderarrow-rsversions and olderparquet-cppversions, respectively.It also adds tests for
LZ4_HADOOPfiles, and tests for the fallback toLZ4_RAW(a test including 2 files with same content, sameCompressionCodec::LZ4, but different compression algorithmLZ4_HADOOPandLZ4_RAW).From now on,
CompressionCodec::LZ4will compress files usingLZ4_HADOOPalgorithm, making LZ4 files generated by this library compatible with other languages arrow implementations.The user interface is changed to allow the user to configure whether to fallback or not, by allowing the user to disable backward compatibility with
no_backward_compatible_lz4(WIP in this PR).Are there any user-facing changes?
The user interface is changed to allow the user to configure whether to fallback or not, by allowing the user to disable backward compatibility with
no_backward_compatible_lz4(WIP in this PR).New structs have been created:
SerializedRowGroupReaderOptionsandSerializedPageReaderOptionsto allow the users to set theCodecOptionsforSerializedRowGroupReaderandSerializedPageReader. The builders for these structs are also added to public interface.SerializedRowGroupReader::new_with_optionsandSerializedPageReader::new_with_optionshave been created to allow to configure these components without breaking public API.NOTE: The API changes are WIP, however, it is not possible to follow the same approach that with
WriterPropertiesand wrapReadOptionsin anArcas its fieldpredicates: Vec<ReadGroupPredicate>is not thread safe. Other option would be to wrap it in just anRcsotype ReadOptionsPtr = Rc<ReadOptions>.