yihua commented on code in PR #19388:
URL: https://github.com/apache/hudi/pull/19388#discussion_r3668346413
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v1/InstantComparatorV1.java:
##########
@@ -74,4 +74,14 @@ public Comparator<HoodieInstant>
requestedTimeOrderedComparator() {
public Comparator<HoodieInstant> completionTimeOrderedComparator() {
return COMPLETION_TIME_BASED_COMPARATOR;
}
+
+ @Override
+ public Comparator<HoodieInstant> orderingComparator() {
+ return REQUESTED_TIME_BASED_COMPARATOR;
+ }
Review Comment:
Adding a comment would be redundant, as it is known that the request time is
required for timeline layout V1.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/InstantComparator.java:
##########
@@ -37,4 +37,16 @@ public interface InstantComparator extends Serializable {
* @return {@link Comparator<HoodieInstant>} that orders primarily based on
completion time and secondary ordering based on {@link
#requestedTimeOrderedComparator()}.
*/
Comparator<HoodieInstant> completionTimeOrderedComparator();
+
+ /**
+ * Returns the comparator implementing the instant ordering of this timeline
version:
+ * completion-time based for v2, requested-time based for v1.
+ */
+ Comparator<HoodieInstant> orderingComparator();
+
+ /**
+ * Returns the timestamp ordering the given instant in this timeline
version: completion time
+ * for v2 (null if the instant is not completed yet), requested time for v1.
+ */
+ String getOrderingTime(HoodieInstant instant);
Review Comment:
Updating javadocs would be good.
##########
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()))
+ || compareTimestamps(getOrderingTime(s), LESSER_THAN_OR_EQUALS,
getOrderingTime(instant.get())))
Review Comment:
The current change is better for generalizing schema fetching.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/SimpleSchemaConflictResolutionStrategy.java:
##########
@@ -77,7 +75,7 @@ public Option<HoodieSchema> resolveConcurrentSchemaEvolution(
// schema and writer schema.
HoodieInstant lastCompletedInstantAtTxnStart =
lastCompletedTxnOwnerInstant.isPresent()
? getInstantInTimelineImmediatelyPriorToTimestamp(
- lastCompletedTxnOwnerInstant.get().getCompletionTime(),
schemaResolver.computeSchemaEvolutionTimelineInReverseOrder()).orElse(null)
+ schemaResolver.getOrderingTime(lastCompletedTxnOwnerInstant.get()),
schemaResolver).orElse(null)
Review Comment:
This is intentional. Good idea to parameterize the tests on both table v6
and v8.
--
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]
