[HUDI-6702][RFC-46] Support customized logic #9809

linliu-code · 2023-09-29T19:19:28Z

Change Logs

For Spark workflow, current HoodieRecordMerger interface does not support custom delete logic like the getInsertValue method does in HoodieRecordPayload interface.

This change fills the hole for the merger interface.
Functional tests are added to test the behavior the new method.

Impact

No impact on existing workflows.

Risk level (write none, low medium or high below)

Low.

Documentation Update

No need to update any public documenation.

Contributor's checklist

Read through contributor's guide
Change Logs and Impact were stated clearly
Adequate tests were added if applicable
CI passed

hudi-common/src/main/java/org/apache/hudi/common/model/HoodieRecordMerger.java

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieMergeHandle.java

linliu-code · 2023-10-02T21:46:32Z

@beyond1920, please check if this would work for your workflow.

codope

One comment, the rest looks good. Thanks for simplifying.

codope · 2023-10-03T02:10:43Z

hudi-common/src/main/java/org/apache/hudi/common/model/HoodieRecordMerger.java

+   *
+   * <p> This interface is experimental and might be evolved in the future.
+   **/
+  default Pair<Boolean, Boolean> shouldFlush(HoodieRecord record, Schema schema, TypedProperties props) throws IOException {


How's this going to evolve over time? What if users want to implement this method and Pair<Boolean, Boolean> is not sufficient? Then we add another method?
Also, what's the thinking behind the arguments of this method? If it's going to be simple enough to just decide whether to flush or not, do we need record, schema and props?

If it's going to be simple enough to just decide whether to flush or not

I kind of agree, we can simplify the returned value as a true/false. But maybe @linliu-code has some other considerations here, @linliu-code can you clarify.

Then we add another method?

I think we may change the method signature directly, by marking this method as expremental, we do not guarantee any compatibility in future versions. And because we have a default impl, so it should be feasible.

if we're only going to have 3 cases, can we use an enum instead? I think it can be more clear to the reader and future devs when implementing this method. The overhead of a new Pair being created vs the enum singletons can add up as well when processing 10s of thousands of records.

I kind of agree, we can simplify the returned value as a true/false. But maybe @linliu-code has some other considerations here, @linliu-code can you clarify.

@danny0405, @codope, the reason that we need a pair of boolean variables is that: if a merger decides not to flush the combined record, it faces the question if the old record (the record in the base file) should be kept or not. This question could be very critical, so we should not guess it for the developer who implements their custom merger.

if we're only going to have 3 cases, can we use an enum instead? I think it can be more clear to the reader and future devs when implementing this method. The overhead of a new Pair being created vs the enum singletons can add up as well when processing 10s of thousands of records.

@the-other-tim-brown, sounds a good point.

This question could be very critical,

I didn't see such request from any user, even for the contributor from Kuaishou, they just want to keep the merged record or drop it totally. Let's not introduce new semantics if there is no real use case as back-up.

We can evolve the returned value as a Pair or Enum if there are more feedbacks, at this time point, the behavior for keeping the old record seems not clear to me.

This question could be very critical,

I didn't see such request from any user, even for the contributor from Kuaishou, they just want to keep the merged record or drop it totally. Let's not introduce new semantics if there is no real use case as back-up.

We can evolve the returned value as a Pair or Enum if there are more feedbacks, at this time point, the behavior for keeping the old record seems not clear to me.

Even in current implementation of HoodieMergeHandle, we are still facing this problem: when the shouldFlush function returns false, should we return true or false in writeRecord function? Returning true means skipping the old record, false means keeping the old record. No matter which one we choose in advance, we still face the possible situation: what if a user wants the other way?

@danny0405 , saw your comment in the slack, we can use the simple signature right now since we can evolve it when we need in the future.

danny0405 · 2023-10-03T03:05:40Z

...atasource/hudi-spark/src/test/java/org/apache/hudi/TestHoodieMergeHandleWithSparkMerger.java

+  }
+
+  @Test
+  public void testShouldFlush1() throws Exception {


Can we give these method specific names instead of 1, 2, 3... suffix.

danny0405

Blocked for the clarification of the return type.

For Spark workflow, current HoodieRecordMerger interface does not support custom delete logic like the getInsertValue method does in HoodieRecordPayload interface. This change fills the hole for the merger interface.

linliu-code · 2023-10-03T23:05:19Z

Test failures seems not related. Ran failing tests in local, and they passed.

codope · 2023-10-04T09:48:13Z

@hudi-bot run azure

codope · 2023-10-05T14:08:53Z

Landing this PR. The integ test failure is due to a flaky test which will be fixed by HUDI-6917.

beyond1920 · 2023-10-07T09:57:56Z

@linliu-code @danny0405 Sorry for late response. I was just back from vacation.
The return type is true/false is enough.
But something is missed in this PR. For example, when read snapshot of MOR table, we should also check whether should return the records or not for those records which keys only existed in log files but does not existed in base file.
For example some methods in org.apache.hudi.LogFileIterator and org.apache.hudi.RecordMergingFileIterator.
It may lead to different query results for a MOR HUDI table before and after compaction.

danny0405 reviewed Sep 30, 2023

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/common/model/HoodieRecordMerger.java Outdated Show resolved Hide resolved

danny0405 reviewed Sep 30, 2023

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/common/model/HoodieRecordMerger.java Outdated Show resolved Hide resolved

danny0405 reviewed Sep 30, 2023

View reviewed changes

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieMergeHandle.java Outdated Show resolved Hide resolved

linliu-code force-pushed the HUDI-6702_support_custom_delete_logic_through_merger branch 2 times, most recently from 181594a to 9611675 Compare October 2, 2023 21:06

codope reviewed Oct 3, 2023

View reviewed changes

danny0405 reviewed Oct 3, 2023

View reviewed changes

linliu-code force-pushed the HUDI-6702_support_custom_delete_logic_through_merger branch 2 times, most recently from 3516d7c to a417df8 Compare October 3, 2023 07:23

codope approved these changes Oct 3, 2023

View reviewed changes

danny0405 requested changes Oct 3, 2023

View reviewed changes

[HUDI-6702][RFC-46] Support customized logic

a150f8a

For Spark workflow, current HoodieRecordMerger interface does not support custom delete logic like the getInsertValue method does in HoodieRecordPayload interface. This change fills the hole for the merger interface.

linliu-code force-pushed the HUDI-6702_support_custom_delete_logic_through_merger branch from a417df8 to a150f8a Compare October 3, 2023 16:54

danny0405 approved these changes Oct 4, 2023

View reviewed changes

codope merged commit 65cb387 into apache:master Oct 5, 2023

danny0405 mentioned this pull request Aug 13, 2025

[HUDI-9622] Add implementation of MergeHandle backed by the HoodieFileGroupReader #13699

Merged

4 tasks

hudi-bot mentioned this pull request Nov 30, 2025

Reiterate the custom validation support for custom merger #17150

Open

[HUDI-6702][RFC-46] Support customized logic #9809

[HUDI-6702][RFC-46] Support customized logic #9809

Uh oh!

Conversation

linliu-code commented Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Logs

Impact

Risk level (write none, low medium or high below)

Documentation Update

Contributor's checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

linliu-code commented Oct 2, 2023

Uh oh!

codope left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linliu-code Oct 3, 2023 • edited by danny0405 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danny0405 left a comment

Choose a reason for hiding this comment

Uh oh!

linliu-code commented Oct 3, 2023

Uh oh!

codope commented Oct 4, 2023

Uh oh!

codope commented Oct 5, 2023

Uh oh!

beyond1920 commented Oct 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

linliu-code commented Sep 29, 2023 •

edited

Loading

linliu-code Oct 3, 2023 •

edited by danny0405

Loading

beyond1920 commented Oct 7, 2023 •

edited

Loading