hudi-agent commented on code in PR #19128:
URL: https://github.com/apache/hudi/pull/19128#discussion_r3503152789
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java:
##########
@@ -392,6 +421,11 @@ public Builder compactionSmallFileSize(long
smallFileLimitBytes) {
return this;
}
+ public Builder withParquetSmallFileFractionOfMaxFileSize(double fraction) {
+ compactionConfig.setValue(PARQUET_SMALL_FILE_FRACTION_MAX_FILE_SIZE,
String.valueOf(fraction));
Review Comment:
🤖 nit: the adjacent builder method is `compactionSmallFileSize` (no `with`
prefix), so `withParquetSmallFileFractionOfMaxFileSize` is inconsistent within
this builder. Could you either drop the `with` prefix here, or shorten it to
something like `parquetSmallFileLimitFraction` to match the style of the
surrounding methods?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java:
##########
@@ -105,7 +107,34 @@ public class HoodieCompactionConfig extends HoodieConfig {
.withDocumentation("During upsert operation, we opportunistically expand
existing small files on storage, instead of writing"
+ " new files, to keep number of files to an optimum. This config
sets the file size limit below which a file on storage "
+ " becomes a candidate to be selected as such a `small file`. By
default, treat any file <= 100MB as a small file."
- + " Also note that if this set <= 0, will not try to get small files
and directly write new files");
+ + " Also note that if this set <= 0, will not try to get small files
and directly write new files."
+ + " As of the fraction-based small-file config, this byte value is
no longer read directly at runtime — it feeds"
+ + " the infer function of
`hoodie.parquet.small.file.fraction.max.file.size` so tables that only set this
key keep"
+ + " working. If both this and the fraction config are set
explicitly, the fraction config takes precedence.");
+
+ public static final ConfigProperty<Double>
PARQUET_SMALL_FILE_FRACTION_MAX_FILE_SIZE = ConfigProperty
+ .key("hoodie.parquet.small.file.fraction.max.file.size")
+ .defaultValue(0.0)
+ .markAdvanced()
+ .sinceVersion("1.3.0")
+ .withInferFunction(cfg -> {
+ // Fall back to the built-in defaults of the byte-based configs when
the keys are not yet
+ // present in props. This happens when
HoodieCompactionConfig.Builder.build() runs
+ // standalone (before HoodieStorageConfig defaults are merged in the
outer
+ // HoodieWriteConfig.Builder.setDefaults path). Using *OrDefault
ensures the fraction is
+ // still correctly inferred as 100MB/120MB in that path.
+ long maxFileBytes =
cfg.getLongOrDefault(HoodieStorageConfig.PARQUET_MAX_FILE_SIZE);
Review Comment:
🤖 I think this breaks back-compat when a user sets a non-default
`max.file.size` AND passes a standalone `HoodieCompactionConfig` (the common
`withCompactionConfig(HoodieCompactionConfig.newBuilder()...build())` pattern).
`build()` runs `setDefaults` here while max is still the 120MB default, so the
fraction gets baked as 100MB/120MB≈0.833 into props. When that config is later
merged into a write config whose real max is e.g. 1GB, the fraction key is
already present so the outer `setDefaults` won't re-infer — and
`getParquetSmallFileLimit()` returns 0.833*1GB≈853MB instead of the intended
~100MB. Today this path returns a constant 100MB, so it's a regression. Could
you confirm? Case 6 in the test only passes because it uses the
default-compaction path where `fromProperties(writeConfig.getProps())`
propagates the real max before infer runs; the explicit-standalone path isn't
covered.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java:
##########
@@ -105,7 +107,34 @@ public class HoodieCompactionConfig extends HoodieConfig {
.withDocumentation("During upsert operation, we opportunistically expand
existing small files on storage, instead of writing"
+ " new files, to keep number of files to an optimum. This config
sets the file size limit below which a file on storage "
+ " becomes a candidate to be selected as such a `small file`. By
default, treat any file <= 100MB as a small file."
- + " Also note that if this set <= 0, will not try to get small files
and directly write new files");
+ + " Also note that if this set <= 0, will not try to get small files
and directly write new files."
+ + " As of the fraction-based small-file config, this byte value is
no longer read directly at runtime — it feeds"
+ + " the infer function of
`hoodie.parquet.small.file.fraction.max.file.size` so tables that only set this
key keep"
+ + " working. If both this and the fraction config are set
explicitly, the fraction config takes precedence.");
+
+ public static final ConfigProperty<Double>
PARQUET_SMALL_FILE_FRACTION_MAX_FILE_SIZE = ConfigProperty
+ .key("hoodie.parquet.small.file.fraction.max.file.size")
Review Comment:
🤖 nit: could you consider renaming the key to
`"hoodie.parquet.small.file.limit.fraction"`? The current
`fraction.max.file.size` suffix reads ambiguously — it's not clear whether the
key *is* a fraction or *is* some computed size. Keeping the existing
`small.file.limit` segment (matching `hoodie.parquet.small.file.limit`) and
appending `.fraction` makes the relationship and meaning immediately obvious.
<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]
