voonhous commented on code in PR #19293:
URL: https://github.com/apache/hudi/pull/19293#discussion_r3592543689
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/sink/partitioner/profile/DeltaWriteProfile.java:
##########
@@ -123,10 +123,9 @@ private boolean isSmallFile(FileSlice fileSlice) {
}
private double logFileToParquetCompressionRatio() {
- if (config.getLogDataBlockFormat().isPresent()
- && config.getLogDataBlockFormat().get() ==
HoodieLogBlock.HoodieLogBlockType.PARQUET_DATA_BLOCK) {
- return 1D;
+ if (config.getWriteVersion().lesserThan(HoodieTableVersion.TEN)) {
Review Comment:
One caveat on the gate: `getWriteVersion()` reads
`hoodie.write.table.version`, which defaults to `current()` (=10). Normally
that's reconciled to the on-disk version, but mid-upgrade (v9 -> v10) or with a
misconfigured writer a slice can still hold legacy inline Avro logs while this
reports `10` -- and we'd skip calibration for row-based logs that genuinely
need it. Might be worth a comment noting this assumes a reconciled write
version.
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/sink/partitioner/profile/DeltaWriteProfile.java:
##########
@@ -123,10 +123,9 @@ private boolean isSmallFile(FileSlice fileSlice) {
}
private double logFileToParquetCompressionRatio() {
- if (config.getLogDataBlockFormat().isPresent()
- && config.getLogDataBlockFormat().get() ==
HoodieLogBlock.HoodieLogBlockType.PARQUET_DATA_BLOCK) {
- return 1D;
+ if (config.getWriteVersion().lesserThan(HoodieTableVersion.TEN)) {
+ return config.getLogFileToParquetCompressionRatio();
}
- return config.getLogFileToParquetCompressionRatio();
+ return 1D;
Review Comment:
Worth flagging that Spark and Flink diverge after this change. The Spark
callers (`SparkUpsertDeltaCommitPartitioner`, `PartitionDirectoryConverter`)
pass `getLogFileToParquetCompressionRatio()` with no version gate, so on an
upgraded v10 slice that still has inline Avro logs Spark calibrates them and
Flink (forced to `1D`) doesn't.
Since `FileSlice` already routes native -> physical / inline -> ratio per
file, this gate is mostly redundant for the `getTotalFileSize` path -- it only
really matters for `calculateRecordSizeThroughCommitMetadata`, which isn't
per-file. Could we align the two engines, or drop a note on why they differ?
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/sink/partitioner/profile/DeltaWriteProfile.java:
##########
@@ -123,10 +123,9 @@ private boolean isSmallFile(FileSlice fileSlice) {
}
private double logFileToParquetCompressionRatio() {
- if (config.getLogDataBlockFormat().isPresent()
- && config.getLogDataBlockFormat().get() ==
HoodieLogBlock.HoodieLogBlockType.PARQUET_DATA_BLOCK) {
- return 1D;
+ if (config.getWriteVersion().lesserThan(HoodieTableVersion.TEN)) {
Review Comment:
This version gate quietly reverts the `PARQUET_DATA_BLOCK` skip added in
#18991. On a v9 table with `hoodie.logfile.data.block.format=parquet` (or
`hfile`) the log files are still inline (`isNativeLogFile() == false`), so we
hand back the ratio and re-calibrate blocks whose physical size is already
parquet-sized -- the exact over-calibration #18991 fixed. The removed
`testDeltaWriteProfileRecordsPerBucketSkipsCompressionRatioForParquetLogBlocks`
was guarding this.
If dropping v9 + non-Avro inline blocks is intentional (native logs being
the way forward), can we call that out in the PR description? If not, the
inline branch needs to look at the configured block format too, not just the
version -- `FileSlice` can't tell Avro from parquet inline blocks on its own,
since the format isn't in the file name.
--
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]
