voonhous commented on code in PR #19388:
URL: https://github.com/apache/hudi/pull/19388#discussion_r3666089944
##########
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:
These three `testNullTypeWriterSchemaTableVersionSix*` tests are the same
body with a different inflight timestamp and expected schema -- worth
collapsing into one `@ParameterizedTest`:
```java
@ParameterizedTest
@MethodSource("tableVersionSixNullSchemaCases") // (instantTime,
expectedSchema)
void testNullTypeWriterSchemaTableVersionSix(String instantTime, String
expectedSchema) throws Exception {
setupInstants(SCHEMA1, SCHEMA2, NULL_SCHEMA, true, false,
tableVersionSixProperties());
...
}
```
And `testNoConflictBackwardsCompatible1TableVersionSix` is
`testNoConflictBackwardsCompatible1` with a different table version. Since the
v1 ordering switch changes how both `lastCompletedInstantAtTxnValidation` and
`lastCompletedInstantAtTxnStart` are picked, it affects RFC-82 cases 2-8, not
just the null-schema path. Parameterizing `setupInstants` over table version
and running the existing suite on both would cover all of those instead of this
one hand-picked case -- and these duplicates go away.
--
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]
