PARQUET-2196: Support LZ4_RAW codec

[wgtmac]

This PR implements the LZ4_RAW codec which was introduced by parquet format v2.9.0. Since there are a lot of common logic between the LZ4_RAW and SNAPPY codecs, this patch moves them into NonBlockedCompressor and NonBlockedDecompressor and make the specific codec extend them.

Added TestLz4RawCodec test to make sure the new codec itself is correct.

[wgtmac]

@pitrou @shangxinli Can you please take a look? Thanks in advance!

[pitrou]

cc @lidavidm @emkornfield : could you give this a look?

[pitrou]

@wgtmac Did you try to read an actual file produced by Parquet C++?

Note you can find such files in https://github.com/apache/parquet-testing/

[wgtmac]

@wgtmac Did you try to read an actual file produced by Parquet C++?

Note you can find such files in https://github.com/apache/parquet-testing/

Yes, I have tried that. I will add some parquet files for compatibility test as well.

[shangxinli]

Thank Gang for contributing! Is there any benchmarking numbers? Any comparison with ZSTD? These are non-blocking questions for review and merging.

[shangxinli]

Nice implementation! For the test, can you add more for interop with lz4?

[wgtmac]

@shangxinli Thanks for the review.

I haven't be able to run the benchmark since the Hadoop Lz4Codec is implemented via JNI and I cannot simply run it on my Mac M1 laptop.

Generally this is the same LZ4 algorithm with the one shipped by Hadoop. The key difference is that LZ4_RAW has removed the redundant block header introduced by Hadoop Lz4Codec and its implementation is in pure Java.

I will add the interop test in a follow-up commit.

[wgtmac]

The interop test has been added. Please take a look again. Thanks! @shangxinli @pitrou

[pitrou]

It would probably be easier to use a git submodule (I wonder why that's not the approach being used in parquet-mr), but in any case reference files should go into the parquet-testing repo, not be privately-maintained in parquet-mr.

[ggershinsky]

Yep, we test encryption interop using binary files in the parquet-testing repo. @wgtmac Please have a look at this code: https://github.com/apache/parquet-mr/blob/master/parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestEncryptionOptions.java#L130

@ggershinsky @emkornfield Yes I did that in the previous commit: 6e02cb2#diff-61de8043597197e5da4b42ebfe2d123b9c42e56fcb520e293c1a10ba55fa93cc but switched to local file to avoid unstable test.

What was the source of the instability?
(encryption interop runs with files from the parquet-testing module for ~3 years now, no problems as far as I remember)

[wgtmac]

I have changed the approach of interop test to follow encryption which downloads test files from parquet-testing repo and verifies the data decompressed and decoded from them.

In addition, the codec test has been modified to be generic which supports LZ4_RAW and SNAPPY. It should be easy to support more codec types.

Please take a look when you have time and let me know if there is any feedback, thanks! @pitrou @emkornfield @ggershinsky @shangxinli

[pitrou]

I think both file extensions for LZ4 and LZ4RAW should be removed.
The reason is that the lz4 command-line utilities expect the LZ4 frame format.
In any case, the .lz4 extension should certainly not be used for the "Hadoop LZ4" compression codec.

[wgtmac]

I searched the parquet-mr repo and found the only usage of the codec extension as below:

https://github.com/apache/parquet-mr/blob/master/parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetOutputFormat.java#L420

  public RecordWriter<Void, T> getRecordWriter(TaskAttemptContext taskAttemptContext, Mode mode)
      throws IOException, InterruptedException {

    final Configuration conf = getConfiguration(taskAttemptContext);

    CompressionCodecName codec = getCodec(taskAttemptContext);
    String extension = codec.getExtension() + ".parquet";
    Path file = getDefaultWorkFile(taskAttemptContext, extension);
    return getRecordWriter(conf, file, codec, mode);
  }

The extension is part of the enum definition and cannot be removed. I propose to rename lz4 to lz4hadoop to explicitly differentiate this lz4 variant from standard lz4. @pitrou

[pitrou]

Sounds good to me.

[pitrou]

@lwhite1 Could you perhaps review this? I can't really comment on the details of the Java code, myself.

[shangxinli]

LGTM, let's see if there are comments from others, otherwise we can merge.

[lwhite1]

Looks good to me.