codope commented on code in PR #19128:
URL: https://github.com/apache/hudi/pull/19128#discussion_r3568116213
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -1827,8 +1827,12 @@ public int getTimelineManifestRetainedVersions() {
return getInt(HoodieArchivalConfig.TIMELINE_MANIFEST_RETAINED_VERSIONS);
}
- public int getParquetSmallFileLimit() {
- return getInt(HoodieCompactionConfig.PARQUET_SMALL_FILE_LIMIT);
+ public long getParquetSmallFileLimit() {
+ // Effective threshold is fraction * max.file.size. The fraction config is
the single source of
+ // truth at runtime; it is either user-set or derived by its infer
function from the byte-based
+ // `hoodie.parquet.small.file.limit` at build time. See
HoodieCompactionConfig.
+ double fraction =
getDouble(HoodieCompactionConfig.PARQUET_SMALL_FILE_FRACTION_MAX_FILE_SIZE);
+ return (long) (fraction * getParquetMaxFileSize());
Review Comment:
Like I mentioned in
https://github.com/apache/hudi/pull/19128/changes#r3568112184 computing
fraction * max here can yield a wrong threshold in the standalone compaction
config path. We can instead do the derivation lazily with explicit precedence
so it's correct regardless of build order.
##########
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:
+1 we can probably even drop the parquet from the method name and simply
call it `compactionSmallFileFraction`
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -1827,8 +1827,12 @@ public int getTimelineManifestRetainedVersions() {
return getInt(HoodieArchivalConfig.TIMELINE_MANIFEST_RETAINED_VERSIONS);
}
- public int getParquetSmallFileLimit() {
- return getInt(HoodieCompactionConfig.PARQUET_SMALL_FILE_LIMIT);
+ public long getParquetSmallFileLimit() {
+ // Effective threshold is fraction * max.file.size. The fraction config is
the single source of
+ // truth at runtime; it is either user-set or derived by its infer
function from the byte-based
+ // `hoodie.parquet.small.file.limit` at build time. See
HoodieCompactionConfig.
+ double fraction =
getDouble(HoodieCompactionConfig.PARQUET_SMALL_FILE_FRACTION_MAX_FILE_SIZE);
Review Comment:
use `getDoubleOrDefault`
##########
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:
So, this infer reads `HoodieStorageConfig.PARQUET_MAX_FILE_SIZE`, but the
property lives in `HoodieCompactionConfig`. IIUC, infer functions run at each
config class's own build() and the result is frozen as an explicit property. So
when a caller builds a standalone HoodieCompactionConfig (the most common
pattern), storage config isn't in scope and
`getLongOrDefault(PARQUET_MAX_FILE_SIZE)` returns the built-in 120 MB, not the
user's actual max. And then the fraction is baked against 120 MB.
In HoodieWriteConfig.Builder.setDefaults() it's never re-derived:
`setDefaultOnCondition(!isCompactionConfigSet, …)` is skipped when the user
supplied the compaction config, and `setDefaults(HoodieWriteConfig.class)` only
iterates fields declared in that class. At runtime `getParquetSmallFileLimit()
= bakedFraction * actualMax`, i.e. wrong-fraction * correct-max.
This `.withCompactionConfig(HoodieCompactionConfig.newBuilder()…build())` +
non-default parquetMaxFileSize pattern spans across test files and prod
FlinkWriteClients.java which is very likely why the CI is failing.
We can drop the infer and do the derivation where the max is actually known,
lazily in `getParquetSmallFileLimit(`), or at the `HoodieWriteConfig` level
after storage is merged. Genrally, an infer that depends on a property from a
different config class does not sounds like a good idea.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/config/TestHoodieWriteConfig.java:
##########
@@ -234,6 +234,77 @@ public void testInferCleaningPolicy() {
assertEquals(HoodieCleaningPolicy.KEEP_LATEST_BY_HOURS,
writeConfig.getCleanerPolicy());
}
+ @Test
+ public void testParquetSmallFileFractionDerivation() {
Review Comment:
we should add a test covering common usage (standalone compaction config +
non-default max). Probably, it's already there that's why test failures in CI.
--
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]
