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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
