nsivabalan commented on code in PR #13517:
URL: https://github.com/apache/hudi/pull/13517#discussion_r2196398483
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/BufferedRecordMergerFactory.java:
##########
@@ -166,12 +166,12 @@ public Pair<Boolean, T> finalMerge(BufferedRecord<T>
olderRecord, BufferedRecord
if (!olderRecord.isCommitTimeOrderingDelete()
&& oldOrderingValue.compareTo(newOrderingValue) > 0) {
olderRecord = partialUpdateStrategy.reconcileFieldsWithOldRecord(
- olderRecord, newerRecord, readerSchema, readerSchema, true);
+ olderRecord, newerRecord, readerSchema, readerSchema, true, true);
Review Comment:
why is this hard coded to true. don't we need to support diff values for
enableEventTimeWaterMark to migrate diff payloads.
may be I am missing something here
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/BufferedRecord.java:
##########
@@ -45,13 +45,28 @@ public class BufferedRecord<T> implements Serializable {
private T record;
private final Integer schemaId;
private final boolean isDelete;
+ private Option<String> eventTimeOpt;
+
+ public BufferedRecord(String recordKey,
+ Comparable orderingValue,
+ T record,
+ Integer schemaId,
+ boolean isDelete) {
+ this(recordKey, orderingValue, record, schemaId, isDelete, Option.empty());
+ }
- public BufferedRecord(String recordKey, Comparable orderingValue, T record,
Integer schemaId, boolean isDelete) {
+ public BufferedRecord(String recordKey,
+ Comparable orderingValue,
+ T record,
+ Integer schemaId,
+ boolean isDelete,
+ Option<String> eventTimeOpt) {
Review Comment:
is the impl fully ready or not yet?
I see only caller to this constr is the other constr at L 50 which sets
Option.empty for eventTimeOpt.
##########
hudi-common/src/main/java/org/apache/hudi/avro/AvroReaderContextTypeHandler.java:
##########
@@ -36,4 +44,33 @@ public String castToString(Object value) {
}
return value.toString();
}
+
+ @Override
+ public Object convertValueForAvroLogicalTypes(Schema fieldSchema,
Review Comment:
did we move this as is from HoodieAvroUtils?
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/PartialUpdateStrategy.java:
##########
@@ -45,6 +54,15 @@ public PartialUpdateStrategy(HoodieReaderContext<T>
readerContext,
this.readerContext = readerContext;
this.partialUpdateMode = partialUpdateMode;
this.partialUpdateProperties = parsePartialUpdateProperties(props);
+ this.shouldKeepEventTimeMetadata = shouldKeepEventTimeMetadata(props);
Review Comment:
may I now why are we implementing diff values for eventTimeMetadata only in
PartialUpdateStrategy.
from the RFC given here
https://github.com/apache/hudi/pull/13499/files#diff-39244fd13d6f45250c46597036bbab6061e939373a2b42082c1d01f037877479
I only see the necessity of this eventtimeMetata for
DefaultHoodieRecordPayload. and for no other paylod.
which means that, its a full record merge based on ordering field. none of
the partial update mode should care about event time metadata.
--
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]
