[GitHub] [hudi] vinothchandar commented on a change in pull request #2106: [HUDI-1284] preCombine all HoodieRecords and update all fields according to orderingVal

[GitBox <gi...@apache.org>]

vinothchandar commented on a change in pull request #2106:
URL: https://github.com/apache/hudi/pull/2106#discussion_r503737912



##########
File path: hudi-common/src/main/java/org/apache/hudi/common/model/HoodieRecordPayload.java
##########
@@ -44,6 +44,22 @@
   @PublicAPIMethod(maturity = ApiMaturityLevel.STABLE)
   T preCombine(T another);
 
+  /**
+   * When more than one HoodieRecord have the same HoodieKey, this function combines all fields(which is not null)

Review comment:
       please keep this javadoc to be just about the preCombine() method, without any context from this PR;s scenarios. 

##########
File path: hudi-common/src/main/java/org/apache/hudi/common/model/HoodieRecordPayload.java
##########
@@ -44,6 +44,22 @@
   @PublicAPIMethod(maturity = ApiMaturityLevel.STABLE)
   T preCombine(T another);
 
+  /**
+   * When more than one HoodieRecord have the same HoodieKey, this function combines all fields(which is not null)

Review comment:
       We can have this javadoc in the actual impl 

##########
File path: hudi-common/src/main/java/org/apache/hudi/common/model/OverwriteWithLatestAvroPayload.java
##########
@@ -56,6 +77,18 @@ public OverwriteWithLatestAvroPayload preCombine(OverwriteWithLatestAvroPayload
     }
   }
 
+  public GenericRecord combineAllFields(List<Schema.Field> fields,GenericRecord priorRec,GenericRecord inferiorRoc) {

Review comment:
       we should def clean up variable names like `inferior`, in the interest of not having any naming with negative connotations. :) 

##########
File path: hudi-common/src/main/java/org/apache/hudi/common/model/OverwriteWithLatestAvroPayload.java
##########
@@ -46,6 +47,26 @@ public OverwriteWithLatestAvroPayload(Option<GenericRecord> record) {
     this(record.isPresent() ? record.get() : null, 0); // natural order
   }
 
+  @Override
+  public OverwriteWithLatestAvroPayload preCombine(OverwriteWithLatestAvroPayload another,Schema schema) throws IOException {

Review comment:
       cc @bvaradar who was exploring something similar in #2141 . My suggestion is to try and deprecate `preCombine` as opposed to adding a new overload. 




----------------------------------------------------------------
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.

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