yihua commented on code in PR #19388:
URL: https://github.com/apache/hudi/pull/19388#discussion_r3668480888
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/TestSimpleSchemaConflictResolutionStrategy.java:
##########
@@ -156,6 +164,66 @@ void testNullTypeWriterSchema() throws Exception {
assertEquals(HoodieSchema.parse(SCHEMA1), result);
}
+ @Test
+ void testNullTypeWriterSchemaCurrTxnInstantWithoutCompletionTime() throws
Exception {
+ setupInstants(SCHEMA1, SCHEMA2, NULL_SCHEMA, true, false);
+ // At pre-commit time the curr txn owner instant is inflight and has no
completion time;
+ // on table version 8 and above the resolution falls back to the latest
table schema.
+ Option<HoodieInstant> currTxnOwnerInstant = Option.of(
+ metaClient.createNewInstant(HoodieInstant.State.INFLIGHT,
COMMIT_ACTION, "0040"));
+ HoodieSchema result = strategy.resolveConcurrentSchemaEvolution(
+ table, config, lastCompletedTxnOwnerInstant,
currTxnOwnerInstant).get();
+ assertEquals(HoodieSchema.parse(SCHEMA2), result);
+ }
+
+ @Test
+ void testNullTypeWriterSchemaTableVersionSixBoundedByRequestedTime() throws
Exception {
+ setupInstants(SCHEMA1, SCHEMA2, NULL_SCHEMA, true, false,
tableVersionSixProperties());
+ // Table version 6 orders the schema evolution timeline by requested time:
a curr txn owner
+ // instant requested between the two commits adopts the table schema of
the earlier commit.
+ Option<HoodieInstant> currTxnOwnerInstant = Option.of(
+ metaClient.createNewInstant(HoodieInstant.State.INFLIGHT,
COMMIT_ACTION, "0015"));
+ HoodieSchema result = strategy.resolveConcurrentSchemaEvolution(
+ table, config, lastCompletedTxnOwnerInstant,
currTxnOwnerInstant).get();
+ assertEquals(HoodieSchema.parse(SCHEMA1), result);
+ }
+
+ @Test
+ void testNullTypeWriterSchemaTableVersionSixAfterAllCommits() throws
Exception {
+ setupInstants(SCHEMA1, SCHEMA2, NULL_SCHEMA, true, false,
tableVersionSixProperties());
+ Option<HoodieInstant> currTxnOwnerInstant = Option.of(
+ metaClient.createNewInstant(HoodieInstant.State.INFLIGHT,
COMMIT_ACTION, "0040"));
+ HoodieSchema result = strategy.resolveConcurrentSchemaEvolution(
+ table, config, lastCompletedTxnOwnerInstant,
currTxnOwnerInstant).get();
+ assertEquals(HoodieSchema.parse(SCHEMA2), result);
+ }
+
+ @Test
+ void testNullTypeWriterSchemaTableVersionSixBeforeAllCommits() throws
Exception {
+ setupInstants(SCHEMA1, SCHEMA2, NULL_SCHEMA, true, false,
tableVersionSixProperties());
+ // No commit is requested at or before the curr txn owner instant, so the
lookup falls back
+ // to the table create schema.
+ Option<HoodieInstant> currTxnOwnerInstant = Option.of(
+ metaClient.createNewInstant(HoodieInstant.State.INFLIGHT,
COMMIT_ACTION, "0005"));
+ HoodieSchema result = strategy.resolveConcurrentSchemaEvolution(
+ table, config, lastCompletedTxnOwnerInstant,
currTxnOwnerInstant).get();
+ assertEquals(HoodieSchema.parse(SCHEMA1), result);
+ }
+
+ @Test
+ void testNoConflictBackwardsCompatible1TableVersionSix() throws Exception {
+ setupInstants(SCHEMA1, SCHEMA2, SCHEMA1, true, false,
tableVersionSixProperties());
+ HoodieSchema result = strategy.resolveConcurrentSchemaEvolution(
+ table, config, lastCompletedTxnOwnerInstant,
nonTableCompactionInstant).get();
+ assertEquals(HoodieSchema.parse(SCHEMA2), result);
+ }
Review Comment:
Done. Collapsed the three into one `@ParameterizedTest`
(`testNullTypeWriterSchemaTableVersionSix`) over (instantTime, expectedSchema),
and parameterized the whole resolution suite over table version 6 and 8 via
`@ValueSource(ints = {6, 8})`, so
`testNoConflictBackwardsCompatible1TableVersionSix` and the other hand-picked
v6 cases are gone.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/TestSimpleSchemaConflictResolutionStrategy.java:
##########
@@ -156,6 +164,66 @@ void testNullTypeWriterSchema() throws Exception {
assertEquals(HoodieSchema.parse(SCHEMA1), result);
}
+ @Test
+ void testNullTypeWriterSchemaCurrTxnInstantWithoutCompletionTime() throws
Exception {
+ setupInstants(SCHEMA1, SCHEMA2, NULL_SCHEMA, true, false);
+ // At pre-commit time the curr txn owner instant is inflight and has no
completion time;
+ // on table version 8 and above the resolution falls back to the latest
table schema.
+ Option<HoodieInstant> currTxnOwnerInstant = Option.of(
+ metaClient.createNewInstant(HoodieInstant.State.INFLIGHT,
COMMIT_ACTION, "0040"));
+ HoodieSchema result = strategy.resolveConcurrentSchemaEvolution(
+ table, config, lastCompletedTxnOwnerInstant,
currTxnOwnerInstant).get();
+ assertEquals(HoodieSchema.parse(SCHEMA2), result);
+ }
+
+ @Test
+ void testNullTypeWriterSchemaTableVersionSixBoundedByRequestedTime() throws
Exception {
+ setupInstants(SCHEMA1, SCHEMA2, NULL_SCHEMA, true, false,
tableVersionSixProperties());
+ // Table version 6 orders the schema evolution timeline by requested time:
a curr txn owner
+ // instant requested between the two commits adopts the table schema of
the earlier commit.
+ Option<HoodieInstant> currTxnOwnerInstant = Option.of(
+ metaClient.createNewInstant(HoodieInstant.State.INFLIGHT,
COMMIT_ACTION, "0015"));
+ HoodieSchema result = strategy.resolveConcurrentSchemaEvolution(
+ table, config, lastCompletedTxnOwnerInstant,
currTxnOwnerInstant).get();
+ assertEquals(HoodieSchema.parse(SCHEMA1), result);
+ }
+
+ @Test
+ void testNullTypeWriterSchemaTableVersionSixAfterAllCommits() throws
Exception {
+ setupInstants(SCHEMA1, SCHEMA2, NULL_SCHEMA, true, false,
tableVersionSixProperties());
+ Option<HoodieInstant> currTxnOwnerInstant = Option.of(
+ metaClient.createNewInstant(HoodieInstant.State.INFLIGHT,
COMMIT_ACTION, "0040"));
+ HoodieSchema result = strategy.resolveConcurrentSchemaEvolution(
+ table, config, lastCompletedTxnOwnerInstant,
currTxnOwnerInstant).get();
+ assertEquals(HoodieSchema.parse(SCHEMA2), result);
+ }
+
+ @Test
+ void testNullTypeWriterSchemaTableVersionSixBeforeAllCommits() throws
Exception {
+ setupInstants(SCHEMA1, SCHEMA2, NULL_SCHEMA, true, false,
tableVersionSixProperties());
+ // No commit is requested at or before the curr txn owner instant, so the
lookup falls back
+ // to the table create schema.
+ Option<HoodieInstant> currTxnOwnerInstant = Option.of(
+ metaClient.createNewInstant(HoodieInstant.State.INFLIGHT,
COMMIT_ACTION, "0005"));
+ HoodieSchema result = strategy.resolveConcurrentSchemaEvolution(
+ table, config, lastCompletedTxnOwnerInstant,
currTxnOwnerInstant).get();
+ assertEquals(HoodieSchema.parse(SCHEMA1), result);
Review Comment:
Done. The before-all-commits case now uses a create schema (SCHEMA3)
distinct from both commit schemas, so the create-schema fallback is observable
instead of coinciding with commit 0010.
##########
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:
Added a javadoc note on `InstantComparatorV2.getOrderingTime` capturing
this: pre-upgrade completion times are mtime-backfilled and not
guaranteed-durable ordering keys, but safe because the upgrade runs a full
compaction with no concurrent writers, so they no longer affect concurrency or
file-slicing. Accepted that an upgraded table carries mtime ordering for that
pre-boundary stretch; the NPE fix and the v1 change stand on their own.
--
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]
