Skip to content

fixed delete record will always be chosen during merging,regardless of the ordering value #17668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
cbg-wx wants to merge 4 commits into from
Closed

fixed delete record will always be chosen during merging,regardless of the ordering value #17668

cbg-wx wants to merge 4 commits into from

Conversation

@cbg-wx
Copy link

@cbg-wx cbg-wx commented Dec 22, 2025 •
edited
Loading

Describe the issue this Pull Request addresses

  • This PR fixed HoodieMergedLogRecordScanner#processNextDeletedRecord, EventTimeAvroPayload#combineAndGetUpdateValue#needUpdatingPersistedRecord method, resovled merge delete record, tranform class issame before compareTo method when ordering field type is String or Decimal.
  • And add EventTimeAvroPayload#preCombine override this method to process delete record.

Summary and Changelog

This change involve the hudi-common module below method：

  • HoodieMergedLogRecordScanner#processNextDeletedRecord
  • EventTimeAvroPayload#combineAndGetUpdateValue#needUpdatingPersistedRecord
  • EventTimeAvroPayload#preCombine

Impact

  • Turn the delete record will always be chosen during merging,regardless of the ordering value into chose the record with max ordering value.
  • Tranform class issame before compareTo method when merged delete record with ordering field type is String or Decimal.

Risk Level

The logic that is not deleting records needs to be checked.

Documentation Update

none

Contributor's checklist

  • Read through contributor's guide
  • Enough context is provided in the sections above
  • Adequate tests were added if applicable

@cbg-wx
resolve When the ordering field of String type or Decimal type,and th…
e7e4c73 
…ese is a record marked as delete record by _hoodie_is_deleted=true,the delete record will always be chosen during merging,regardless of the ordering value.
@cbg-wx
Merge branch 'feature_0_14_1_delete_record_merge' of https://github.c…
476198b 
…om/cbg-wx/hudi into feature_0_14_1_delete_record_merge
@github-actions github-actions bot added the size:L PR with lines of changes in (300, 1000] label Dec 22, 2025
yihua
yihua reviewed Dec 23, 2025
View reviewed changes
hudi-common/src/main/java/org/apache/hudi/common/table/log/HoodieMergedLogRecordScanner.java Outdated

Comparable curOrderingVal = oldRecord.getOrderingValue(this.readerSchema, this.hoodieTableMetaClient.getTableConfig().getProps());
Comparable deleteOrderingVal = deleteRecord.getOrderingValue();
if (curOrderingVal instanceof Utf8){
Copy link
Contributor

@yihua yihua Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The payload classes are deprecated now (https://hudi.apache.org/releases/release-1.1.0#deprecation-of-hoodierecordpayload) and the old log record scanner is going to be removed. The new reader path with merge modes can handle the event time-based merging correctly. @cbg-wx could you check?

Copy link
Contributor

@danny0405 danny0405 Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yihua this is a PR for 0.x-branch, should be a critical fix though.

@danny0405
Copy link
Contributor

danny0405 commented Dec 23, 2025

cc @cshuo for reviewing this PR.

cshuo
cshuo reviewed Dec 23, 2025
View reviewed changes
Copy link
Collaborator

@cshuo cshuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thks @cbg-wx for contributing, left some comments. BTW, can you also format the PR title, you can refer to the doc here.

@github-actions github-actions bot added size:M PR with lines of changes in (100, 300] and removed size:L PR with lines of changes in (300, 1000] labels Dec 24, 2025
cshuo
cshuo reviewed Dec 24, 2025
View reviewed changes
@hudi-bot
Copy link
Collaborator

hudi-bot commented Dec 24, 2025

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@cshuo
Copy link
Collaborator

cshuo commented Dec 24, 2025

0001-fix-comments.patch

hi @cbg-wx, I've fixed some problems in the pr, could you apply the patch and push again. btw, plz use git rebase instead of git merge.

@cbg-wx cbg-wx closed this Dec 25, 2025
@cbg-wx cbg-wx deleted the feature_0_14_1_delete_record_merge branch December 25, 2025 07:15
@cbg-wx cbg-wx mentioned this pull request Dec 25, 2025
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yihua yihua yihua left review comments

@danny0405 danny0405 danny0405 left review comments

+1 more reviewer

@cshuo cshuo cshuo left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size:M PR with lines of changes in (100, 300]

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cbg-wx @danny0405 @hudi-bot @cshuo @yihua