yihua commented on code in PR #18396:
URL: https://github.com/apache/hudi/pull/18396#discussion_r3036289387
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieMergeHandleFactory.java:
##########
@@ -67,13 +66,13 @@ public static <T, I, K, O> HoodieMergeHandle<T, I, K, O>
create(
log.info("Create HoodieMergeHandle implementation {} {}",
mergeHandleClasses.getLeft(), logContext);
Class<?>[] constructorParamTypes = new Class<?>[] {
- HoodieWriteConfig.class, String.class, HoodieTable.class,
Iterator.class,
+ HoodieWriteConfig.class, String.class, HoodieTable.class,
MergeContext.class,
Review Comment:
🤖 Changing the reflection-based constructor parameter types from
`Iterator.class` to `MergeContext.class` is a breaking change for any custom
merge handle implementation registered via config. Since `HoodieMergeHandle` is
`@PublicAPIClass(EVOLVING)`, this is permissible, but it might be worth calling
this out in the PR description or release notes so users with custom merge
handles know to update their constructors.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieWriteMergeHandle.java:
##########
@@ -137,7 +137,7 @@ public HoodieWriteMergeHandle(HoodieWriteConfig config,
String instantTime, Hood
Map<String, HoodieRecord<T>> keyToNewRecords,
String partitionPath, String fileId,
HoodieBaseFile dataFileToBeMerged,
TaskContextSupplier taskContextSupplier,
Option<BaseKeyGenerator> keyGeneratorOpt) {
- super(config, instantTime, hoodieTable, partitionPath, fileId,
taskContextSupplier, dataFileToBeMerged, keyGeneratorOpt,
+ super(config, instantTime, hoodieTable,
MergeContext.create(Collections.emptyIterator()), partitionPath, fileId,
taskContextSupplier, dataFileToBeMerged, keyGeneratorOpt,
Review Comment:
🤖 The compaction constructor creates
`MergeContext.create(Collections.emptyIterator())` without specifying
`numIncomingUpdates`. Could you confirm that this factory method defaults
`numIncomingUpdates` to `-1` (unknown)? If it defaults to `0`, downstream merge
handle logic could incorrectly interpret this as "zero updates expected" rather
than "unknown".
--
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]
