hudi-agent commented on code in PR #19630:
URL: https://github.com/apache/hudi/pull/19630#discussion_r3786644497
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieStorageConfig.java:
##########
@@ -42,6 +42,14 @@ public class HoodieStorageConfig extends HoodieConfig {
.withDocumentation("Target size in bytes for parquet files produced by
Hudi write phases. "
+ "For DFS, this needs to be aligned with the underlying filesystem
block size for optimal performance.");
+ public static final ConfigProperty<String> NATIVE_LOG_MAX_FILE_SIZE =
ConfigProperty
Review Comment:
🤖 This config lives in hudi-common and the docs read as
format/engine-generic, but only the Flink `HoodieRowDataFileWriterFactory`
honors it — `HoodieSparkFileWriterFactory.newParquetFileWriter` still uses
`PARQUET_MAX_FILE_SIZE` unconditionally for native logs. Is the Flink-only
scope intentional? A Spark user setting `hoodie.native.log.max.file.size` would
see it silently ignored; might be worth narrowing the doc or noting the
limitation.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-flink-client/src/main/java/org/apache/hudi/io/storage/row/HoodieRowDataFileWriterFactory.java:
##########
@@ -141,12 +141,31 @@ public HoodieFileWriter newLanceFileWriter(
private static HoodieParquetConfig<HoodieRowDataParquetWriteSupport>
getParquetConfig(
HoodieConfig config, HoodieRowDataParquetWriteSupport writeSupport) {
+ return getParquetConfig(config, writeSupport,
+ config.getLongOrDefault(HoodieStorageConfig.PARQUET_MAX_FILE_SIZE));
+ }
+
+ private static HoodieParquetConfig<HoodieRowDataParquetWriteSupport>
getParquetConfig(
+ HoodieConfig config, HoodieRowDataParquetWriteSupport writeSupport,
StoragePath storagePath) {
+ // Native logs handled by this factory are write-once Parquet files whose
rollover is decided by
+ // the Parquet writer's canWrite(), not by LOGFILE_MAX_SIZE used for Hudi
log containers. Preserve
+ // the Parquet target as the fallback unless a dedicated native-log target
is explicitly configured.
Review Comment:
🤖 nit: with three overloads all named `getParquetConfig`, the `StoragePath`
variant's intent ("pick the right max size based on path") isn't obvious at a
glance — you have to read each signature to follow the dispatch chain. Have you
considered inlining the `maxFileSize` resolution directly in
`newParquetFileWriter` (a couple of lines before the existing call), and
keeping just the single long-taking overload? That would make the decision
visible at the call site without the indirection.
<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]
