Re: [PR] feat(storage): support separate zstd level for native parquet logs [hudi]

["hudi-agent (via GitHub)" <gi...@apache.org>]

hudi-agent commented on code in PR #19781:
URL: https://github.com/apache/hudi/pull/19781#discussion_r3901121902


##########
hudi-hadoop-common/src/test/java/org/apache/hudi/common/util/TestParquetUtils.java:
##########
@@ -90,6 +97,49 @@ public void setup() {
     parquetUtils = new ParquetUtils();
   }
 
+  @Test
+  void testApplyNativeLogZstdCompressionLevel() {
+    String parquetZstdLevel = "parquet.compression.codec.zstd.level";

Review Comment:
   🤖 nit: this string literal duplicates the private `PARQUET_COMPRESSION_CODEC_ZSTD_LEVEL` constant in `ParquetUtils`. If the constant changes, the test would silently pass against the wrong key. Could you either make the constant package-private (or `@VisibleForTesting`) so the test can reference it directly, or at least add a comment tying it back to the source constant?
   
   <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub>



##########
hudi-hadoop-common/src/main/java/org/apache/hudi/common/util/ParquetUtils.java:
##########
@@ -94,6 +97,32 @@
 @Slf4j
 public class ParquetUtils extends FileFormatUtils {
 
+  private static final String PARQUET_COMPRESSION_CODEC_ZSTD_LEVEL = "parquet.compression.codec.zstd.level";
+
+  /**
+   * Returns a storage configuration with the native Parquet log ZSTD compression level applied.
+   * The input configuration is copied only when its ZSTD level is absent or differs from the native log level,
+   * so base file writers and other users of the shared configuration are not affected.
+   */
+  public static <T> StorageConfiguration<T> applyNativeLogZstdCompressionLevel(
+      StoragePath path, StorageConfiguration<T> storageConf, HoodieConfig hoodieConfig) {

Review Comment:
   🤖 This applies the zstd level to every native log write regardless of the actual log compression codec (default is `gzip`). When the codec isn't zstd, setting `parquet.compression.codec.zstd.level` is a no-op but still forces a full `Configuration` deep-copy per log-writer creation. Since `hoodieConfig` already carries `PARQUET_COMPRESSION_CODEC_NAME`, would it make sense to gate this on the codec actually being zstd?
   
   <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:
##########
@@ -218,6 +218,14 @@ public class HoodieStorageConfig extends HoodieConfig {
       .defaultValue("gzip")
       .withDocumentation("Compression Codec for parquet files");
 
+  public static final ConfigProperty<String> LOGFILE_PARQUET_COMPRESSION_CODEC_ZSTD_LEVEL = ConfigProperty
+      .key("hoodie.logfile.parquet.compression.codec.zstd.level")
+      .defaultValue("1")

Review Comment:
   🤖 Since this defaults to `1` and `applyNativeLogZstdCompressionLevel` applies it whenever it differs from the global setting, existing tables using zstd for native log files will silently drop to level 1 on upgrade (from their configured global `parquet.compression.codec.zstd.level`, or parquet's default of 3 if unset). Is that intended for existing users, or should the override only kick in when this config is explicitly set and otherwise inherit the global level? Might be worth calling out as a behavior change either way.
   
   <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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org