[GitHub] [hudi] xushiyan commented on a change in pull request #4681: [HUDI-2987] Update all deprecated calls to new apis in HoodieRecordPayload

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

xushiyan commented on a change in pull request #4681:
URL: https://github.com/apache/hudi/pull/4681#discussion_r803653564



##########
File path: hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java
##########
@@ -288,6 +288,7 @@ private HoodieMetadataColumnStats combineColumnStatsMetadatat(HoodieMetadataPayl
     return this.columnStatMetadata;
   }
 
+  // HoodieMetadataPayload is an internal payload. Hence not implementing newer apis with Properties as last argument.

Review comment:
       for the sake of getting rid of deprecation, what is preventing us from migrating this API for metadata payload?

##########
File path: hudi-spark-datasource/hudi-spark/src/main/java/org/apache/hudi/payload/AWSDmsAvroPayload.java
##########
@@ -68,12 +69,25 @@ public AWSDmsAvroPayload(Option<GenericRecord> record) {
     return delete ? Option.empty() : Option.of(insertValue);
   }
 
+  @Override
+  public Option<IndexedRecord> getInsertValue(Schema schema, Properties properties) throws IOException {
+    IndexedRecord insertValue = super.getInsertValue(schema, properties).get();
+    return handleDeleteOperation(insertValue);

Review comment:
       similarly, shall we make the deprecated methods call this version?

##########
File path: hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/realtime/AbstractRealtimeRecordReader.java
##########
@@ -72,6 +75,9 @@ public AbstractRealtimeRecordReader(RealtimeSplit split, JobConf job) {
 
   private boolean usesCustomPayload() {
     HoodieTableMetaClient metaClient = HoodieTableMetaClient.builder().setConf(jobConf).setBasePath(split.getBasePath()).build();
+    if (metaClient.getTableConfig().getPreCombineField() != null) {
+      this.payloadProps.setProperty(HoodiePayloadProps.PAYLOAD_ORDERING_FIELD_PROP_KEY, metaClient.getTableConfig().getPreCombineField());
+    }

Review comment:
       not a good pattern to insert this logic into `usesCustomPayload()` since it introduces side effect. we can create metaclient in the constructor and use it to init payload props, then keep this method only does what it claims.

##########
File path: hudi-client/hudi-client-common/src/main/java/org/apache/hudi/common/table/log/HoodieFileSliceReader.java
##########
@@ -53,10 +55,12 @@ public static HoodieFileSliceReader getFileSliceReader(
       return new HoodieFileSliceReader(scanner.iterator());
     } else {
       Iterable<HoodieRecord<? extends HoodieRecordPayload>> iterable = () -> scanner.iterator();
+      // todo : wire in event time field as well
+      HoodiePayloadConfig payloadConfig = HoodiePayloadConfig.newBuilder().withPayloadOrderingField(preCombineField).build();

Review comment:
       just to point out this won't give us event time since it only contains ordering field. guess in this class we can't get event time config from users?

##########
File path: hudi-common/src/main/java/org/apache/hudi/common/model/debezium/PostgresDebeziumAvroPayload.java
##########
@@ -71,6 +72,19 @@ protected boolean shouldPickCurrentRecord(IndexedRecord currentRecord, IndexedRe
     return insertSourceLSN < currentSourceLSN;
   }
 
+  @Override
+  public Option<IndexedRecord> combineAndGetUpdateValue(IndexedRecord currentValue, Schema schema, Properties properties) throws IOException {
+    // Specific to Postgres: If the updated record has TOASTED columns,
+    // we will need to keep the previous value for those columns

Review comment:
       @nsivabalan why not make L89 method call this one with empty properties ? 




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