[GitHub] [hudi] danny0405 commented on a diff in pull request #9593: [HUDI-6784] Support deletion logic in merger

["danny0405 (via GitHub)" <gi...@apache.org>]

danny0405 commented on code in PR #9593:
URL: https://github.com/apache/hudi/pull/9593#discussion_r1338015255


##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieMergeHandle.java:
##########
@@ -270,14 +272,46 @@ protected boolean writeUpdateRecord(HoodieRecord<T> newRecord, HoodieRecord<T> o
     if (combineRecordOpt.isPresent()) {
       if (oldRecord.getData() != combineRecordOpt.get().getData()) {
         // the incoming record is chosen
-        isDelete = HoodieOperation.isDelete(newRecord.getOperation());
+        isDelete = is_delete_record(newRecord, writerSchema, config.getProps());
       } else {
         // the incoming record is dropped
         return false;
       }
       updatedRecordsWritten++;
     }
-    return writeRecord(newRecord, combineRecordOpt, writerSchema, config.getPayloadConfig().getProps(), isDelete);
+
+    // Do delete since the newRecord is a delete record.
+    if (isDelete) {
+      recordsDeleted++;
+      newRecord.unseal();
+      newRecord.clearNewLocation();
+      newRecord.seal();
+      newRecord.deflate();
+      return true;
+    }
+
+    // Inject custom insert/abort logic.
+    Option<Pair<HoodieRecord, Schema>> processedRecord = recordMerger.merge(
+        Option.empty(), writerSchema, combineRecordOpt, writerSchema, config.getProps());
+    if (!processedRecord.isPresent()
+        || !is_valid_record(processedRecord.get().getLeft(), writerSchema, config.getProps())) {
+      return false;
+    }
+
+    // Write the record finally.
+    // TODO: remove delete logic from writeRecord function.
+    return writeRecord(newRecord, Option.of(processedRecord.get().getLeft()), writerSchema, config.getPayloadConfig().getProps(), isDelete);
+  }
+
+  protected boolean is_delete_record(HoodieRecord record, Schema schema, TypedProperties props) throws IOException {
+    return record.isDelete(schema, props)
+        || record instanceof HoodieEmptyRecord
+        || (record.getData() != null && record.getData() instanceof EmptyHoodieRecordPayload)
+        || HoodieOperation.isDelete(record.getOperation());

Review Comment:
   What are the gains we bind all these check together ? Empty record and delete record are meant to be dropped but are they handled in the same logic originally?



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org