PARQUET-456: Add GZip compressor #11
Conversation
|
I created a JIRA to track this issue |
|
@knizhnik please prefix PR description with PARQUET-456: |
|
This will have to be rebased -- pls let us know when there is a passing build. |
Conflicts: example/CMakeLists.txt src/compression/CMakeLists.txt src/parquet/compression/codec.h
|
thanks @knizhnik |
|
Can you please add zlib to the sandbox build toolchain? See thirdparty/ and feel free to reuse the CMake module for Snappy so that $ZLIB_HOME can be configured by the user |
| lz4-codec.cc | ||
| snappy-codec.cc | ||
| gzip-codec.cc | ||
| ) |
There was a problem hiding this comment.
The location of this file has changed, can you remove or update?
|
Please add zlib as a configurable part of the build toolchain as we should not require the user to install zlib at the system level.
## Snappy
find_package(Snappy REQUIRED)
include_directories(SYSTEM ${SNAPPY_INCLUDE_DIR})
add_library(snappystatic STATIC IMPORTED)
set_target_properties(snappystatic PROPERTIES IMPORTED_LOCATION ${SNAPPY_STATIC_LIB})
Thank you! |
| if [ -n "$F_ALL" -o -n "$F_ZLIB" ]; then | ||
| cd $TP_DIR/$ZLIB_BASEDIR/cmake_unofficial | ||
| CFLAGS=-fPIC cmake -DCMAKE_INSTALL_PREFIX:PATH=$PREFIX $ZLIB_DIR | ||
| make -j$PARALLEL install |
There was a problem hiding this comment.
It looks like zlib does have a CMakeLists.txt, but it's in the base directory of the tarball, so remove the /cmake_unofficial
|
@knizhnik I can take over from here if you like, please advise |
|
Sorry, I do not have much experience in using git. This is the reason of my delay with the answer - I just wanted to wait for tomorrow and ask help of my colleague. I have already merged with upstream. Not sure if it is still possible to combine all my changes into single commit... So it will be great if you can take over, you will safe a lot of my time:) |
|
No problem! I really appreciate the work you've done on this. I refactored some things that conflict with this in #32 so I'll resolve the conflict and post a new PR. I'll ping you if I need help with anything else. |
|
I'll take a look at this within the next week or so; I also want to add unit tests for page decompression. Will circle back |
| @@ -51,6 +51,12 @@ ColumnReader::ColumnReader(const parquet::ColumnMetaData* metadata, | |||
| case CompressionCodec::SNAPPY: | |||
| decompressor_.reset(new SnappyCodec()); | |||
| break; | |||
| case CompressionCodec::LZO: | |||
| decompressor_.reset(new Lz4Codec()); | |||
| break; | |||
There was a problem hiding this comment.
Does this patch also add LZO support?
There was a problem hiding this comment.
The decompressor was already available, but never initialized here. I'm going to rebase this patch after #38 and add unit tests for the decompression code path using a test fixture (unless someone else wants to do that)
There was a problem hiding this comment.
Hmm, I tried enabling Lz4Codec this way on the current trunk and it crashed. I think more work is needed to enable lzo support.
There was a problem hiding this comment.
Noted — thanks for trying it out so I know what to expect =)
|
I'm going to pick up this patch after the present patch queue is cleared, so later this week or early next week |
|
@knizhnik thanks again for getting this going. still some work remaining with this but should be done in the next week or so |
To be able to read files created using Spark I have to add Gzip compression support using zlib