hudi-agent commented on code in PR #19685:
URL: https://github.com/apache/hudi/pull/19685#discussion_r3870141188
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieStorageConfig.java:
##########
@@ -534,12 +549,47 @@ public String getBloomFilterType() {
return getString(BLOOM_FILTER_TYPE);
}
+ @VisibleForTesting
+ static String getDefaultParquetCompressionCodec(EngineType engineType) {
+ switch (engineType) {
+ case FLINK:
+ return ZSTD_COMPRESSION_CODEC;
+ case SPARK:
+ // Uses ZSTD as the default Parquet compression codec with Spark 3.5
and newer. Spark 3.3 and 3.4
+ // retain GZIP because the non-vectorized file-group reader uses
parquet-java 1.12.x and can leak
+ // off-heap memory when reading ZSTD files
https://issues.apache.org/jira/browse/PARQUET-2160.
+ Option<String> sparkVersion = getSparkRuntimeVersion();
+ return sparkVersion.isPresent()
+ && StringUtils.compareVersions(sparkVersion.get(),
MIN_SPARK_VERSION_WITH_ZSTD_DEFAULT) >= 0
+ ? ZSTD_COMPRESSION_CODEC : GZIP_COMPRESSION_CODEC;
+ default:
+ // The Java client does not own its Parquet runtime: Parquet
dependencies are provided by
+ // the embedding application, and older Parquet versions use Hadoop
native ZSTD rather than
+ // zstd-jni. For example, the recommended Kafka HDFS Connector 10.1.0
uses Parquet 1.11.1,
+ // which risks leaking memory when reading ZSTD-compressed files. Keep
GZIP as the portable
+ // default across supported Java deployments.
+ return GZIP_COMPRESSION_CODEC;
+ }
+ }
+
+ private static Option<String> getSparkRuntimeVersion() {
+ try {
+ Class<?> sparkPackageClass = Class.forName("org.apache.spark.package$");
+ Object sparkPackage = sparkPackageClass.getField("MODULE$").get(null);
+ return Option.of((String)
sparkPackageClass.getMethod("SPARK_VERSION").invoke(sparkPackage));
+ } catch (ReflectiveOperationException | LinkageError e) {
+ log.debug("Unable to resolve the Spark runtime version; using the legacy
Parquet compression codec default: {}", e.toString());
+ return Option.empty();
+ }
+ }
+
public static HoodieStorageConfig.Builder newBuilder() {
return new Builder();
}
public static class Builder {
+ private EngineType engineType = EngineType.SPARK;
Review Comment:
🤖 nit: since
`.parquetCompressionCodec(writeConfig.getParquetCompressionCodec())` is always
called explicitly at every callsite that resets the codec (e.g.
`SparkBinaryCopyClusteringExecutionStrategy`), the `engineType` field in the
Builder is effectively only consulted when no explicit value is set. That's the
intended fast-path, but worth a brief comment here (or on `build()`) explaining
that the engine default only fires when the codec key is absent from props —
could save a future reader some head-scratching.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieStorageConfig.java:
##########
@@ -534,12 +549,47 @@ public String getBloomFilterType() {
return getString(BLOOM_FILTER_TYPE);
}
+ @VisibleForTesting
+ static String getDefaultParquetCompressionCodec(EngineType engineType) {
+ switch (engineType) {
+ case FLINK:
+ return ZSTD_COMPRESSION_CODEC;
Review Comment:
🤖 nit: could you flip the name to `ZSTD_DEFAULT_MIN_SPARK_VERSION` (or
`MIN_SPARK_VERSION_FOR_ZSTD_DEFAULT`)? The current
`MIN_SPARK_VERSION_WITH_ZSTD_DEFAULT` reads as "minimum Spark version that has
a ZSTD default", which is accurate, but the more common Hudi constant style
puts the subject concept first — makes scanning constants easier.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieStorageConfig.java:
##########
Review Comment:
🤖 **Line 543:** The builder defaults engineType to SPARK, so any caller that
builds a HoodieStorageConfig without withEngineType(...) resolves the codec via
Spark-runtime reflection, making the effective default zstd on any classpath
that happens to have Spark 3.5+, even in a Java/Flink context. All three
production call sites are patched here, but would it be safer to default this
to GZIP when the engine is not explicitly provided, so a future caller that
forgets withEngineType fails safe rather than silently writing zstd?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
