This is an automated email from the ASF dual-hosted git repository.
dongjoon pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/orc.git
The following commit(s) were added to refs/heads/main by this push:
new 3c63bf782 ORC-1942: Fix `PhysicalFsWriter` not to change `ZstdCodec`'s
default option
3c63bf782 is described below
commit 3c63bf782e0f94a8bc20561f31043e531893e601
Author: Dongjoon Hyun <[email protected]>
AuthorDate: Mon Jun 30 15:49:57 2025 -0700
ORC-1942: Fix `PhysicalFsWriter` not to change `ZstdCodec`'s default option
### What changes were proposed in this pull request?
This PR aims to fix `PhysicalFsWriter` to change `tempOptions` directly.
### Why are the changes needed?
In the following code path, `tempOptions` is supposed to be updated and
used. However, `codec.getDefaultOptions() instanceof ZstdCodec.ZstdOptions
options` code is currently updating the return value of
`codec.getDefaultOptions()` via a variable `options`.
https://github.com/apache/orc/blob/d3843bc043bea97bdd81a0f8e1fab3329efc7308/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java#L118-L127
Technically, `ZstdCodec.getDefaultOptions()` returns the final static
variable. This AS-IS code is trying to change this static object which leads
unintended side-effects. We should avoid this code pattern.
https://github.com/apache/orc/blob/d3843bc043bea97bdd81a0f8e1fab3329efc7308/java/core/src/java/org/apache/orc/impl/ZstdCodec.java#L150-L156
### How was this patch tested?
Pass the CIs with a newly added test case.
### Was this patch authored or co-authored using generative AI tooling?
No.
Closes #2304 from dongjoon-hyun/ORC-1942.
Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
---
.../java/org/apache/orc/impl/PhysicalFsWriter.java | 3 +--
.../src/java/org/apache/orc/impl/ZstdCodec.java | 2 +-
.../org/apache/orc/impl/TestPhysicalFsWriter.java | 21 +++++++++++++++++++++
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java
b/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java
index 4eb5f8562..87f777a7e 100644
--- a/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java
+++ b/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java
@@ -116,8 +116,7 @@ public class PhysicalFsWriter implements PhysicalWriter {
CompressionCodec codec = OrcCodecPool.getCodec(opts.getCompress());
if (codec != null){
CompressionCodec.Options tempOptions = codec.getDefaultOptions();
- if (codec instanceof ZstdCodec &&
- codec.getDefaultOptions() instanceof ZstdCodec.ZstdOptions
options) {
+ if (codec instanceof ZstdCodec && tempOptions instanceof
ZstdCodec.ZstdOptions options) {
OrcFile.ZstdCompressOptions zstdCompressOptions =
opts.getZstdCompressOptions();
if (zstdCompressOptions != null) {
options.setLevel(zstdCompressOptions.getCompressionZstdLevel());
diff --git a/java/core/src/java/org/apache/orc/impl/ZstdCodec.java
b/java/core/src/java/org/apache/orc/impl/ZstdCodec.java
index ab53ca0bd..d352c860f 100644
--- a/java/core/src/java/org/apache/orc/impl/ZstdCodec.java
+++ b/java/core/src/java/org/apache/orc/impl/ZstdCodec.java
@@ -152,7 +152,7 @@ public class ZstdCodec implements CompressionCodec,
DirectDecompressionCodec {
@Override
public Options getDefaultOptions() {
- return DEFAULT_OPTIONS;
+ return DEFAULT_OPTIONS.copy();
}
/**
diff --git a/java/core/src/test/org/apache/orc/impl/TestPhysicalFsWriter.java
b/java/core/src/test/org/apache/orc/impl/TestPhysicalFsWriter.java
index 9feac3104..62fcc80b3 100644
--- a/java/core/src/test/org/apache/orc/impl/TestPhysicalFsWriter.java
+++ b/java/core/src/test/org/apache/orc/impl/TestPhysicalFsWriter.java
@@ -26,7 +26,9 @@ import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.fs.permission.FsPermission;
import org.apache.hadoop.util.Progressable;
+import org.apache.orc.CompressionCodec;
import org.apache.orc.CompressionKind;
+import org.apache.orc.OrcConf;
import org.apache.orc.OrcFile;
import org.apache.orc.OrcProto;
import org.apache.orc.PhysicalWriter;
@@ -330,4 +332,23 @@ public class TestPhysicalFsWriter {
assertEquals(62 * 1024, dirEntry.getDataLength());
assertEquals(endOfStripe, shim.lastShortBlock);
}
+
+ @Test
+ public void testZstdCodec() throws IOException {
+ CompressionCodec zstdCodec = OrcCodecPool.getCodec(CompressionKind.ZSTD);
+ int originalHashCode = zstdCodec.getDefaultOptions().hashCode();
+
+ Configuration conf = new Configuration();
+ conf.setInt(OrcConf.COMPRESSION_ZSTD_LEVEL.getAttribute(), 9);
+ MockHadoopShim shim = new MockHadoopShim();
+ TypeDescription schema = TypeDescription.fromString("int");
+ OrcFile.WriterOptions opts =
+ OrcFile.writerOptions(conf)
+ .compress(CompressionKind.ZSTD)
+ .setSchema(schema)
+ .setShims(shim);
+ MemoryFileSystem fs = new MemoryFileSystem();
+ PhysicalFsWriter writer = new PhysicalFsWriter(fs, new Path("test1.orc"),
opts);
+ assertEquals(originalHashCode, zstdCodec.getDefaultOptions().hashCode());
+ }
}
