yihua commented on code in PR #19388:
URL: https://github.com/apache/hudi/pull/19388#discussion_r3668482217
##########
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:
Done. Stated on `orderingComparator()` that implementations must keep it
consistent with `getOrderingTime()`, which returns the primary key it orders
by, and why: the sort-then-bound timeline walk relies on the two agreeing.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/TestConcurrentSchemaEvolutionTableSchemaGetter.java:
##########
@@ -403,6 +410,76 @@ void testGetTableSchema(HoodieSchema inputSchema, boolean
includeMetadataFields,
includeMetadataFields, Option.of(instant)).get());
}
+ @Test
+ void testTableVersionEightAndAboveOrdersByCompletionTime() throws Exception {
+ metaClient =
HoodieTestUtils.getMetaClientBuilder(HoodieTableType.COPY_ON_WRITE, new
Properties(), "")
+ .initTable(getDefaultStorageConf(), basePath);
+ // The ordering is driven by the timeline layout version.
+ assertEquals(TimelineLayoutVersion.VERSION_2,
metaClient.getTimelineLayoutVersion().getVersion());
+ testTable = HoodieTestTable.of(metaClient);
+
+ // Completion order inverts requested order: requested 001 completes last
(at 100) with
+ // schema 2, requested 009 completes first (at 050) with schema 1.
+ testTable.addCommit("001", Option.of("100"), Option.of(buildMetadata(
+ Collections.emptyList(), Collections.emptyMap(), Option.empty(),
WriteOperationType.UNKNOWN,
+ SCHEMA_WITHOUT_METADATA_STR2, COMMIT_ACTION)));
+ testTable.addCommit("009", Option.of("050"), Option.of(buildMetadata(
+ Collections.emptyList(), Collections.emptyMap(), Option.empty(),
WriteOperationType.UNKNOWN,
+ SCHEMA_WITHOUT_METADATA_STR, COMMIT_ACTION)));
+
+ ConcurrentSchemaEvolutionTableSchemaGetter resolver = new
ConcurrentSchemaEvolutionTableSchemaGetter(metaClient);
+ // The latest table schema follows completion time: schema 2 of requested
001, completed 100.
+ assertEquals(SCHEMA_WITHOUT_METADATA2.toString(),
+ resolver.getTableSchemaIfPresent(false,
Option.empty()).get().toString());
+ // A target completed at 075 only sees the commit completed at 050
(requested 009, schema 1).
+ assertEquals(SCHEMA_WITHOUT_METADATA.toString(),
+ resolver.getTableSchemaIfPresent(false,
+ Option.of(metaClient.getInstantGenerator().createNewInstant(
+ HoodieInstant.State.COMPLETED, COMMIT_ACTION, "005",
"075"))).get().toString());
+ }
+
+ @Test
+ void testTableVersionSixOrdersByRequestedTime() throws Exception {
+ Properties properties = new Properties();
+ properties.setProperty(WRITE_TABLE_VERSION.key(), "6");
+ metaClient =
HoodieTestUtils.getMetaClientBuilder(HoodieTableType.COPY_ON_WRITE, properties,
"")
+ .initTable(getDefaultStorageConf(), basePath);
+ // The ordering is driven by the timeline layout version.
+ assertEquals(TimelineLayoutVersion.VERSION_1,
metaClient.getTimelineLayoutVersion().getVersion());
+ testTable = HoodieTestTable.of(metaClient);
+
+ // Same layout as the table-version-8 test: requested 001 carries schema
2, requested 009
+ // carries schema 1. The completion times below are ignored by the
table-version-6
+ // (timeline layout v1) instant file naming.
+ testTable.addCommit("001", Option.of("100"), Option.of(buildMetadata(
+ Collections.emptyList(), Collections.emptyMap(), Option.empty(),
WriteOperationType.UNKNOWN,
+ SCHEMA_WITHOUT_METADATA_STR2, COMMIT_ACTION)));
+ testTable.addCommit("009", Option.of("050"), Option.of(buildMetadata(
+ Collections.emptyList(), Collections.emptyMap(), Option.empty(),
WriteOperationType.UNKNOWN,
+ SCHEMA_WITHOUT_METADATA_STR, COMMIT_ACTION)));
+ // Invert the file modification times so that the mtime-derived completion
order disagrees
+ // with the requested order, mirroring the table-version-8 fixture above.
+ Path timelinePath =
Paths.get(metaClient.getTimelinePath().makeQualified(new
URI("file:///")).toUri());
+ Files.setLastModifiedTime(timelinePath.resolve("001.commit"),
FileTime.fromMillis(2_000_000_000_000L));
+ Files.setLastModifiedTime(timelinePath.resolve("009.commit"),
FileTime.fromMillis(1_000_000_000_000L));
+ metaClient.reloadActiveTimeline();
Review Comment:
Done. Added an assertion right after the reload that the mtime-derived
completion time of `001` is greater than that of `009`, so the inversion has to
stick for the test to be meaningful.
##########
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:
Agreed, and it is intentional. Parameterized the resolution suite over table
version 6 and 8 (`@ValueSource(ints = {6, 8})`) so every RFC-82 case runs on
both layouts, replacing the one hand-picked v6 case. The fixture's requested
and completion orders agree, so the outcomes match across versions while the v1
(requested-time) ordering path is now exercised end to end.
--
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]
