danny0405 commented on code in PR #13469:
URL: https://github.com/apache/hudi/pull/13469#discussion_r2159839744
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/HoodieFileGroupReader.java:
##########
@@ -158,16 +158,12 @@ private HoodieFileGroupReader(HoodieReaderContext<T>
readerContext, HoodieStorag
? new PositionBasedSchemaHandler<>(readerContext, dataSchema,
requestedSchema, internalSchemaOpt, tableConfig, props)
: new FileGroupReaderSchemaHandler<>(readerContext, dataSchema,
requestedSchema, internalSchemaOpt, tableConfig, props));
this.outputConverter =
readerContext.getSchemaHandler().getOutputConverter();
- this.orderingFieldName = recordMergeMode ==
RecordMergeMode.COMMIT_TIME_ORDERING
- ? Option.empty()
- : Option.ofNullable(ConfigUtils.getOrderingField(props))
Review Comment:
The current ordering value field priority sequence is:
1. hoodie.payload.ordering.field
2. hoodie.datasource.write.precombine.field
3. hoodie.table.precombine.field
That is the `#2` has higher priority than `#3` let's keep that for backward
compatibility. So I think the logic is fine here, we just need to supplement
the `props` for `#2` and `#3`. `#1` is deprecated and let's remove the support
for it for file group reader.
One more thing I can think of is to tweak the check priority in
ConfigUtils#etOrderingField: the `hoodie.payload.ordering.field` is deprecated
and should be put in the last check(the check happens in row-level and is
performance sensitive, each check has a hashKey computation for the keys).
Also we should fix the legacy log scanner to better use
`hoodie.datasource.write.precombine.field` instead.
--
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]
