fix(client): fix NPE in schema conflict resolution on commits with null writer schema - #19388
Conversation
…ll writer schema When schema conflict resolution is enabled and a writer commits a batch whose writer schema is the Avro null schema (an ingestion round that writes no data), the resolution strategy resolves the table schema at the current transaction's owner instant. At pre-commit time that instant is inflight with no completion time, so the completion-time filter in the schema getter calls String.compareTo(null) and the commit fails with an NPE. - Expose the per-timeline-version instant ordering as a first-class API on InstantComparator: orderingComparator() / getOrderingTime(instant), requested-time based in v1 and completion-time based in v2. - 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 no longer bounds the lookup instead of throwing. - SimpleSchemaConflictResolutionStrategy adopts the table schema as of the owner instant on the null-writer-schema path, using the version-appropriate ordering time. - Add regression coverage in TestSimpleSchemaConflictResolutionStrategy, TestConcurrentSchemaEvolutionTableSchemaGetter, and TestInstantComparators.
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR fixes an NPE in schema conflict resolution when a writer commits a null (empty) writer schema, by introducing a timeline-version-aware "ordering time" (completion time for layout v2, requested time for v1) and using it in place of hardcoded completion time. I traced the write/pre-commit path across both table versions, the caller in TransactionUtils, and the InstantComparator interface change: the inflight-target NPE is properly guarded via the isNullOrEmpty(getOrderingTime(...)) short-circuit, the v1/v2 filter-and-comparator pairs stay consistent, the interface addition has no external implementors, and the blast radius is contained to the conflict-resolution path. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. A couple of small naming and clarity suggestions below.
cc @yihua
…ion 6 and 8; docs(common): ordering-key invariant and upgrade-boundary completion time
|
Comments are addressed. |
voonhous
left a comment
There was a problem hiding this comment.
All my comments addressed.
LGTM
…ll writer schema (#19388) * fix(client): fix NPE in schema conflict resolution on commits with null writer schema When schema conflict resolution is enabled and a writer commits a batch whose writer schema is the Avro null schema (an ingestion round that writes no data), the resolution strategy resolves the table schema at the current transaction's owner instant. At pre-commit time that instant is inflight with no completion time, so the completion-time filter in the schema getter calls String.compareTo(null) and the commit fails with an NPE. - Expose the per-timeline-version instant ordering as a first-class API on InstantComparator: orderingComparator() / getOrderingTime(instant), requested-time based in v1 and completion-time based in v2. - 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 no longer bounds the lookup instead of throwing. - SimpleSchemaConflictResolutionStrategy adopts the table schema as of the owner instant on the null-writer-schema path, using the version-appropriate ordering time. - Add regression coverage in TestSimpleSchemaConflictResolutionStrategy, TestConcurrentSchemaEvolutionTableSchemaGetter, and TestInstantComparators. * test(client): parameterize schema conflict resolution over table version 6 and 8; docs(common): ordering-key invariant and upgrade-boundary completion time (cherry picked from commit b475651)
…ll writer schema (#19388) * fix(client): fix NPE in schema conflict resolution on commits with null writer schema When schema conflict resolution is enabled and a writer commits a batch whose writer schema is the Avro null schema (an ingestion round that writes no data), the resolution strategy resolves the table schema at the current transaction's owner instant. At pre-commit time that instant is inflight with no completion time, so the completion-time filter in the schema getter calls String.compareTo(null) and the commit fails with an NPE. - Expose the per-timeline-version instant ordering as a first-class API on InstantComparator: orderingComparator() / getOrderingTime(instant), requested-time based in v1 and completion-time based in v2. - 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 no longer bounds the lookup instead of throwing. - SimpleSchemaConflictResolutionStrategy adopts the table schema as of the owner instant on the null-writer-schema path, using the version-appropriate ordering time. - Add regression coverage in TestSimpleSchemaConflictResolutionStrategy, TestConcurrentSchemaEvolutionTableSchemaGetter, and TestInstantComparators. * test(client): parameterize schema conflict resolution over table version 6 and 8; docs(common): ordering-key invariant and upgrade-boundary completion time (cherry picked from commit b475651)
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.resolveConcurrentSchemaEvolutionresolves 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 inConcurrentSchemaEvolutionTableSchemaGetter.getLastCommitMetadataWithValidSchemaFromTimelinecallsString.compareTo(null), and the commit fails:The defect is twofold, and Hudi 1.x writes both table versions:
A transaction with a null writer schema does not evolve the schema, so the resolution should adopt the current table schema.
Summary and Changelog
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.TestSimpleSchemaConflictResolutionStrategygains the inflight-instant NPE regression plus table-version-6 cases proving requested-time bounding;TestConcurrentSchemaEvolutionTableSchemaGettergains 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;TestInstantComparatorscovers the newInstantComparatorordering 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