yihua commented on code in PR #19388:
URL: https://github.com/apache/hudi/pull/19388#discussion_r3667562081
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/ConcurrentSchemaEvolutionTableSchemaGetter.java:
##########
@@ -160,9 +170,11 @@ Option<Pair<HoodieInstant, HoodieSchema>>
getLastCommitMetadataWithValidSchemaFr
// the timeline finding a completed instant containing a valid schema.
ConcurrentHashMap<HoodieInstant, HoodieSchema> tableSchemaAtInstant = new
ConcurrentHashMap<>();
Option<HoodieInstant> instantWithTableSchema =
Option.fromJavaOptional(reversedTimelineStream
- // If a completion time is specified, find the first eligible instant
in the schema evolution timeline.
- // Should switch to completion time based.
- .filter(s -> instant.isEmpty() ||
compareTimestamps(s.getCompletionTime(), LESSER_THAN_OR_EQUALS,
instant.get().getCompletionTime()))
+ // Find the first eligible instant whose ordering time is no later
than the target instant's;
+ // a target instant without an ordering time (not completed yet, on
table version 8 and above)
+ // does not bound the lookup.
+ .filter(s -> instant.isEmpty() ||
StringUtils.isNullOrEmpty(getOrderingTime(instant.get()))
Review Comment:
`getOrderingTime` is cheap so extracting that to a variable is an overkill.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/ConcurrentSchemaEvolutionTableSchemaGetter.java:
##########
@@ -160,9 +170,11 @@ Option<Pair<HoodieInstant, HoodieSchema>>
getLastCommitMetadataWithValidSchemaFr
// the timeline finding a completed instant containing a valid schema.
ConcurrentHashMap<HoodieInstant, HoodieSchema> tableSchemaAtInstant = new
ConcurrentHashMap<>();
Option<HoodieInstant> instantWithTableSchema =
Option.fromJavaOptional(reversedTimelineStream
- // If a completion time is specified, find the first eligible instant
in the schema evolution timeline.
- // Should switch to completion time based.
- .filter(s -> instant.isEmpty() ||
compareTimestamps(s.getCompletionTime(), LESSER_THAN_OR_EQUALS,
instant.get().getCompletionTime()))
+ // Find the first eligible instant whose ordering time is no later
than the target instant's;
+ // a target instant without an ordering time (not completed yet, on
table version 8 and above)
+ // does not bound the lookup.
+ .filter(s -> instant.isEmpty() ||
StringUtils.isNullOrEmpty(getOrderingTime(instant.get()))
Review Comment:
`getOrderingTime` is cheap, so extracting that to a variable is overkill.
--
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]
