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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
