hudi-agent commented on code in PR #19388:
URL: https://github.com/apache/hudi/pull/19388#discussion_r3663265025
##########
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:
🤖 nit: `getOrderingTime(instant.get())` is evaluated twice in this lambda —
could you extract it to a local variable (`String targetOrderingTime =
getOrderingTime(instant.get())`) before the stream chain? It'd make the
condition a bit easier to scan.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/SimpleSchemaConflictResolutionStrategy.java:
##########
@@ -157,9 +155,9 @@ public Option<HoodieSchema>
resolveConcurrentSchemaEvolution(
}
private Option<HoodieInstant>
getInstantInTimelineImmediatelyPriorToTimestamp(
Review Comment:
🤖 nit: the method now internally calls
`schemaResolver.computeSchemaEvolutionTimelineInReverseOrder()`, so it does
more than its name suggests ("in timeline" no longer captures that the timeline
is also computed here). Something like
`getInstantImmediatelyPriorToOrderingTimestamp` might align better with the new
signature.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
