Skip to content

Commit 4b54090

Browse files
committed
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]> (cherry picked from commit 3c63bf7) Signed-off-by: Dongjoon Hyun <[email protected]>
1 parent 0f26b63 commit 4b54090

File tree

3 files changed

+23
-3
lines changed

3 files changed

+23
-3
lines changed

java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,7 @@ public PhysicalFsWriter(FSDataOutputStream outputStream,
116116
CompressionCodec codec = OrcCodecPool.getCodec(opts.getCompress());
117117
if (codec != null){
118118
CompressionCodec.Options tempOptions = codec.getDefaultOptions();
119-
if (codec instanceof ZstdCodec &&
120-
codec.getDefaultOptions() instanceof ZstdCodec.ZstdOptions options) {
119+
if (codec instanceof ZstdCodec && tempOptions instanceof ZstdCodec.ZstdOptions options) {
121120
OrcFile.ZstdCompressOptions zstdCompressOptions = opts.getZstdCompressOptions();
122121
if (zstdCompressOptions != null) {
123122
options.setLevel(zstdCompressOptions.getCompressionZstdLevel());

java/core/src/java/org/apache/orc/impl/ZstdCodec.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ public int hashCode() {
152152

153153
@Override
154154
public Options getDefaultOptions() {
155-
return DEFAULT_OPTIONS;
155+
return DEFAULT_OPTIONS.copy();
156156
}
157157

158158
/**

java/core/src/test/org/apache/orc/impl/TestPhysicalFsWriter.java

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@
2626
import org.apache.hadoop.fs.Path;
2727
import org.apache.hadoop.fs.permission.FsPermission;
2828
import org.apache.hadoop.util.Progressable;
29+
import org.apache.orc.CompressionCodec;
2930
import org.apache.orc.CompressionKind;
31+
import org.apache.orc.OrcConf;
3032
import org.apache.orc.OrcFile;
3133
import org.apache.orc.OrcProto;
3234
import org.apache.orc.PhysicalWriter;
@@ -330,4 +332,23 @@ public void testShortBlock() throws IOException {
330332
assertEquals(62 * 1024, dirEntry.getDataLength());
331333
assertEquals(endOfStripe, shim.lastShortBlock);
332334
}
335+
336+
@Test
337+
public void testZstdCodec() throws IOException {
338+
CompressionCodec zstdCodec = OrcCodecPool.getCodec(CompressionKind.ZSTD);
339+
int originalHashCode = zstdCodec.getDefaultOptions().hashCode();
340+
341+
Configuration conf = new Configuration();
342+
conf.setInt(OrcConf.COMPRESSION_ZSTD_LEVEL.getAttribute(), 9);
343+
MockHadoopShim shim = new MockHadoopShim();
344+
TypeDescription schema = TypeDescription.fromString("int");
345+
OrcFile.WriterOptions opts =
346+
OrcFile.writerOptions(conf)
347+
.compress(CompressionKind.ZSTD)
348+
.setSchema(schema)
349+
.setShims(shim);
350+
MemoryFileSystem fs = new MemoryFileSystem();
351+
PhysicalFsWriter writer = new PhysicalFsWriter(fs, new Path("test1.orc"), opts);
352+
assertEquals(originalHashCode, zstdCodec.getDefaultOptions().hashCode());
353+
}
333354
}

0 commit comments

Comments
 (0)