ORC-817: Replace aircompressor ZStandard compression with zstd-jni

[dchristle]

What changes were proposed in this pull request?

This PR proposes to replace the aircompressor library for ORC's ZStandard compression with zstd-jni, which is a set of JNI bindings around the official zstd library. In addition to switching the underlying library, this PR also exposes the compression level and "long mode" settings to ORC users. These settings allow user choice around different speed/compression tradeoffs, rather than the current approach that primarily uses a default setting.

Why are the changes needed?

These change makes sense for a few reasons:

• ORC users will gain all the improvements from the main zstd library. It is under active development and receives regular speed and compression improvements. In contrast, aircompressor's zstd implementation is older and stale.
• ORC users will be able to use the entire speed/compression tradeoff space. Today, aircompressor's implementation has only one of eight compression strategies (link). This means only a small range of faster but less compressive strategies can be exposed to ORC users. ORC storage with high compression (e.g. for large-but-infrequently-used data) is a clear use case that this PR would unlock.
• It will harmonize the Java ORC implementation with other projects in the Hadoop ecosystem. Parquet, Spark, and even the C++ ORC reader/writers all rely on the official zstd implementation either via zstd-jni or directly. In this way, the Java reader/writer code is an outlier.
• Detection and fixing any bugs or regressions will generally happen much faster, given the larger number of users and active developer community of zstd and zstd-jni.

The largest tradeoff is that zstd-jni wraps compiled code. That said, many microprocessor architectures are already targeted & bundled into zstd-jni, so this should be a rare hurdle.

Open issues:

• What is the best way to expose codec-specific options to users? In this PR, we add the compression level, window log size, and a boolean for enabling long mode, as new conf settings. But the CompressionCodec interface seems limited to exposing an enum with 3 options for speed, e.g. FAST or DEFAULT, and other codec-specific configs don't have a clear way to make it down into the codec implementation itself. I think we want to allow users to set the actual level as an integer, and to specify the window log size & long mode boolean as they wish. It wasn't clear how I could communicate these confs down to the lower level ZstdCodec within the bounds of the existing CompressionCodec interface. Right now, I used a hack to get the codec to read the conf options.
• I still need to implement the DirectByteBuffer handling case. Right now, each call is treated the same way and will incur unnecessary copying if the input ByteBuffer is direct.
• The existing code has a loop structure to repeatedly decompress. I wasn't sure why this exists, but we should mimic it in this PR, and I haven't done that yet.
• Benchmarks should be added to this PR description. Although zstd-jni should have superior performance across the board, it's important to actually measure that with the benchmark suite.

List of changes:

• Add zstd-jni dependency, and add a new CompressionCodec ZstdCodec that uses it. Add ORC conf to set compression level.

• Add ORC conf to use long mode, and add configuration setters for windowLog and longModeEnable.

• Add tests that verify the correctness of writing and reading across compression levels, window sizes, and long mode use.

• Add test for compatibility between Zstd aircompressor and zstd-jni implementations.

• Fix filterWithSeek test with a smaller percentage.

• Minor formatting and spelling fixes.

How was this patch tested?

• Unit tests for reading and writing ORC files using a variety of compression levels, window logs, and long mode booleans, all pass.
• Unit test to compress and decompress between aircompressor and zstd-jni passes. Note that the current aircompressor implementation uses a small subset of levels, so the test only compares data using the default compression settings.

@dongjoon-hyun @wgtmac

[dongjoon-hyun]

This should be extremely carefully handled, @dchristle .
I'm not considering this at Apache ORC 1.8.0 timeframe.

[dongjoon-hyun]

Please provide a performance benchmark result at least.

[omalley]

We need to leave the pure java library as an option with automatic fallback because not every platform will have the native library. Furthermore, if there is a bug in the aircompresssor codec, we may need to configure it for reading old files.

I also second Dongjoon's concern about benchmarks. JNI can have a positive impact, but it can also have negative impacts. That is especially true if non-native ByteBuffers (or byte arrays) are used.

[github-actions]

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

[tomscut]

Hi @dchristle , do you have a performance benchmark result? Thanks.

[cxzl25]

I did some tests based on this PR. Under the same Spark resources, zstd-jni compression is faster than aircompressor.

Here is also a performance comparison. opensearch-project/OpenSearch#9422 (comment)

spark

spark.executor.instances=600
spark.executor.cores=3

use aircompressor

use zstd-jni

[wgtmac]

Thanks for providing the benchmark result. @cxzl25

As @omalley has commented, it would be good to use zstd-jni by default but we have to keep the pure java impl as a fallback.

[cxzl25]

it would be good to use zstd-jni by default but we have to keep the pure java impl as a fallback.

I think we should be able to achieve this by automatically fallback, such as the following code,
or providing configuration items, so that the user can specify the implementation of a certain zstd.

org.apache.orc.impl.WriterImpl

  static {
    try {
      Native.load();
    } catch (UnsatisfiedLinkError | ExceptionInInitializerError e) {
    }
  }

  public static CompressionCodec createCodec(CompressionKind kind) {
    ...
      case ZSTD:
        if (Native.isLoaded()) {
          return new ZstdCodec();
        } else {
          return new AircompressorCodec(kind, new ZstdCompressor(),
                  new ZstdDecompressor());
        }

[dongjoon-hyun]

+1 for that fallback.

Could you take over this PR, @cxzl25 ?
I can wait for your PR to include it to Apache ORC 2.0.0.

[dongjoon-hyun]

BTW, we need more time to test because zstd-jni has memory issues depending on its usage.
IIRC, it was reported in Parquet and Spark community before. It's hard to detect memory-leak.