You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "yihua (via GitHub)" <gi...@apache.org> on 2026/07/28 05:56:45 UTC
[PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
yihua opened a new pull request, #19388:
URL: https://github.com/apache/hudi/pull/19388
### Describe the issue this Pull Request addresses
When schema conflict resolution is enabled and a writer commits a batch whose writer schema is the Avro null schema (for example, an ingestion round that writes no data), `SimpleSchemaConflictResolutionStrategy.resolveConcurrentSchemaEvolution` resolves the table schema at the current transaction's owner instant. At pre-commit time that instant is inflight and has no completion time, so the completion-time filter in `ConcurrentSchemaEvolutionTableSchemaGetter.getLastCommitMetadataWithValidSchemaFromTimeline` calls `String.compareTo(null)`, and the commit fails:
```
Caused by: org.apache.hudi.exception.HoodieException: Unable to get table schema
at org.apache.hudi.client.transaction.SimpleSchemaConflictResolutionStrategy.getTableSchemaAtInstant(SimpleSchemaConflictResolutionStrategy.java:173)
at org.apache.hudi.client.transaction.SimpleSchemaConflictResolutionStrategy.resolveConcurrentSchemaEvolution(SimpleSchemaConflictResolutionStrategy.java:74)
...
Caused by: java.lang.NullPointerException
at java.lang.String.compareTo(String.java:1155)
at org.apache.hudi.common.table.timeline.InstantComparison.lambda$static$3(InstantComparison.java:34)
at org.apache.hudi.common.table.timeline.InstantComparison.compareTimestamps(InstantComparison.java:38)
at org.apache.hudi.client.transaction.ConcurrentSchemaEvolutionTableSchemaGetter.lambda$getLastCommitMetadataWithValidSchemaFromTimeline$3(ConcurrentSchemaEvolutionTableSchemaGetter.java:175)
```
The defect is twofold, and Hudi 1.x writes both table versions:
- On table version 6, ordering the schema evolution timeline by completion time is wrong to begin with: version 6 does not record completion time on disk (the in-memory value is synthesized from the meta file modification time), and 0.x writers concurrently writing the same table order the timeline by requested time. Requested-time ordering restores 0.x parity and also removes the NPE on this version, since every instant carries a requested time.
- On table version 8 and above, completion-time ordering is correct, but the same NPE fires for an empty-batch commit: the current transaction's owner instant is not completed yet and has no completion time.
A transaction with a null writer schema does not evolve the schema, so the resolution should adopt the current table schema.
### Summary and Changelog
- The per-timeline-version instant ordering is exposed as a first-class API on `InstantComparator`: `orderingComparator()` / `getOrderingTime(instant)` (requested-time based in v1, completion-time based in v2), so version handling lives in the timeline layer rather than in callers.
- `ConcurrentSchemaEvolutionTableSchemaGetter`: sorts and bounds the schema evolution timeline with that ordering (completion time for table version 8 and above, requested time for earlier versions, matching 0.x). A target instant without an ordering time (not completed yet, on table version 8 and above) does not bound the lookup instead of throwing NPE.
- `SimpleSchemaConflictResolutionStrategy`: the null-writer-schema path adopts the table schema as of the current transaction's owner instant. On table version 6 the lookup is bounded by the instant's requested time (matching 0.x behavior); on table version 8 and above the inflight instant carries no completion time, so the latest table schema is resolved. The prior-instant lookup for the transaction start snapshot also uses the version-appropriate ordering time.
- Tests: `TestSimpleSchemaConflictResolutionStrategy` gains the inflight-instant NPE regression plus table-version-6 cases proving requested-time bounding; `TestConcurrentSchemaEvolutionTableSchemaGetter` gains latest-schema ordering regressions with the same two-commit layout on both versions (the earlier-requested commit completes last), so table version 8 and above must return the completion-time winner and table version 6 must return the requested-time winner even when the synthesized completion order disagrees; `TestInstantComparators` covers the new `InstantComparator` ordering APIs directly.
### Impact
Fixes commit failures on multi-writer tables for ingestion rounds that write no data, on table version 6 and table version 8 and above alike. No public API or user-facing behavior change otherwise.
### Risk Level
low
The new regression tests fail with the production NPE signature before the fix and pass after; the table-version-6 ordering tests fail without the version-aware ordering and pass with it; both full test classes pass.
### Documentation Update
none
### Contributor's checklist
- [x] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
- [x] Enough context is provided in the sections above
- [x] Adequate tests were added if applicable
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "hudi-agent (via GitHub)" <gi...@apache.org>.
hudi-agent commented on code in PR #19388:
URL: https://github.com/apache/hudi/pull/19388#discussion_r3663265025
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/ConcurrentSchemaEvolutionTableSchemaGetter.java:
##########
@@ -160,9 +170,11 @@ Option<Pair<HoodieInstant, HoodieSchema>> getLastCommitMetadataWithValidSchemaFr
// the timeline finding a completed instant containing a valid schema.
ConcurrentHashMap<HoodieInstant, HoodieSchema> tableSchemaAtInstant = new ConcurrentHashMap<>();
Option<HoodieInstant> instantWithTableSchema = Option.fromJavaOptional(reversedTimelineStream
- // If a completion time is specified, find the first eligible instant in the schema evolution timeline.
- // Should switch to completion time based.
- .filter(s -> instant.isEmpty() || compareTimestamps(s.getCompletionTime(), LESSER_THAN_OR_EQUALS, instant.get().getCompletionTime()))
+ // Find the first eligible instant whose ordering time is no later than the target instant's;
+ // a target instant without an ordering time (not completed yet, on table version 8 and above)
+ // does not bound the lookup.
+ .filter(s -> instant.isEmpty() || StringUtils.isNullOrEmpty(getOrderingTime(instant.get()))
Review Comment:
🤖 nit: `getOrderingTime(instant.get())` is evaluated twice in this lambda — could you extract it to a local variable (`String targetOrderingTime = getOrderingTime(instant.get())`) before the stream chain? It'd make the condition a bit easier to scan.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/SimpleSchemaConflictResolutionStrategy.java:
##########
@@ -157,9 +155,9 @@ public Option<HoodieSchema> resolveConcurrentSchemaEvolution(
}
private Option<HoodieInstant> getInstantInTimelineImmediatelyPriorToTimestamp(
Review Comment:
🤖 nit: the method now internally calls `schemaResolver.computeSchemaEvolutionTimelineInReverseOrder()`, so it does more than its name suggests ("in timeline" no longer captures that the timeline is also computed here). Something like `getInstantImmediatelyPriorToOrderingTimestamp` might align better with the new signature.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19388:
URL: https://github.com/apache/hudi/pull/19388#discussion_r3666972370
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/ConcurrentSchemaEvolutionTableSchemaGetter.java:
##########
@@ -160,9 +170,11 @@ Option<Pair<HoodieInstant, HoodieSchema>> getLastCommitMetadataWithValidSchemaFr
// the timeline finding a completed instant containing a valid schema.
ConcurrentHashMap<HoodieInstant, HoodieSchema> tableSchemaAtInstant = new ConcurrentHashMap<>();
Option<HoodieInstant> instantWithTableSchema = Option.fromJavaOptional(reversedTimelineStream
- // If a completion time is specified, find the first eligible instant in the schema evolution timeline.
- // Should switch to completion time based.
- .filter(s -> instant.isEmpty() || compareTimestamps(s.getCompletionTime(), LESSER_THAN_OR_EQUALS, instant.get().getCompletionTime()))
+ // Find the first eligible instant whose ordering time is no later than the target instant's;
+ // a target instant without an ordering time (not completed yet, on table version 8 and above)
+ // does not bound the lookup.
+ .filter(s -> instant.isEmpty() || StringUtils.isNullOrEmpty(getOrderingTime(instant.get()))
+ || compareTimestamps(getOrderingTime(s), LESSER_THAN_OR_EQUALS, getOrderingTime(instant.get())))
Review Comment:
This makes the shared bounded-lookup primitive silently drop its bound whenever the target has no ordering time. Only one caller actually wants that -- the `isSchemaNull()` branch -- but every future caller inherits it, and in a conflict-*detection* path a fail-open direction masks conflicts rather than surfacing them.
Since `SimpleSchemaConflictResolutionStrategy` is the only production caller, the decision could live there and this primitive could keep a strict contract:
```java
if (writerSchemaOfTxn.isSchemaNull()) {
// A txn writing no data does not evolve the schema; adopt the table schema as of its owner
// instant. On layout v2 an inflight owner has no completion time, so resolve the latest.
HoodieInstant owner = currTxnOwnerInstant.get();
return getTableSchemaAtInstant(schemaResolver,
StringUtils.isNullOrEmpty(schemaResolver.getOrderingTime(owner)) ? Option.empty() : Option.of(owner));
}
```
Behaviour is identical -- `Option.empty()` walks the same unbounded path through `getTableSchemaFromTimelineWithCache` -- but the fail-open becomes a deliberate call-site choice instead of a property of the primitive. Not blocking, your call.
##########
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:
Worth asserting that this fixture actually took effect.
The commits are written `001` then `009`, so the *natural* mtime order already agrees with requested order. The assertions below only discriminate requested-vs-completion ordering *because* of this inversion. If `setLastModifiedTime` ever stops sticking -- mtime granularity, a different storage impl under the harness, someone reordering the `addCommit` calls -- both orderings agree again and the whole test goes green against code that never got fixed.
One assertion after the reload closes it:
```java
// the inversion has to stick, otherwise the assertions below also hold under completion-time ordering
Map<String, String> completionTimes = metaClient.getActiveTimeline().getInstantsAsStream()
.collect(Collectors.toMap(HoodieInstant::requestedTime, HoodieInstant::getCompletionTime));
assertTrue(compareTimestamps(completionTimes.get("001"), GREATER_THAN, completionTimes.get("009")));
```
##########
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:
Flagging the blast radius on this line. On table version 6 the bound is now a requested time and the stream underneath is sorted by requested time, so `lastCompletedInstantAtTxnStart` -- and `lastCompletedInstantAtTxnValidation` just below -- can now select a different instant than before. That's every RFC-82 case on v6, not only the null-writer-schema path this PR set out to fix.
I think the change is right (v1 completion times are mtime-derived, see the other thread). But the added v6 coverage is one hand-picked case, `testNoConflictBackwardsCompatible1TableVersionSix`. Parameterizing `setupInstants` over table version so the existing suite runs on both would actually exercise this -- same suggestion I left on the test file.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v1/InstantComparatorV1.java:
##########
@@ -74,4 +74,14 @@ public Comparator<HoodieInstant> requestedTimeOrderedComparator() {
public Comparator<HoodieInstant> completionTimeOrderedComparator() {
return COMPLETION_TIME_BASED_COMPARATOR;
}
+
+ @Override
+ public Comparator<HoodieInstant> orderingComparator() {
+ return REQUESTED_TIME_BASED_COMPARATOR;
+ }
Review Comment:
Minor, on the framing rather than the code.
The PR description says requested-time ordering here "restores 0.x parity". But concurrent schema evolution conflict detection landed in `e0af47fe97df` ([HUDI-8219] #12781, Mar 2025) and used completion-time ordering from its very first commit -- there's no 0.x behaviour to restore, this code never existed there.
The actual justification is stronger and I'd rather see it in the commit message: v1 completion times aren't recorded on disk at all, they're synthesized per-read from file mtime (`InstantGeneratorV1:79-80`), so they were never a durable ordering key. Worth rewording so whoever does archaeology on this line later gets the real reason.
##########
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:
This one can't fail either way. `setupInstants` passes `setTableCreateSchema(SCHEMA1)` and commit `0010` also carries SCHEMA1, so the assertion holds whether the lookup fell back to the create schema (what the comment claims is being tested) or simply matched `0010`.
Threading a distinct create schema through -- SCHEMA3, say -- would make it actually test the fallback.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "yihua (via GitHub)" <gi...@apache.org>.
yihua commented on code in PR #19388:
URL: https://github.com/apache/hudi/pull/19388#discussion_r3667566745
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/SimpleSchemaConflictResolutionStrategy.java:
##########
@@ -157,9 +155,9 @@ public Option<HoodieSchema> resolveConcurrentSchemaEvolution(
}
private Option<HoodieInstant> getInstantInTimelineImmediatelyPriorToTimestamp(
Review Comment:
This is existing code, so I will keep it as is for simplicity of this PR.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/SimpleSchemaConflictResolutionStrategy.java:
##########
@@ -157,9 +155,9 @@ public Option<HoodieSchema> resolveConcurrentSchemaEvolution(
}
private Option<HoodieInstant> getInstantInTimelineImmediatelyPriorToTimestamp(
Review Comment:
This is existing code, so I will keep it as is for the simplicity of this PR.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "yihua (via GitHub)" <gi...@apache.org>.
yihua commented on PR #19388:
URL: https://github.com/apache/hudi/pull/19388#issuecomment-5109434926
Comments are addressed.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19388:
URL: https://github.com/apache/hudi/pull/19388#issuecomment-5100581482
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "b6d6ba16b7e7ad6abe41924428bde081dce615e9",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15757",
"triggerID" : "b6d6ba16b7e7ad6abe41924428bde081dce615e9",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* b6d6ba16b7e7ad6abe41924428bde081dce615e9 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15757)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "yihua (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19388:
URL: https://github.com/apache/hudi/pull/19388#issuecomment-5100547955
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "b6d6ba16b7e7ad6abe41924428bde081dce615e9",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "b6d6ba16b7e7ad6abe41924428bde081dce615e9",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* b6d6ba16b7e7ad6abe41924428bde081dce615e9 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
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, every instant before the upgrade boundary carries a durable completion time that is really a file mtime, with nothing in the name distinguishing it from a genuine one. `orderingComparator()` on v2 orders that stretch of the timeline by exactly what this PR rejects on v1 -- and a restore or bulk copy *before* the upgrade ran 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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous merged PR #19388:
URL: https://github.com/apache/hudi/pull/19388
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "yihua (via GitHub)" <gi...@apache.org>.
yihua commented on code in PR #19388:
URL: https://github.com/apache/hudi/pull/19388#discussion_r3667562081
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/ConcurrentSchemaEvolutionTableSchemaGetter.java:
##########
@@ -160,9 +170,11 @@ Option<Pair<HoodieInstant, HoodieSchema>> getLastCommitMetadataWithValidSchemaFr
// the timeline finding a completed instant containing a valid schema.
ConcurrentHashMap<HoodieInstant, HoodieSchema> tableSchemaAtInstant = new ConcurrentHashMap<>();
Option<HoodieInstant> instantWithTableSchema = Option.fromJavaOptional(reversedTimelineStream
- // If a completion time is specified, find the first eligible instant in the schema evolution timeline.
- // Should switch to completion time based.
- .filter(s -> instant.isEmpty() || compareTimestamps(s.getCompletionTime(), LESSER_THAN_OR_EQUALS, instant.get().getCompletionTime()))
+ // Find the first eligible instant whose ordering time is no later than the target instant's;
+ // a target instant without an ordering time (not completed yet, on table version 8 and above)
+ // does not bound the lookup.
+ .filter(s -> instant.isEmpty() || StringUtils.isNullOrEmpty(getOrderingTime(instant.get()))
Review Comment:
`getOrderingTime` is cheap so extracting that to a variable is an overkill.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/ConcurrentSchemaEvolutionTableSchemaGetter.java:
##########
@@ -160,9 +170,11 @@ Option<Pair<HoodieInstant, HoodieSchema>> getLastCommitMetadataWithValidSchemaFr
// the timeline finding a completed instant containing a valid schema.
ConcurrentHashMap<HoodieInstant, HoodieSchema> tableSchemaAtInstant = new ConcurrentHashMap<>();
Option<HoodieInstant> instantWithTableSchema = Option.fromJavaOptional(reversedTimelineStream
- // If a completion time is specified, find the first eligible instant in the schema evolution timeline.
- // Should switch to completion time based.
- .filter(s -> instant.isEmpty() || compareTimestamps(s.getCompletionTime(), LESSER_THAN_OR_EQUALS, instant.get().getCompletionTime()))
+ // Find the first eligible instant whose ordering time is no later than the target instant's;
+ // a target instant without an ordering time (not completed yet, on table version 8 and above)
+ // does not bound the lookup.
+ .filter(s -> instant.isEmpty() || StringUtils.isNullOrEmpty(getOrderingTime(instant.get()))
Review Comment:
`getOrderingTime` is cheap, so extracting that to a variable is overkill.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19388:
URL: https://github.com/apache/hudi/pull/19388#issuecomment-5108668577
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "b6d6ba16b7e7ad6abe41924428bde081dce615e9",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15757",
"triggerID" : "b6d6ba16b7e7ad6abe41924428bde081dce615e9",
"triggerType" : "PUSH"
}, {
"hash" : "130f9b68bf678f277bcb0c9c9846bead050a2a90",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "130f9b68bf678f277bcb0c9c9846bead050a2a90",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* b6d6ba16b7e7ad6abe41924428bde081dce615e9 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15757)
* 130f9b68bf678f277bcb0c9c9846bead050a2a90 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "yihua (via GitHub)" <gi...@apache.org>.
yihua commented on code in PR #19388:
URL: https://github.com/apache/hudi/pull/19388#discussion_r3668346413
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v1/InstantComparatorV1.java:
##########
@@ -74,4 +74,14 @@ public Comparator<HoodieInstant> requestedTimeOrderedComparator() {
public Comparator<HoodieInstant> completionTimeOrderedComparator() {
return COMPLETION_TIME_BASED_COMPARATOR;
}
+
+ @Override
+ public Comparator<HoodieInstant> orderingComparator() {
+ return REQUESTED_TIME_BASED_COMPARATOR;
+ }
Review Comment:
Adding a comment would be redundant, as it is known that the request time is required for timeline layout V1.
##########
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:
Updating javadocs would be good.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/ConcurrentSchemaEvolutionTableSchemaGetter.java:
##########
@@ -160,9 +170,11 @@ Option<Pair<HoodieInstant, HoodieSchema>> getLastCommitMetadataWithValidSchemaFr
// the timeline finding a completed instant containing a valid schema.
ConcurrentHashMap<HoodieInstant, HoodieSchema> tableSchemaAtInstant = new ConcurrentHashMap<>();
Option<HoodieInstant> instantWithTableSchema = Option.fromJavaOptional(reversedTimelineStream
- // If a completion time is specified, find the first eligible instant in the schema evolution timeline.
- // Should switch to completion time based.
- .filter(s -> instant.isEmpty() || compareTimestamps(s.getCompletionTime(), LESSER_THAN_OR_EQUALS, instant.get().getCompletionTime()))
+ // Find the first eligible instant whose ordering time is no later than the target instant's;
+ // a target instant without an ordering time (not completed yet, on table version 8 and above)
+ // does not bound the lookup.
+ .filter(s -> instant.isEmpty() || StringUtils.isNullOrEmpty(getOrderingTime(instant.get()))
+ || compareTimestamps(getOrderingTime(s), LESSER_THAN_OR_EQUALS, getOrderingTime(instant.get())))
Review Comment:
The current change is better for generalizing schema fetching.
##########
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:
This is intentional. Good idea to parameterize the tests on both table v6 and v8.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19388:
URL: https://github.com/apache/hudi/pull/19388#issuecomment-5109610402
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "b6d6ba16b7e7ad6abe41924428bde081dce615e9",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15757",
"triggerID" : "b6d6ba16b7e7ad6abe41924428bde081dce615e9",
"triggerType" : "PUSH"
}, {
"hash" : "130f9b68bf678f277bcb0c9c9846bead050a2a90",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15784",
"triggerID" : "130f9b68bf678f277bcb0c9c9846bead050a2a90",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 130f9b68bf678f277bcb0c9c9846bead050a2a90 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15784)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19388:
URL: https://github.com/apache/hudi/pull/19388#issuecomment-5101462317
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "b6d6ba16b7e7ad6abe41924428bde081dce615e9",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15757",
"triggerID" : "b6d6ba16b7e7ad6abe41924428bde081dce615e9",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* b6d6ba16b7e7ad6abe41924428bde081dce615e9 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15757)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "yihua (via GitHub)" <gi...@apache.org>.
yihua commented on code in PR #19388:
URL: https://github.com/apache/hudi/pull/19388#discussion_r3668258842
##########
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:
During the upgrade from table version 6 to version 8, the completion time is backfilled based on the file modification time of the completed commits, and full compaction is done. Note that the completion time only matters when there are concurrent writers and when determining MOR file slices. During the upgrade, there must be no other writers on the same table; given that full compaction is done, the completion time of completed commits are no longer useful. So such completion time backfill leveraging the file modification time is safe. However, in table version 6, the file modification time is not a reliable way of determining the ordering of instants (not materialized into the instant file name and metadata), thus table version 6 should only use requested time for ordering instants, same as Hudi 0.x releases.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "yihua (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): fix NPE in schema conflict resolution on commits with null writer schema [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19388:
URL: https://github.com/apache/hudi/pull/19388#issuecomment-5108714558
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "b6d6ba16b7e7ad6abe41924428bde081dce615e9",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15757",
"triggerID" : "b6d6ba16b7e7ad6abe41924428bde081dce615e9",
"triggerType" : "PUSH"
}, {
"hash" : "130f9b68bf678f277bcb0c9c9846bead050a2a90",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15784",
"triggerID" : "130f9b68bf678f277bcb0c9c9846bead050a2a90",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* b6d6ba16b7e7ad6abe41924428bde081dce615e9 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15757)
* 130f9b68bf678f277bcb0c9c9846bead050a2a90 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15784)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org