feat(storage): support separate zstd level for native parquet logs - #19781
feat(storage): support separate zstd level for native parquet logs#19781cshuo wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19781 +/- ##
============================================
+ Coverage 78.11% 78.31% +0.19%
- Complexity 33673 33894 +221
============================================
Files 2540 2541 +1
Lines 141413 141759 +346
Branches 17123 17351 +228
============================================
+ Hits 110467 111019 +552
+ Misses 23250 23036 -214
- Partials 7696 7704 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR adds a hoodie.logfile.parquet.compression.codec.zstd.level config (default 1) and a ParquetUtils helper that applies it only to native Parquet log paths, wired into the Spark/Flink/Avro writer factories ahead of the custom injector. The mechanics look correct — newInstance() deep-copies so base-file configs aren't mutated, and native-vs-base path detection is right. One thing worth double-checking is the backward-compat implication of the default value, noted inline. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. One naming nit worth addressing; code is otherwise clean and well-structured.
| return storageConf; | ||
| } | ||
|
|
||
| int nativeLogZstdLevel = |
There was a problem hiding this 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.
There was a problem hiding this comment.
Fixed in 47c2d95. The code now uses ZstandardCodec.PARQUET_COMPRESS_ZSTD_LEVEL. We intentionally retain fail-fast validation for an explicitly malformed global value, with a clear HoodieException and dedicated test, rather than silently hiding user misconfiguration.
Addresses review comments 3901121902, 3910843797, 3910843801, and 3910843806.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR adds an advanced hoodie.logfile.parquet.compression.codec.zstd.level config (default 1) and a ParquetUtils helper that applies it only to native .log.parquet write paths, ahead of the custom config injector, across the Spark, Flink, and Avro/Java writer factories. I traced the native-log write path (HoodieNativeLogFormatWriter → HoodieFileWriterFactory.getFileWriter → the path-based newParquetFileWriter), confirmed newInstance() deep-copies so the shared/base-file config is never mutated, and confirmed the level change is compression-ratio-only with no storage-format or read-compat impact. No new issues flagged from this automated pass beyond the backward-compat default and codec-applicability points already raised in earlier rounds. A Hudi committer or PMC member can take it from here for a final review.
| * Prepares the storage and Hudi configurations used by a Parquet writer. Built-in writer configuration | ||
| * overrides are applied first so that the user-provided config injector remains the highest-priority extension point. | ||
| */ | ||
| public static Pair<StorageConfiguration, HoodieConfig> prepareParquetWriterConfigs( |
There was a problem hiding this comment.
-> ingectParquetWriterConfigs
There was a problem hiding this comment.
Fixed in 0d0fe35. Renamed the helper to injectParquetWriterConfigs and updated all Spark, Flink, Avro, and test call sites.
| * 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( |
There was a problem hiding this comment.
-> ingectDefaultZstdCompressionLevel
There was a problem hiding this comment.
Fixed in 0d0fe35. Renamed the helper to injectDefaultZstdCompressionLevel and updated the related tests and documentation.
Addresses review comments 3912563203 and 3912569450.
5 participants
Describe the issue this Pull Request addresses
Closes #19780.
The global Hadoop
parquet.compression.codec.zstd.levelsetting currently applies to both Parquet base files and native Parquet log files. Native logs favor write latency and may not need the higher compression level selected for base files, but there is no independent native-log setting.Summary and Changelog
hoodie.logfile.parquet.compression.codec.zstd.levelconfiguration with a default value of1.ParquetUtilshelper that applies the native-log level only to native log paths, copying the storage configuration only when the global level is absent or different.HoodieParquetConfigInjectorin Spark, Flink, and Avro/Java Parquet writer factories, preserving the custom injector as the highest-priority extension point.Impact
Users can tune native Parquet log Zstd compression independently from base files. Native logs default to level
1, while base files retain the global Hadoop setting. This adds one optional advanced configuration and does not change Hudi's storage format or public APIs.Risk Level
Low. The override is limited to native log paths, base-file paths retain the original configuration instance, and custom Parquet config injectors still run last. The targeted
TestParquetUtils#testApplyNativeLogZstdCompressionLeveland Flink Parquet writer factory test passed.Documentation Update
The new configuration and its default behavior are documented in
HoodieStorageConfig. No storage-format documentation changes are required.Contributor's checklist