danny0405 commented on code in PR #19781:
URL: https://github.com/apache/hudi/pull/19781#discussion_r3910843806
##########
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) {
+ if (!FSUtils.isNativeLogFile(path.getName())) {
+ return storageConf;
+ }
+
+ int nativeLogZstdLevel =
Review Comment:
Two small robustness improvements here: can we use Parquet's
`ZstandardCodec.PARQUET_COMPRESS_ZSTD_LEVEL` constant instead of duplicating
the key (or document why an older-Parquet compatibility constraint requires the
literal), and compare the existing value with the desired value as strings
instead of calling `Integer.parseInt`? A malformed inherited global value
should be replaceable by an explicit native-log override rather than failing
before the override is applied.
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieStorageConfig.java:
##########
@@ -218,6 +218,14 @@ public class HoodieStorageConfig extends HoodieConfig {
.defaultValue("gzip")
Review Comment:
Could this be a `ConfigProperty<Integer>` with `defaultValue(1)`? The
setting is numeric and is consumed through `getIntOrDefault`; using the typed
form would match Hudi's other numeric configs and make the contract clearer.
##########
hudi-client/hudi-flink-client/src/main/java/org/apache/hudi/io/storage/row/HoodieRowDataFileWriterFactory.java:
##########
@@ -98,7 +99,10 @@ public HoodieFileWriter newParquetFileWriter(
boolean populateMetaFields =
MetaFieldsMode.resolve(config).toLegacyPopulateMetaFields();
boolean withOperation =
config.getBooleanOrDefault(HoodieWriteConfig.ALLOW_OPERATION_METADATA_FIELD);
- Pair<StorageConfiguration, HoodieConfig> injectedConfigs =
HoodieParquetConfigInjector.applyConfigInjector(storagePath, storage.getConf(),
config);
+ StorageConfiguration storageConf =
+ ParquetUtils.applyNativeLogZstdCompressionLevel(storagePath,
storage.getConf(), config);
Review Comment:
Could we compose this built-in native-log override and the custom injector
behind one shared `prepareParquetWriterConfigs(...)` helper? The same two-step
setup is now repeated in the Flink, Spark, and Avro factories, so a future
Parquet writer path could easily miss it. Keeping the custom injector last
would preserve its ability to override the built-in setting.
--
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]
