yihua commented on code in PR #11943:
URL: https://github.com/apache/hudi/pull/11943#discussion_r1805544464
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java:
##########
@@ -55,7 +57,7 @@ public abstract class HoodieReaderContext<T> {
private HoodieFileGroupReaderSchemaHandler<T> schemaHandler = null;
private String tablePath = null;
private String latestCommitTime = null;
- private HoodieRecordMerger recordMerger = null;
+ private Option<HoodieRecordMerger> recordMerger = null;
Review Comment:
`Option.empty()`?
##########
hudi-common/src/main/java/org/apache/hudi/common/model/HoodieAvroRecordMerger.java:
##########
@@ -39,7 +39,7 @@ public class HoodieAvroRecordMerger implements
HoodieRecordMerger, OperationMode
@Override
public String getMergingStrategy() {
- return HoodieRecordMerger.DEFAULT_MERGER_STRATEGY_UUID;
+ return HoodieRecordMerger.PAYLOAD_BASED_MERGER_STRATEGY_UUID;
Review Comment:
Should we disallow user setting the strategy ID
`PAYLOAD_BASED_MERGER_STRATEGY_UUID` and thrown an error if it is set in the
write config? `PAYLOAD_BASED_MERGER_STRATEGY_UUID` is reserved for internal
usage.
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieCommonConfig.java:
##########
@@ -82,7 +82,7 @@ public class HoodieCommonConfig extends HoodieConfig {
+ " operation will fail schema compatibility check. Set this option
to true will make the missing "
+ " column be filled with null values to successfully complete the
write operation.");
- public static final ConfigProperty<String> RECORD_MERGE_MODE =
HoodieTableConfig.RECORD_MERGE_MODE;
+ public static final ConfigProperty<RecordMergeMode> RECORD_MERGE_MODE =
HoodieTableConfig.RECORD_MERGE_MODE;
Review Comment:
Could we get rid on this now given that we have both the table and write
configs of merge mode?
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java:
##########
@@ -89,11 +91,11 @@ public void setLatestCommitTime(String latestCommitTime) {
this.latestCommitTime = latestCommitTime;
}
- public HoodieRecordMerger getRecordMerger() {
+ public Option<HoodieRecordMerger> getRecordMerger() {
return recordMerger;
}
- public void setRecordMerger(HoodieRecordMerger recordMerger) {
+ public void setRecordMerger(Option<HoodieRecordMerger> recordMerger) {
this.recordMerger = recordMerger;
}
Review Comment:
Should the argument avoid `Option`?
```suggestion
public void setRecordMerger(HoodieRecordMerger recordMerger) {
this.recordMerger = Option.of(recordMerger);
}
```
##########
hudi-client/hudi-spark-client/src/main/scala/org/apache/hudi/SparkFileFormatInternalRowReaderContext.scala:
##########
@@ -127,6 +129,14 @@ class
SparkFileFormatInternalRowReaderContext(parquetFileReader: SparkParquetRea
deserializer.deserialize(avroRecord).get.asInstanceOf[InternalRow]
}
+ override def convertToAvroRecord(record: InternalRow, schema: Schema):
GenericRecord = {
Review Comment:
Note to myself: revisit for integration and performance impact.
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieConfig.java:
##########
@@ -71,6 +71,10 @@ public <T> void setValue(String key, String val) {
props.setProperty(key, val);
}
+ public <T> void clearValue(ConfigProperty<T> cfg) {
+ props.remove(cfg.key());
Review Comment:
Should this consider all alternative config keys too?
##########
hudi-common/src/main/java/org/apache/hudi/common/model/DefaultHoodieRecordPayload.java:
##########
@@ -190,7 +190,8 @@ protected boolean needUpdatingPersistedRecord(IndexedRecord
currentValue,
Comparable incomingOrderingVal = (Comparable)
HoodieAvroUtils.getNestedFieldVal((GenericRecord) incomingRecord,
orderField,
true, consistentLogicalTimestampEnabled);
- return persistedOrderingVal == null || ((Comparable)
persistedOrderingVal).compareTo(incomingOrderingVal) <= 0;
+ return persistedOrderingVal == null || ((persistedOrderingVal instanceof
String || incomingOrderingVal instanceof String)
+ ? ((Comparable)
persistedOrderingVal.toString()).compareTo(incomingOrderingVal.toString()) <= 0
+ : ((Comparable) persistedOrderingVal).compareTo(incomingOrderingVal)
<= 0);
Review Comment:
To be safe, let's avoid any changes in existing payload classes.
--
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]
