[HUDI-5031] Fix MERGE INTO creates empty partition files when source table has partitions but target table does not #6983

weimingdiit · 2022-10-18T07:19:44Z

…rce table has partitions and the target table does not

Change Logs

Hudi merge into creates empty partition files when the source table has partitions and the target table does not

Impact

No impact for users, this is a internal code refactoring.

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

medium，see ut for detail

Documentation Update

Describe any necessary documentation update if there is any new feature, config, or user-facing change

The config description must be updated if new configs are added or the default value of the configs are changed
Any new feature or user-facing change requires updating the Hudi website. Please create a Jira ticket, attach the
ticket number here and follow the instruction to make
changes to the website.

Contributor's checklist

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

...ent/hudi-client-common/src/main/java/org/apache/hudi/execution/CopyOnWriteInsertHandler.java

YannByron · 2022-10-25T15:05:59Z

@weimingdiit i think only merge into can cause this issue, right?

weimingdiit · 2022-10-26T01:13:36Z

@weimingdiit i think only merge into can cause this issue, right?

@YannByron yes, it is

xushiyan · 2022-10-28T04:58:29Z

...ent/hudi-client-common/src/main/java/org/apache/hudi/execution/CopyOnWriteInsertHandler.java

-    if (handle == null) {
-      // If the records are sorted, this means that we encounter a new partition path
-      // and the records for the previous partition path are all written,
-      // so we can safely closely existing open handle to reduce memory footprint.
-      if (areRecordsSorted) {
-        closeOpenHandles();
+    if (handle == null || !handle.canWrite(payload.record)) {
+      if (handle == null) {


why change this logic flow? pls revert unnecessary changes

if no change this logic，Some code is duplicate:

in my first version :
first judge ignored record, other code no change，but every record need to judge,it is unnecessary

second version:
i move ignored record logic into if condition, but it need twice，if(handle == null) {...} and if(!handle.canWrite(payload.record)) {...}, and writeHandleFactory.create() also call twice, it is duplicate.

so i change this logic flow.

@xushiyan how about this? If worried about causing other hidden problems, I can change the code , revert logic flow and return early

xushiyan · 2022-10-28T05:01:45Z

...ent/hudi-client-common/src/main/java/org/apache/hudi/execution/CopyOnWriteInsertHandler.java

-          insertPayload.getPartitionPath(), idPrefix, taskContextSupplier);
+              insertPayload.getPartitionPath(), idPrefix, taskContextSupplier);


unnecessary change

xushiyan · 2022-10-28T05:11:44Z

...ent/hudi-client-common/src/main/java/org/apache/hudi/execution/CopyOnWriteInsertHandler.java

+      Option<IndexedRecord> insertRecord = payload.insertValue;
+      // just skip the ignored record，do not make partitions on fs
+      if (insertRecord.isPresent() && insertRecord.get().equals(IGNORE_RECORD)) {
+        return;
+      }


if you want to return based on these conditions, why not return early?

after judge handle, so you don't have to judge every Record, Just judge when the if condition is met，Most records do not need to be judged

@xushiyan how about that?

weimingdiit · 2022-11-21T08:37:35Z

@xushiyan @yihua @XuQianJin-Stars hi, please take a review for this pr. thks!

bvaradar · 2023-10-06T05:15:29Z

@weimingdiit : FYI: Have rebased and made slight changes. Will help take this to completion

…table has partitions but target table does not

hudi-bot · 2023-10-18T22:30:19Z

CI report:

003721a Azure: FAILURE
0066bfa Azure: PENDING

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

…table has partitions but target table does not (#6983) * [HUDI-5031] Fix MERGE INTO creates empty partition files when source table has partitions but target table does not Co-authored-by: jameswei <jameswei@didiglobal.com> Co-authored-by: balaji.varadarajan <vbalaji@apache.org>

nsivabalan assigned xushiyan Oct 21, 2022

nsivabalan added priority:major degraded perf; unable to move forward; potential bugs spark-sql labels Oct 21, 2022

KnightChess reviewed Oct 25, 2022

View reviewed changes

...ent/hudi-client-common/src/main/java/org/apache/hudi/execution/CopyOnWriteInsertHandler.java Outdated Show resolved Hide resolved

weimingdiit closed this Oct 25, 2022

weimingdiit reopened this Oct 25, 2022

weimingdiit force-pushed the HUDI-5031 branch from 58464a6 to f5a8b04 Compare October 28, 2022 03:24

xushiyan reviewed Oct 28, 2022

View reviewed changes

weimingdiit force-pushed the HUDI-5031 branch from f5a8b04 to 5d6e723 Compare November 4, 2022 04:29

XuQianJin-Stars requested review from KnightChess, yihua, XuQianJin-Stars and xushiyan and removed request for KnightChess November 21, 2022 01:41

weimingdiit force-pushed the HUDI-5031 branch from 5d6e723 to 862be37 Compare November 21, 2022 07:40

weimingdiit force-pushed the HUDI-5031 branch 2 times, most recently from 48ef299 to 139a4b2 Compare November 25, 2022 08:59

nsivabalan added the release-0.12.2 Patches targetted for 0.12.2 label Dec 6, 2022

codope changed the title ~~[HUDI-5031]Hudi merge into creates empty partition files when the sou…~~ [HUDI-5031] Fix MERGE INTO creates empty partition files when source table has partitions but target table does not Dec 7, 2022

weimingdiit force-pushed the HUDI-5031 branch from 139a4b2 to d1694b0 Compare December 21, 2022 08:38

weimingdiit force-pushed the HUDI-5031 branch from d1694b0 to d2f4ce7 Compare December 29, 2022 09:11

bvaradar added the bug-fix For PRs which fixes bugs label Oct 4, 2023

bvaradar force-pushed the HUDI-5031 branch from d2f4ce7 to 0593cd2 Compare October 6, 2023 05:13

bvaradar force-pushed the HUDI-5031 branch from 0593cd2 to 003721a Compare October 18, 2023 03:54

bvaradar approved these changes Oct 18, 2023

View reviewed changes

[HUDI-5031] Fix MERGE INTO creates empty partition files when source …

d806baf

…table has partitions but target table does not

bvaradar added 2 commits October 18, 2023 14:13

Track skipped records for assertion

f90e313

Use write config props

0066bfa

bvaradar force-pushed the HUDI-5031 branch from 003721a to 0066bfa Compare October 18, 2023 21:33

bvaradar merged commit 61051a3 into apache:master Oct 18, 2023
23 of 28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-5031] Fix MERGE INTO creates empty partition files when source table has partitions but target table does not #6983

[HUDI-5031] Fix MERGE INTO creates empty partition files when source table has partitions but target table does not #6983

weimingdiit commented Oct 18, 2022 •

edited

YannByron commented Oct 25, 2022

weimingdiit commented Oct 26, 2022 •

edited

xushiyan Oct 28, 2022

weimingdiit Oct 28, 2022

weimingdiit Oct 31, 2022

xushiyan Oct 28, 2022

xushiyan Oct 28, 2022

weimingdiit Oct 28, 2022 •

edited

weimingdiit Nov 16, 2022

weimingdiit commented Nov 21, 2022

bvaradar commented Oct 6, 2023

hudi-bot commented Oct 18, 2023

		insertPayload.getPartitionPath(), idPrefix, taskContextSupplier);
		insertPayload.getPartitionPath(), idPrefix, taskContextSupplier);

[HUDI-5031] Fix MERGE INTO creates empty partition files when source table has partitions but target table does not #6983

[HUDI-5031] Fix MERGE INTO creates empty partition files when source table has partitions but target table does not #6983

Conversation

weimingdiit commented Oct 18, 2022 • edited

Change Logs

Impact

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

Documentation Update

Contributor's checklist

YannByron commented Oct 25, 2022

weimingdiit commented Oct 26, 2022 • edited

xushiyan Oct 28, 2022

Choose a reason for hiding this comment

weimingdiit Oct 28, 2022

Choose a reason for hiding this comment

weimingdiit Oct 31, 2022

Choose a reason for hiding this comment

xushiyan Oct 28, 2022

Choose a reason for hiding this comment

xushiyan Oct 28, 2022

Choose a reason for hiding this comment

weimingdiit Oct 28, 2022 • edited

Choose a reason for hiding this comment

weimingdiit Nov 16, 2022

Choose a reason for hiding this comment

weimingdiit commented Nov 21, 2022

bvaradar commented Oct 6, 2023

hudi-bot commented Oct 18, 2023

CI report:

weimingdiit commented Oct 18, 2022 •

edited

weimingdiit commented Oct 26, 2022 •

edited

weimingdiit Oct 28, 2022 •

edited