danny0405 commented on code in PR #19685:
URL: https://github.com/apache/hudi/pull/19685#discussion_r3877626391
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieStorageConfig.java:
##########
@@ -760,7 +765,11 @@ public Builder withParquetConfigInjectorClass(String
parquetConfigInjectorClass)
}
public HoodieStorageConfig build() {
- storageConfig.setDefaults(HoodieStorageConfig.class.getName());
+ // The Parquet codec default depends on the write engine. Leave it unset
here so that
+ // HoodieWriteConfig can resolve it after the final engine type is
known. This also preserves
+ // whether callers explicitly configured the codec when a partial
storage config is passed on.
+ storageConfig.setDefaults(HoodieStorageConfig.class.getName(),
Review Comment:
Could we model this as a contextual default instead of excluding a declared
default during build? ConfigProperty already supports noDefaultValue(String
docOnDefaultValue), so this property can remain unset in HoodieStorageConfig
and be resolved by HoodieWriteConfig once engineType is known. Then build() can
use the normal setDefaults call, and an explicit codec in a partial storage
config is naturally preserved because HoodieWriteConfig.setDefaultValue does
not overwrite existing values. This also avoids the inconsistent state where
defaultValue() says zstd but HoodieStorageConfig.build() intentionally leaves
the property absent. Bare low-level writer paths that require a generic ZSTD
fallback can pass that fallback explicitly.
--
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]
