voonhous commented on code in PR #19388:
URL: https://github.com/apache/hudi/pull/19388#discussion_r3666737428
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/InstantComparatorV2.java:
##########
@@ -70,4 +70,14 @@ public Comparator<HoodieInstant>
requestedTimeOrderedComparator() {
public Comparator<HoodieInstant> completionTimeOrderedComparator() {
return COMPLETION_TIME_BASED_COMPARATOR;
}
+
+ @Override
+ public Comparator<HoodieInstant> orderingComparator() {
+ return COMPLETION_TIME_BASED_COMPARATOR;
+ }
+
+ @Override
+ public String getOrderingTime(HoodieInstant instant) {
+ return instant.getCompletionTime();
+ }
Review Comment:
Candid question on the v2 side. The argument for dropping completion-time
ordering on v1 is that those completion times are mtime-synthesized. But
`SevenToEightUpgradeHandler.upgradeActiveTimelineInstant` bakes that same mtime
straight into the v2 filename on upgrade:
```java
String completionTime = instant.getCompletionTime(); // this is the file
modification time
String startTime = instant.requestedTime();
replacedFileName = replacedFileName.replace(startTime, startTime +
UNDERSCORE + completionTime);
```
So on an upgraded 0.x table, everything before the upgrade boundary has a
*durable* mtime-derived completion time, and `isLegacy` doesn't survive the
rename, so it's indistinguishable from a real one. `orderingComparator()` on v2
orders that stretch of the timeline by exactly what this PR rejects on v1 --
and a restore or bulk copy can invert it against requested order, which is the
same disagreement `testTableVersionSixOrdersByRequestedTime` constructs by hand.
Not asking you to fix it here; the NPE fix and the v1 change stand on their
own. But is "completion-time ordering is correct on table version 8 and above"
actually true for upgraded tables, or is it accepted that they carry mtime
ordering permanently? If the latter, worth a javadoc note so the next reader
doesn't take `getOrderingTime()` on v2 as a guaranteed-durable key.
--
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]
