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.
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
[HUDI-3855] Fixing FILENAME_METADATA_FIELD
not being correctly updated in HoodieMergeHandle
#5296
Conversation
…mply old record is carried over
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good find on the gap. I should have caught this when I worked on compaction preserve metadata. my bad.
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieMergeHandle.java
Show resolved
Hide resolved
@hudi-bot run azure |
@@ -370,6 +360,16 @@ public void write(GenericRecord oldRecord) { | |||
} | |||
} | |||
|
|||
protected void writeToFile(HoodieKey key, GenericRecord avroRecord, boolean shouldPreserveRecordMetadata) throws IOException { | |||
if (shouldPreserveRecordMetadata) { | |||
// NOTE: `FILENAME_METADATA_FIELD` has to be rewritten to correctly point to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can create handle reuse this method ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally. Previously it relied on preserveMetadata
fields which were not available in CreateHandle
, so had to bail on moving it to WriteHandle
class, but then after refactoring forgot to update CreateHandle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do want to review all handles more holistically and cleanup quite a bit of duplication and unnecessary complication that we've currently amassed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is great if you can do that.
…ted in `HoodieMergeHandle` (#5296) Fixing FILENAME_METADATA_FIELD not being correctly updated in HoodieMergeHandle, in cases when old-record is carried over from existing file as is. - Revisited HoodieFileWriter API to accept HoodieKey instead of HoodieRecord - Fixed FILENAME_METADATA_FIELD not being overridden in cases when simply old record is carried over - Exposing standard JVM's debugger ports in Docker setup
Tips
What is the purpose of the pull request
Fixing
FILENAME_METADATA_FIELD
not being correctly updated inHoodieMergeHandle
, in cases when old-record is carried over from existing file as is.Brief change log
HoodieRecord
Verify this pull request
This pull request is already covered by existing tests, such as (please describe tests).
This change added tests and can be verified as follows:
Committer checklist
Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.