voonhous commented on code in PR #19388:
URL: https://github.com/apache/hudi/pull/19388#discussion_r3666951522
##########
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:
These two have to agree -- `getOrderingTime()` must be the primary sort key
of `orderingComparator()` -- but nothing in the javadoc says so, and nothing
enforces it.
The consumer relies on it:
`getLastCommitMetadataWithValidSchemaFromTimeline` sorts descending by the
comparator, then filters on `getOrderingTime(s) <= bound` and takes
`findFirst()`. That "sort, then first match" is only the right instant if the
two keys are the same. If an implementation ever overrode one and not the other
-- say comparator on requested time, `getOrderingTime` on completion time --
the walk returns the highest-*requested* instant under a *completion* bound.
Wrong schema, no exception, nothing in a stack trace to point at it.
Since they're separate abstract methods, that's a fairly easy mistake for
whoever adds a layout v3. Could you state the invariant on
`orderingComparator()`? Something like:
```java
/**
* Returns the comparator implementing the instant ordering of this timeline
version:
* completion-time based for v2, requested-time based for v1.
*
* <p>Implementations must keep this consistent with {@link
#getOrderingTime(HoodieInstant)},
* which returns the primary key this comparator orders by.
*/
```
--
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]
