Fokko commented on code in PR #3880:
URL: https://github.com/apache/parquet-java/pull/3880#discussion_r4410333774
##########
parquet-hadoop/pom.xml:
##########
@@ -246,33 +252,4 @@
</plugins>
</build>
- <profiles>
- <!-- PARQUET-1975 Do not add brotli-codec for ARM64 architectures -->
- <profile>
- <id>non-aarch64</id>
- <activation>
- <os>
- <arch>!aarch64</arch>
- </os>
- </activation>
- <repositories>
- <repository>
- <id>jitpack.io</id>
- <url>https://jitpack.io</url>
- <name>Jitpack.io repository</name>
- <!-- needed for brotli-codec -->
- </repository>
- </repositories>
- <dependencies>
- <dependency>
- <groupId>com.github.rdblue</groupId>
Review Comment:
😆 @rdblue
##########
parquet-hadoop/pom.xml:
##########
@@ -174,6 +174,12 @@
<artifactId>zstd-jni</artifactId>
<version>${zstd-jni.version}</version>
</dependency>
+ <dependency>
+ <groupId>com.aayushatharva.brotli4j</groupId>
Review Comment:
Checked the package and it looks legit. Even it is optional, the licenses
are also in line with the ASF requirements.
##########
parquet-common/src/main/java/org/apache/parquet/hadoop/metadata/CompressionCodecName.java:
##########
@@ -27,7 +27,7 @@ public enum CompressionCodecName {
SNAPPY("org.apache.parquet.hadoop.codec.SnappyCodec",
CompressionCodec.SNAPPY, ".snappy"),
GZIP("org.apache.hadoop.io.compress.GzipCodec", CompressionCodec.GZIP,
".gz"),
LZO("com.hadoop.compression.lzo.LzoCodec", CompressionCodec.LZO, ".lzo"),
- BROTLI("org.apache.hadoop.io.compress.BrotliCodec", CompressionCodec.BROTLI,
".br"),
+ BROTLI("org.apache.parquet.hadoop.codec.BrotliCodec",
CompressionCodec.BROTLI, ".br"),
Review Comment:
Renaming the class here changes how
`CodecConfig.getHadoopCompressionCodec()` resolves
`mapred.output.compression.codec`. That goes through
`fromCompressionCodec(Class)`, which matches on the exact class name. A job
that still has
`mapred.output.compression.codec=org.apache.hadoop.io.compress.BrotliCodec` set
(with the old brotli-codec jar on its classpath) no longer matches. It hits the
`CompressionCodecNotSupportedException` catch in `CodecConfig` and **silently
writes UNCOMPRESSED**, with only a warning in the logs.
Could we keep the old class name as an alias? Here's a patch with a small
test; `TestBrotliCodec` and `TestDirectCodecFactory` still pass with it:
<details><summary>Patch</summary>
```diff
diff --git
a/parquet-common/src/main/java/org/apache/parquet/hadoop/metadata/CompressionCodecName.java
b/parquet-common/src/main/java/org/apache/parquet/hadoop/metadata/CompressionCodecName.java
index 5354e6c69..176328a38 100644
---
a/parquet-common/src/main/java/org/apache/parquet/hadoop/metadata/CompressionCodecName.java
+++
b/parquet-common/src/main/java/org/apache/parquet/hadoop/metadata/CompressionCodecName.java
@@ -39,17 +39,31 @@ public enum CompressionCodecName {
return valueOf(name.toUpperCase(Locale.ENGLISH));
}
+ // Brotli codec class from the removed jbrotli-based brotli-codec
artifact. Still accepted so that
+ // jobs configured through mapred.output.compression.codec keep writing
Brotli.
+ private static final String LEGACY_BROTLI_CODEC_CLASS =
"org.apache.hadoop.io.compress.BrotliCodec";
+
public static CompressionCodecName fromCompressionCodec(Class<?> clazz) {
if (clazz == null) {
return UNCOMPRESSED;
}
- String name = clazz.getName();
+ CompressionCodecName codec =
fromCompressionCodecClassName(clazz.getName());
+ if (codec == null) {
+ throw new CompressionCodecNotSupportedException(clazz);
+ }
+ return codec;
+ }
+
+ static CompressionCodecName fromCompressionCodecClassName(String name) {
for (CompressionCodecName codec : CompressionCodecName.values()) {
if (name.equals(codec.getHadoopCompressionCodecClassName())) {
return codec;
}
}
- throw new CompressionCodecNotSupportedException(clazz);
+ if (LEGACY_BROTLI_CODEC_CLASS.equals(name)) {
+ return BROTLI;
+ }
+ return null;
}
public static CompressionCodecName fromParquet(CompressionCodec codec) {
diff --git
a/parquet-common/src/test/java/org/apache/parquet/hadoop/metadata/TestCompressionCodecName.java
b/parquet-common/src/test/java/org/apache/parquet/hadoop/metadata/TestCompressionCodecName.java
new file mode 100644
index 000000000..5bb2145a5
--- /dev/null
+++
b/parquet-common/src/test/java/org/apache/parquet/hadoop/metadata/TestCompressionCodecName.java
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.hadoop.metadata;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import
org.apache.parquet.hadoop.codec.CompressionCodecNotSupportedException;
+import org.junit.jupiter.api.Test;
+
+public class TestCompressionCodecName {
+
+ @Test
+ public void testLegacyBrotliCodecClassMapsToBrotli() {
+
assertThat(CompressionCodecName.fromCompressionCodecClassName("org.apache.hadoop.io.compress.BrotliCodec"))
+ .isEqualTo(CompressionCodecName.BROTLI);
+ }
+
+ @Test
+ public void testBrotliCodecClassMapsToBrotli() {
+
assertThat(CompressionCodecName.fromCompressionCodecClassName("org.apache.parquet.hadoop.codec.BrotliCodec"))
+ .isEqualTo(CompressionCodecName.BROTLI);
+ }
+
+ @Test
+ public void testCodecClassNamesRoundTrip() {
+ for (CompressionCodecName codec : CompressionCodecName.values()) {
+ if (codec.getHadoopCompressionCodecClassName() != null) {
+ assertThat(CompressionCodecName.fromCompressionCodecClassName(
+ codec.getHadoopCompressionCodecClassName()))
+ .isEqualTo(codec);
+ }
+ }
+ }
+
+ @Test
+ public void testUnknownCodecClass() {
+
assertThat(CompressionCodecName.fromCompressionCodecClassName(String.class.getName()))
+ .isNull();
+ assertThatThrownBy(() ->
CompressionCodecName.fromCompressionCodec(String.class))
+ .isInstanceOf(CompressionCodecNotSupportedException.class);
+ }
+
+ @Test
+ public void testNullCodecClassIsUncompressed() {
+
assertThat(CompressionCodecName.fromCompressionCodec(null)).isEqualTo(CompressionCodecName.UNCOMPRESSED);
+ }
+}
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
