alexeykudinkin commented on code in PR #5296:
URL: https://github.com/apache/hudi/pull/5296#discussion_r848809893
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieMergeHandle.java:
##########
@@ -354,12 +349,7 @@ public void write(GenericRecord oldRecord) {
if (copyOldRecord) {
// this should work as it is, since this is an existing record
try {
- // rewrite file names
- // do not preserve FILENAME_METADATA_FIELD
- if (preserveMetadata && useWriterSchemaForCompaction) {
- oldRecord.put(HoodieRecord.FILENAME_METADATA_FIELD_POS,
newFilePath.getName());
- }
- fileWriter.writeAvro(key, oldRecord);
Review Comment:
There are obviously exceptions to this rule, but it requires guarding in
making sure that the in-place changes do not escape the scope they were
originally intended for
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieMergeHandle.java:
##########
@@ -354,12 +349,7 @@ public void write(GenericRecord oldRecord) {
if (copyOldRecord) {
// this should work as it is, since this is an existing record
try {
- // rewrite file names
- // do not preserve FILENAME_METADATA_FIELD
- if (preserveMetadata && useWriterSchemaForCompaction) {
- oldRecord.put(HoodieRecord.FILENAME_METADATA_FIELD_POS,
newFilePath.getName());
- }
- fileWriter.writeAvro(key, oldRecord);
Review Comment:
First and foremost is that we should not be updating passed in record in
place* (ie we should treat it as immutable). Rewriting the record does
essentially clone it, allowing us to modify it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
