linliu-code commented on code in PR #12597:
URL: https://github.com/apache/hudi/pull/12597#discussion_r1931357674
##########
hudi-common/src/main/java/org/apache/hudi/common/util/HoodieRecordUtils.java:
##########
@@ -76,6 +76,9 @@ public static HoodieRecordMerger loadRecordMerger(String
mergerClass) {
*/
public static HoodieRecordMerger createRecordMerger(String basePath,
EngineType engineType,
List<String>
mergerClassList, String recordMergerStrategy) {
+ // Currently we fall back to `HoodieAvroRecordMerger` (event time based)
even the specified merger strategy
+ // is commit time based. This behavior has been treated as the norm in
Hudi.
+ // TODO: evaluate the impact of this behavior and unify/simplify merge
behavior in Hudi repo.
Review Comment:
I found a bug in this logic. When we fall back to default merger, it does
not care about mergerStrategy, which could produce incorrect result. I cannot
fix it right, since the fixing could bring many CI tests to fail. So I left a
comment here to fix it in a later version.
--
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]
