HADOOP-19855. Replace native zstd C bindings with zstd-jni library#8399
HADOOP-19855. Replace native zstd C bindings with zstd-jni library#8399pan3793 wants to merge 6 commits intoapache:trunkfrom
Conversation
|
(!) A patch to the testing environment has been detected. |
|
(!) A patch to the testing environment has been detected. |
|
(!) A patch to the testing environment has been detected. |
|
(!) A patch to the testing environment has been detected. |
|
(!) A patch to the testing environment has been detected. |
| make the downstream pull it. --> | ||
| <!-- snappy-java/zstd-jni contains native library and cannot be relocated. | ||
| So we explicitly exclude it from shaded jar to prevent possible conflict. | ||
| Declare it as transitive dependency to make the downstream pull it. --> |
There was a problem hiding this comment.
in short, handling zst-jni in the same way as snappy-java
|
cc @sunchao @cnauroth @aajisaka @luben @dongjoon-hyun @cxzl25 @sandip-db I know this change is many years overdue because almost all downstream projects that use the Hadoop Codec have opted to implement a zstd codec based on |
aajisaka
left a comment
There was a problem hiding this comment.
Looks good to me, +1. Great improvement.
| } | ||
| } | ||
|
|
||
| public static boolean isNativeCodeLoaded() { |
There was a problem hiding this comment.
This class is not @Public, so I think it's okay to remove.
I saw 1 external usage in GitHub: https://github.com/trinodb/trino-hadoop-apache/blob/a1db88d0d14e4331034db0bcfb2dbc61d807db74/src/test/java/io/trino/hadoop/TestHadoopNative.java#L51. It's test only and it does not actually verify the zstd support.
There was a problem hiding this comment.
trino-hadoop-apache shouldn't be an issue, it's a dedicated project used by trino to override some hadoop class implementation, so it's easy to adapt the hadoop upstream change.
There was a problem hiding this comment.
Pull request overview
This PR migrates Hadoop’s ZStandard (zstd) compression implementation from custom native libzstd C/JNI bindings to the com.github.luben:zstd-jni library, removing the need to build/install libzstd as part of Hadoop native builds while keeping ZStandardCodec as the integration point.
Changes:
- Add
zstd-jnias a managed dependency and wire it into relevant client/runtime modules. - Replace native zstd compressor/decompressor implementations with zstd-jni-backed Java implementations.
- Remove zstd native build wiring (CMake, build properties, docker tooling/docs) and update unit tests to no longer require native zstd.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| hadoop-project/pom.xml | Adds zstd-jni.version and dependency management; removes zstd bundling/env setup. |
| hadoop-project-dist/pom.xml | Removes zstd native distribution bundling flags/args. |
| hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/pom.xml | Removes zstd-related native build properties/env vars; minor message formatting. |
| hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/compress/zstd/TestZStandardCompressorDecompressor.java | Removes native-availability assumptions so tests run with zstd-jni. |
| hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/compress/TestCompressionStreamReuse.java | Removes native-availability assumption for zstd reuse test. |
| hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/compress/TestCodec.java | Removes native-availability assumption for zstd sequence file test. |
| hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/NativeCodeLoader.c | Removes buildSupportsZstd() JNI implementation. |
| hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/zstd/ZStandardDecompressor.c | Deletes native zstd decompressor implementation. |
| hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/zstd/ZStandardCompressor.c | Deletes native zstd compressor implementation. |
| hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/zstd/org_apache_hadoop_io_compress_zstd.h | Deletes zstd native header. |
| hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/NativeLibraryChecker.java | Removes zstd native checks/output from checknative. |
| hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/NativeCodeLoader.java | Removes buildSupportsZstd() declaration. |
| hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/compress/zstd/ZStandardDecompressor.java | Reimplements decompression via ZstdDecompressCtx streaming API. |
| hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/compress/zstd/ZStandardCompressor.java | Reimplements compression via ZstdCompressCtx streaming API. |
| hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/compress/zstd/package-info.java | Updates zstd naming in package docs. |
| hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/compress/ZStandardCodec.java | Removes native-loading checks; reports library as zstd-jni. |
| hadoop-common-project/hadoop-common/src/config.h.cmake | Removes HADOOP_ZSTD_LIBRARY definition. |
| hadoop-common-project/hadoop-common/src/CMakeLists.txt | Removes zstd detection/build/linking from native build. |
| hadoop-common-project/hadoop-common/pom.xml | Adds zstd-jni dependency; removes zstd native build properties/args. |
| hadoop-client-modules/hadoop-client-runtime/pom.xml | Adds runtime dependency on zstd-jni; excludes from shading/relocation. |
| hadoop-client-modules/hadoop-client-minicluster/pom.xml | Adds runtime dependency on zstd-jni; excludes from shading/relocation. |
| hadoop-client-modules/hadoop-client-check-test-invariants/pom.xml | Updates invariant checks to exclude zstd-jni from relocation constraints. |
| hadoop-client-modules/hadoop-client-check-invariants/pom.xml | Updates invariant checks to exclude zstd-jni from relocation constraints. |
| hadoop-client-modules/hadoop-client-api/pom.xml | Adds transitive dependency on zstd-jni; excludes from shading/relocation. |
| dev-support/docker/pkg-resolver/packages.json | Removes libzstd-dev from base build package set. |
| dev-support/docker/pkg-resolver/install-zstandard.sh | Deletes zstd install helper script. |
| dev-support/docker/Dockerfile_rockylinux_8 | Removes zstd install step from docker image build. |
| BUILDING.txt | Removes zstd native dependency/build option documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| boolean allConsumed = (uncompressedDirectBufLen - uncompressedDirectBufOff <= 0); | ||
| // Use END only when finish=true, no more user data, and all direct-buffer | ||
| // data consumed (mirrors ZSTD_endStream); otherwise FLUSH (mirrors | ||
| // ZSTD_compressStream + ZSTD_flushStream). | ||
| boolean shouldEnd = finish && userBufLen == 0 && allConsumed; | ||
| if (!allConsumed || shouldEnd) { | ||
| // Re-initialize the output direct buffer |
There was a problem hiding this comment.
In compress(), when allConsumed is true and shouldEnd is false, the method skips calling the zstd streaming API and returns 0. This can break streaming output draining: zstd may still have pending compressed bytes to flush even when no more input is available (especially with small output buffers), and the old native implementation would continue flushing in this state. Consider always invoking the zstd stream with FLUSH whenever there is no remaining compressedDirectBuf output, even if there is currently no input, and use the returned done/state to decide when it’s safe to report needsInput() or produce 0 bytes.
There was a problem hiding this comment.
changed to always call FLUSH/END to drain any internally buffered bytes, even with no input, which mirrors how old native code always calls ZSTD_flushStream
| // zstd-jni: use streaming decompression directly on the provided buffers | ||
| int origDstPos = dst.position(); | ||
| boolean done = zstdJniCtx.decompressDirectByteBufferStream(dst, src); | ||
| if (done) { | ||
| finished = true; | ||
| remaining = 0; | ||
| } else { | ||
| src.position(src.limit()); | ||
| remaining = src.limit() - src.position(); | ||
| } |
There was a problem hiding this comment.
inflateDirect() sets finished=true and remaining=0 whenever done is true. If the source buffer contains concatenated frames or trailing bytes (i.e., src.position() < src.limit() after the call), this will misreport getRemaining() and can incorrectly signal end-of-stream while input bytes are still present. Align this logic with the non-direct decompress() path: compute remaining from src.limit() - src.position() and only set finished when done and remaining==0 (or otherwise explicitly document/handle concatenated frames).
There was a problem hiding this comment.
aligned finished/remaining logic with decompress() - only set finished when done && remaining==0
| @Override | ||
| protected void finalize() { | ||
| reset(); | ||
| } |
There was a problem hiding this comment.
finalize() currently calls reset() rather than end(). With the zstd-jni backend this risks leaking the native decompression context if callers forget to call end(), because reset() does not release the underlying native resources. If finalize() is retained, it should ensure the context is closed (and ideally delegate to super.finalize() as appropriate); alternatively, consider removing finalize() and relying on explicit end()/try-with-resources patterns if that matches project conventions.
There was a problem hiding this comment.
replaced with end().
I would keep finalize() in this PR to mirror the existing behavior. Removing it might be evaluated in the future
|
💔 -1 overall
This message was automatically generated. |
…ensure release resource
…lly buffered bytes, even with no input (mirrors how native always calls ZSTD_flushStream) Removed the early return n=0 when input is exhausted. The streaming API is now always called (with FLUSH or END), so any internally buffered bytes from a previous call where the output buffer was too small will continue to be drained — matching how the native ZSTD_flushStream was always invoked unconditionally.
…ly set finished when done && remaining==0)
|
💔 -1 overall
This message was automatically generated. |
|
follow yetus response to rebase on trunk (though it does not have conflicts) |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
charlesconnell
left a comment
There was a problem hiding this comment.
zstd-jni can operator on byte arrays, if you wish to avoid copying to/from ByteBuffers. This does come at the expense of potental JNI critical section stalls though.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@charlesconnell, thanks for your comments, I'd like to mirror the old behavior in this patch and post optimization or features (e.g., multi-threads) to later dedicated PRs. Actually, I want to port the PARQUET-1866 to the Hadoop codebase in the next PR |
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
Description of PR
Replaces Hadoop's custom native C bindings for ZStandard compression (
ZStandardCompressor.c,ZStandardDecompressor.c) with thecom.github.luben:zstd-jniJVM library. This eliminates the requirement to build or install libzstd natively, making ZStandard compression available on all platforms without a native build.This is similar to
zstd-jniis the de facto choice for JVM applications to the zStandard compression algorithm, it was adopted by many projects - Parquet, Spark, Avro, Kafka, HBase, ElasticSearch, etc.It's relatively painful to set up / upgrade native libraries on each Hadoop client and node, downstream projects like Parquet/Spark/HBase used to choose to implement their own zstd codec to overcome such a challenge, resulting in a lot of repetitive work:
zstd-jnihas been used and bundled by Hadoop,hadoop-3.5.0/share/hadoop/tools/lib/zstd-jni-1.5.6-4.jarHow was this patch tested?
Pass all existing UTs:
Manually tested the implementation is compatible with the existing Parquet/Spark zstd codec:
Compatible with Parquet ZstandardCodec PARQUET-1866
Modify Parquet to allow it to use Hadoop ZstandardCodec
https://github.com/apache/parquet-java/blob/apache-parquet-1.17.0/parquet-common/src/main/java/org/apache/parquet/hadoop/metadata/CompressionCodecName.java#L32
Then replace the Spark shipped
parquet-commonjar with the modified one.Create a Parquet Zstd table with Parquet ZstandardCodec
Create a Parquet Zstd table with Hadoop ZstandardCodec
Test two tables using different ZstandardCodec that can be read by each other
Compatiable with SPARK-52482
Apply SPARK-47829 first
Create a JSON zstd table using Hadoop ZstandardCodec
Try reading using Spark ZStdCompressionCodec, unable to read the table directly, but able to read using path syntax, it's by design.
(Use the unmodified Spark 4.1.1)
Compatible with
zstdCLIAble to use
zstdCLI to decompress the JSON file compressed by Hadoop ZstandardCodecpart-00000-1f9622e4-f4ce-47dc-95ac-512fa5acfd6b-c000.json.zst, and vice versa.For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?AI Tooling
This PR contains content generated by Claude Code (Sonnet 4.6).
where is the name of the AI tool used.
https://www.apache.org/legal/generative-tooling.html