wombatu-kun commented on code in PR #19478:
URL: https://github.com/apache/hudi/pull/19478#discussion_r3709274176
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/split/HudiSplitFactory.java:
##########
@@ -127,7 +127,10 @@ private static List<HudiSplit> createSplitsForBaseFile(
}
ImmutableList.Builder<HudiSplit> splits = ImmutableList.builder();
- long targetSplitSizeInBytes = Math.max(targetSplitSize.toBytes(),
baseFile.getPathInfo().getBlockSize());
+ // Slicing is governed solely by the target split size; the block size
reported by
+ // storage is not meaningful on object stores and must not influence
split sizing.
+ long targetSplitSizeInBytes = targetSplitSize.toBytes();
Review Comment:
The guard sits after the `fileSize == 0` early return and is absent from
`createSplitForMergeOnRead`, so the same invalid target throws for one file
slice and silently passes for another. Validating once in the
`HudiSplitFactory` constructor next to the existing `requireNonNull` would
cover every path.
##########
hudi-trino/src/test/java/io/trino/plugin/hudi/split/TestHudiSplitFactory.java:
##########
@@ -192,6 +239,7 @@ private static FileSlice createFileSlice(DataSize
baseFileSize, Option<DataSize>
{
String fileId = "5a4f6a70-0306-40a8-952b-045b0d8ff0d4-0";
HoodieFileGroupId fileGroupId = new HoodieFileGroupId("partition",
fileId);
+ // Deliberately nonzero: split generation must ignore the reported
block size
Review Comment:
8MB is below the 128MB target, so
`testCreateHudiSplitsWithFileSmallerThanDefaultTarget` and
`testCreateHudiSplitsWithFileLargerThanDefaultTarget` pass on master unchanged
and do not pin the `max()` removal. Setting the fixture block size to the base
file size - what `convertToPathInfo` now reports - makes the 500MB case produce
one split without the fix and four with it.
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/split/HudiSplitFactory.java:
##########
@@ -127,7 +127,10 @@ private static List<HudiSplit> createSplitsForBaseFile(
}
ImmutableList.Builder<HudiSplit> splits = ImmutableList.builder();
- long targetSplitSizeInBytes = Math.max(targetSplitSize.toBytes(),
baseFile.getPathInfo().getBlockSize());
+ // Slicing is governed solely by the target split size; the block size
reported by
+ // storage is not meaningful on object stores and must not influence
split sizing.
+ long targetSplitSizeInBytes = targetSplitSize.toBytes();
+ checkArgument(targetSplitSizeInBytes > 0, "targetSplitSize must be
positive: %s", targetSplitSize);
Review Comment:
`hudi.target-split-size` and the `target_split_size` session property still
accept `0B`, so this only trips once split generation is already running and
surfaces as a raw IllegalArgumentException.
`PropertyMetadataUtil.validateMinDataSize` on the session property (the shape
already used for `parquet_small_file_threshold`) plus `@MinDataSize` on
`HudiConfig.getTargetSplitSize` would reject it at config time - follow-up, not
a blocker.
--
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]
