nsivabalan commented on code in PR #13079:
URL: https://github.com/apache/hudi/pull/13079#discussion_r2027441289
##########
hudi-common/src/main/java/org/apache/hudi/common/model/HoodieRecordMerger.java:
##########
@@ -186,23 +187,28 @@ default String[] getMandatoryFieldsForMerging(Schema
dataSchema, HoodieTableConf
*/
String getMergingStrategy();
- static String getRecordMergeStrategyId(RecordMergeMode mergeMode,
- String payloadClassName,
- String recordMergeStrategyId) {
+ static String getRecordMergeStrategyId(RecordMergeMode mergeMode, String
payloadClassName,
+ String recordMergeStrategyId,
HoodieTableVersion tableVersion) {
switch (mergeMode) {
case COMMIT_TIME_ORDERING:
return COMMIT_TIME_BASED_MERGE_STRATEGY_UUID;
case EVENT_TIME_ORDERING:
return EVENT_TIME_BASED_MERGE_STRATEGY_UUID;
case CUSTOM:
default:
+ String stategyId = null;
Review Comment:
on 2nd thought, I have a question on tbl v 6.
Is the `else` condition even feasible?
in other words, why do we even allow table version 6 to be created with
custom merger w/o a payload class?
Likely its possible only when using 1.0 binary and not 0.15.0.
Should we just standardize the behavior and keep it same for tbl v 6 in 1.0.
and disallow this case.
##########
hudi-common/src/main/java/org/apache/hudi/common/model/HoodieRecordMerger.java:
##########
@@ -186,23 +187,28 @@ default String[] getMandatoryFieldsForMerging(Schema
dataSchema, HoodieTableConf
*/
String getMergingStrategy();
- static String getRecordMergeStrategyId(RecordMergeMode mergeMode,
- String payloadClassName,
- String recordMergeStrategyId) {
+ static String getRecordMergeStrategyId(RecordMergeMode mergeMode, String
payloadClassName,
+ String recordMergeStrategyId,
HoodieTableVersion tableVersion) {
switch (mergeMode) {
case COMMIT_TIME_ORDERING:
return COMMIT_TIME_BASED_MERGE_STRATEGY_UUID;
case EVENT_TIME_ORDERING:
return EVENT_TIME_BASED_MERGE_STRATEGY_UUID;
case CUSTOM:
default:
+ String stategyId = null;
Review Comment:
expected behavior
tbl v8:
if merge strategy id is non null: return the same.
else if payload is not null, return merge stra id for custom payload.
else null
tbl v6:
if payload is not null: return merge stra id for custom payload.
else if merge strategy id is non null: return the same.
else null
##########
hudi-common/src/main/java/org/apache/hudi/common/model/HoodieRecordMerger.java:
##########
@@ -186,23 +187,28 @@ default String[] getMandatoryFieldsForMerging(Schema
dataSchema, HoodieTableConf
*/
String getMergingStrategy();
- static String getRecordMergeStrategyId(RecordMergeMode mergeMode,
- String payloadClassName,
- String recordMergeStrategyId) {
+ static String getRecordMergeStrategyId(RecordMergeMode mergeMode, String
payloadClassName,
+ String recordMergeStrategyId,
HoodieTableVersion tableVersion) {
switch (mergeMode) {
case COMMIT_TIME_ORDERING:
return COMMIT_TIME_BASED_MERGE_STRATEGY_UUID;
case EVENT_TIME_ORDERING:
return EVENT_TIME_BASED_MERGE_STRATEGY_UUID;
case CUSTOM:
default:
+ String stategyId = null;
Review Comment:
then above logic might become simpler.
tbl v8:
if merge strategy id is non null: return the same.
else if payload is not null, return merge stra id for custom payload.
else null
tbl v6:
if payload is not null: return merge stra id for custom payload.
else null
--
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]
