[HUDI-6909] Use isDelete instead of isDeleted function for Spark merger #12007
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
linliu-code
changed the title
linliu-code
force-pushed
the
HUDI-6909_fix_a_bug_in_the_spark_merger
branch
from
b416e96
to
c08c321
Compare
danny0405
reviewed
Sep 26, 2024
| @@ -50,7 +50,7 @@ public Option<Pair<HoodieRecord, Schema>> merge(HoodieRecord older, Schema oldSc | |||
|
|
|||
| if (newer instanceof HoodieSparkRecord) { | |||
| HoodieSparkRecord newSparkRecord = (HoodieSparkRecord) newer; | |||
| if (newSparkRecord.isDeleted()) { | |||
| if (newSparkRecord.isDelete(newSchema, props)) { | |||
There was a problem hiding this comment.
We better add a UT for the merger.
There was a problem hiding this comment.
Ok, I will add a UT for this.
There was a problem hiding this comment.
the UT has been added.
linliu-code
force-pushed
the
HUDI-6909_fix_a_bug_in_the_spark_merger
branch
from
c08c321
to
4c37fd3
Compare
github-actions
bot
added
size:M
linliu-code
force-pushed
the
HUDI-6909_fix_a_bug_in_the_spark_merger
branch
from
4c37fd3
to
6c7a5c1
Compare
danny0405
reviewed
Sep 27, 2024
|
|
||
| class TestDefaultSparkRecordMerger { | ||
| static final String RECORD_KEY_FIELD_NAME = "record_key"; | ||
| static final String PARTITION_PATH_FIELD_NAME = "partition_path"; |
There was a problem hiding this comment.
I see there are also IT tests: TestHoodieMergeHandleWithSparkMerger, do we also need to add a test case there?
There was a problem hiding this comment.
No. the delete record with hoodieoperation is tested, and the fg reader has been used to read.
danny0405
reviewed
Sep 28, 2024
| * If the input records are not Spark record, it throws. | ||
| */ | ||
| @Test | ||
| void testMergerWithArvroRecord() { |
danny0405
reviewed
Sep 28, 2024
| * If the new record is a delete record, the merged record is empty. | ||
| */ | ||
| @Test | ||
| void testMergerWithNewRecordIsDeleteRecord() throws IOException { |
There was a problem hiding this comment.
testMergerWithNewRecordAsDelete ?
danny0405
reviewed
Sep 28, 2024
| * If the old record is a delete record, the merged record is the new record. | ||
| */ | ||
| @Test | ||
| void testMergerWithOldRecordIsDeleteRecord() throws IOException { |
There was a problem hiding this comment.
testMergerWithOldRecordAsDelete?
danny0405
approved these changes
Sep 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change Logs
isDeleted()only checks if the data == null.isDelete()returns true whenTherefore, remove
isDeleted()and its underlying variable.Impact
Clean the logic of determining delete record for Spark.
Risk level (write none, low medium or high below)
Medium.
Existing tests should be sufficient.
Documentation Update
Describe any necessary documentation update if there is any new feature, config, or user-facing change. If not, put "none".
ticket number here and follow the instruction to make
changes to the website.
Contributor's checklist