refactor(schema): dissolve AvroSchemaUtils, dedupe compat - #19810
refactor(schema): dissolve AvroSchemaUtils, dedupe compat#19810voonhous wants to merge 6 commits into
Conversation
voonhous
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19810 +/- ##
============================================
+ Coverage 78.32% 78.33% +0.01%
+ Complexity 33897 33896 -1
============================================
Files 2540 2539 -1
Lines 141763 141748 -15
Branches 17197 17191 -6
============================================
+ Hits 111029 111038 +9
+ Misses 23031 23013 -18
+ Partials 7703 7697 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
a66245a to
f8d7374
Compare
refactor(schema): schema util contracts, dead API and folds Part 1 of #16639. The HoodieSchema migration moved schema helpers out of AvroSchemaUtils and HoodieAvroUtils but never wrote down what belongs where, so helpers kept being re-implemented instead of found. Behavior-preserving. The routing rule, now in the class javadocs: HoodieSchema answers questions about one schema and holds the factories; HoodieSchemaUtils holds table-schema transforms and the Hudi record shapes; HoodieSchemaCompatibility is the only compatibility and projection entry point; HoodieAvroUtils holds Avro record and value operations; common.schema.internal is the InternalSchema domain. Look-alike helpers with different semantics cross-reference each other instead of being merged. Code changes to match it: - HoodieSchemas folds into HoodieSchemaUtils (createDeleteLogSchema). - AvroSchemaCache deleted: its one caller interned a generated-class static, a no-op since #18967 removed the last consumer. - LocalHoodieSchemaCache moves from common.util to common.schema; getInstance() becomes create(), which is what it always did. - convertBytesToBigDecimal(byte[], HoodieSchema) moves to HoodieAvroUtils; the (byte[], int, int) shim and the caller-less HoodieSchemaUtils.convertValueForSpecificDataTypes and hasSmallPrecisionDecimalField are deleted. - HoodieAvroUtils.getRecordColumnValues takes HoodieSchema, since both callers unwrapped one to call it; it still interns through HoodieAvroSchemaCache and now reuses the static PROPERTIES and a fixed array instead of allocating per call. - 13 caller-less members narrowed; test-only ones get @VisibleForTesting. - Stale "equivalent to X" javadocs fixed, cross-references added. Tests pin delete-log field order and logical types, case-insensitive projection, nested column values, the HoodieMetadataPayload reference-equality fast path, the getRecordColumnValues intern, and LocalHoodieSchemaCache (test restored from #17740). Pre-existing bugs found on the way are filed, not fixed here: #19823, #19825, #19826. Part 2 (#19810) implements asNullable natively, dissolves AvroSchemaUtils and dedupes the compatibility cluster. Closes #15908
HoodieSchemaUtils#asNullable unwrapped its argument to Avro, called AvroSchemaUtils#asNullable, which immediately re-wrapped it as a HoodieSchema to run the InternalSchema column-nullability update, then flattened the result back to Avro for the caller to wrap once more. Run the same pipeline on the HoodieSchema directly: collect the required top-level fields, convert to InternalSchema, apply a ColumnUpdateChange that marks them nullable, convert back under the original full name. Two details preserved on purpose: Avro's Schema#isNullable is true for a bare NULL type while HoodieSchema#isNullable is not, so the filter excludes NULL-typed fields explicitly; and when every field is already nullable the input instance is returned (the old path returned an equal fresh wrapper; the sole caller, Flink ClusteringOperator, does not compare identities). Tests pin the actual behavior of the InternalSchema round trip, which is unchanged by this commit: per-field docs, names, namespace and field order survive; record-level doc and custom props do not; a NULL-typed field next to a required field is rejected by the converter. AvroSchemaUtils#asNullable is now unused and is removed with its class in the next commit. Part of apache#16639.
- HoodieSchemaCompatibility#lookupWriterField re-implemented the checker's lookupWriterField (direct name, then reader aliases, throw on more than one match). Keep the facade's RECORD precondition and delegate to the checker. - HoodieSchemaCompatibilityChecker restated the primitive widening table (LONG<-INT, FLOAT<-INT/LONG, DOUBLE<-INT/LONG/FLOAT, BYTES<-STRING, STRING<-BYTES/numeric) that HoodieSchemaTypePromotion already encodes; the five cases now call canPromote. TIMESTAMP<-LONG and UUID<-STRING stay as explicit checker-only cases with a comment on why they must not become projection promotions (apache#19384 gates the deduction that produces that pair). checkDecimalWidening untouched. - The 3-arg isSchemaCompatible named its parameters readerSchema and writerSchema but forwards them as prevSchema and newSchema, where the reader is the second argument. Rename the parameters and rewrite the javadoc; no reordering, callers already pass (prev, new). areSchemasCompatible gets a javadoc stating that its reader is the first argument, names are not checked and no missing-field check runs. Tests pin the promotion table from both sides (logical-type pairs accepted by the checker and rejected by canPromote), lookupWriterField (direct, alias, ambiguous, absent, non-record) and the argument order of areSchemasCompatible. Part of apache#16639.
AvroSchemaUtils had no callers outside hudi-common, a HoodieSchema twin for every method, and survived only as a delegate target. Its three remaining production uses were all inside HoodieAvroUtils, so the helpers move there with their bodies unchanged: getNonNullTypeFromUnion stays public (HoodieSchema.Blob and a hudi-hadoop-common test use the strict unwrap; its javadoc states the two-branch contract and points at the lenient siblings), isNullable becomes private and createNewSchemaFromFieldsWithReference package-private. HoodieSchema.Blob no longer reaches into a util class from its static initializer: a local nullable(Schema) builds the [null, X] unions (inputs are LONG, BYTES and a RECORD, so the old NULL-type guard was never exercised) and REFERENCE_FIELD_COUNT is derived from a REFERENCE_SCHEMA constant instead of unwrapping the union around it. HoodieSchemaUtils#createNullableSchema, a caller-less shim over the deleted method, is removed; HoodieSchema#createNullable is the idempotent native equivalent. TestAvroSchemaUtils folds into TestHoodieAvroUtils. New tests pin the strict unwrap (non-union as-is, both branch orders, three-branch and null-less unions rejected) and the Blob shape (3 fields, 4 reference fields, null first in every nullable union). Closes the AvroSchemaUtils half of apache#16639.
f8d7374 to
cd15b3f
Compare
github-actions
Bot
added
size:L
voonhous
left a comment
voonhous
left a comment
There was a problem hiding this comment.
Self-review of the three commits. Old and new asNullable were run side by side on 20 schemas (nested records, non-null defaults, null-last unions, logical types, enum/fixed, VARIANT/VECTOR/BLOB, non-record) with identical output, and the persisted Blob schema JSON is byte-identical before and after. One pre-existing bug surfaced on the way and is filed as #19833 (BLOB reference field ids collide with table field ids); it is deliberately not fixed here so the PR stays a pure refactor.
nit: feel free to ignore. The InternalSchema route inside asNullable was reuse rather than design (HUDI-8841 picked ColumnUpdateChange because it existed); a field-wise HoodieSchema.createNullable wrap would be lossless but is a behaviour change. Could the "Left for later" list in the description gain that line?
| return schema; | ||
| } | ||
|
|
||
| InternalSchema internalSchema = InternalSchemaConverter.convert(schema); |
There was a problem hiding this comment.
major: pre-existing, not introduced by this rewrite: the deleted AvroSchemaUtils#asNullable path gives the identical output. InternalSchemaConverter.buildBlobInternalRecordType() gives the blob's nested reference fields ids 0..3 and InternalSchema.buildIdToField is one flat last-put-wins map, so on {id: int, b: nullable BLOB} this call returns a record whose first field is external_path instead of id; a required BLOB throws Cannot update nullability for column 'b'. Reachable from Flink ClusteringOperator.open() on any BLOB table. Could we keep the fix out of this PR so the conversion stays as on master and the PR remains a pure refactor?
There was a problem hiding this comment.
Filed as #19833 with the repro, will leave this comment unresolved, but will keep it as a separate fix.
asNullable failed on a non-record input with whatever HoodieSchema#getFields throws (an IllegalStateException; the deleted Avro path threw AvroRuntimeException). Make the RECORD-only contract explicit with a checkArgument and say so in the javadoc, which also now names the three pre-existing losses of the InternalSchema round trip: non-null defaults, ENUM, and null-last union order. HoodieSchemaTypePromotion's closing javadoc line still said the class is used only by HoodieSchemaProjectionChecker, which the header edited in the previous commit contradicts. Drop it.
- TestHoodieSchema: the Blob schema is persisted by every BLOB table, so testCreateBlob pins its serialized form literally instead of checking counts and nullability field by field. - TestHoodieSchemaCompatibility: assert the five reverse numeric narrowings the checker must reject; the hudi-spark guard for the reversed-argument bug class (HUDI-1493) has never executed. Reuse HoodieSchemaTestUtils.createRecord instead of a local twin; note the end-to-end alias coverage. - TestHoodieSchemaTypePromotion: writer-side canPromote(LONG, DATE) and (LONG, TIME) are rejected. - TestHoodieSchemaUtils: a non-null default, an ENUM and a null-last union come back changed from the asNullable round trip, a VECTOR column survives, and a non-record input is rejected.
|
@wombatu-kun The second part of the Avro Utils refactoring is ready for review! Can you please help to review it? Thank you! |
assertCompatible/assertIncompatible take (reader, writer), the reverse of the (prev, new) order isSchemaCompatible expects, so the argument order is easy to misread at the call sites. Spell out the direction being asserted, the reader-first convention, and the single-field record wrapping.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR dissolves AvroSchemaUtils (moving its three remaining helpers into HoodieAvroUtils), implements HoodieSchemaUtils#asNullable natively over HoodieSchema/InternalSchema instead of round-tripping through Avro, and dedupes the compatibility cluster (lookupWriterField delegation, the primitive-widening cases routed through HoodieSchemaTypePromotion#canPromote, and a parameter rename on the 3-arg isSchemaCompatible). I traced each dedup to confirm the behavior-preserving claim: the canPromote collapse only runs in the different-types branch (so its equal-type short-circuit is unreachable and the widening table matches exactly), the two lookupWriterField bodies are behaviorally equivalent, the Blob static-init ordering is sound, and the asNullable NULL-type handling and RECORD precondition are safe for the sole caller. 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. One minor naming nit on a private helper; the rest of the refactor is clean.
cc @yihua
| * Wraps the given schema into the canonical Avro nullable union {@code [null, schema]}. None of the blob | ||
| * field types is NULL, so no further validation is needed here. | ||
| */ | ||
| private static Schema nullable(Schema schema) { |
There was a problem hiding this comment.
🤖 nit: nullable reads as an adjective (a property check) rather than a factory — could you rename it to wrapNullable or createNullableUnion so call sites like nullable(bytesField) are self-explanatory?
4 participants
Describe the issue this Pull Request addresses
Closes #16639 (part 2; part 1 is #19809, merged as 93f1f71). Rebased onto master; the three commits are all new. Part 1 wrote down where schema helpers belong; this removes the class that has no place in that rule and the duplicated logic in the compatibility cluster.
Summary and Changelog
Behavior-preserving. One concern per commit; each compiles and reverts alone.
AvroSchemaUtilsis gone: its three remaining uses were all insideHoodieAvroUtils, so the helpers move there unchanged;HoodieSchema.Blobbuilds its fields without it; the caller-lessHoodieSchemaUtils#createNullableSchemashim goes.HoodieSchemaUtils#asNullableruns the InternalSchema nullability update on theHoodieSchemadirectly instead of bouncing through Avro twice.lookupWriterFieldwas implemented twice (facade now delegates); the checker's primitive-widening cases callHoodieSchemaTypePromotion#canPromote(same table,TIMESTAMP <- LONG/UUID <- STRINGstay checker-only and explained); the 3-argisSchemaCompatiblenamed its parameters reader/writer while routing them prev/new, so they are renamed, not reordered.Details per commit
implement asNullable natively: old path was HoodieSchema -> Avro -> HoodieSchema -> InternalSchema -> HoodieSchema -> Avro -> HoodieSchema. Two details kept: Avro'sSchema#isNullableis true for a bare NULL type whileHoodieSchema#isNullableis not, so NULL-typed fields are excluded explicitly; the all-nullable case returns the input instance (was an equal fresh wrapper; the sole caller, FlinkClusteringOperator, does not compare identities). New tests pin the round trip's actual, unchanged behavior: per-field docs survive, record-level doc/props do not, a NULL-typed field beside a required one is rejected.dedupe compatibility helpers:lookupWriterFieldkeeps the facade's stricter RECORD precondition then delegates. Checker cases LONG/FLOAT/DOUBLE/BYTES/STRING collapse into onecanPromotecall.TIMESTAMP <- LONGandUUID <- STRINGmust not become projection promotions or writer-schema deduction would silently drop the logical type; fix(schema): require a per-field override to promote a bare long to a timestamp logical type #19384 gates the deduction that produces that pair. Decimal widening untouched (the projection checker requires fixed-size parity, the compatibility checker does not; documented, not resolved).areSchemasCompatibledocumented: reader is the first argument, no name check, no missing-field check. Tests pin the promotion table from both sides,lookupWriterField(direct, alias, ambiguous, absent, non-record) and the argument order ofareSchemasCompatible.dissolve AvroSchemaUtils:getNonNullTypeFromUnionpublic,isNullableprivate,createNewSchemaFromFieldsWithReferencepackage-private, all verbatim.HoodieSchema.Blobuses a localnullable(Schema)(inputs are LONG, BYTES and a RECORD, so the old NULL-type guard was never exercised) and derivesREFERENCE_FIELD_COUNTfrom aREFERENCE_SCHEMAconstant instead of unwrapping the union around it.TestAvroSchemaUtilsfolds intoTestHoodieAvroUtils; new tests pin the strict unwrap and the Blob shape (3 fields, 4 reference fields, null first in every nullable union).asNullablerejects non-RECORD input withIllegalArgumentException(was an AvroNot a recorderror from the deleted path, unreachable either way); tests pin the persisted Blob schema JSON literally, the five reverse numeric narrowings, the writer-sidecanPromote(LONG, DATE/TIME), and the three pre-existing losses of theasNullableround trip (non-null default, ENUM, null-last union) plus a VECTOR column surviving it; staleHoodieSchemaTypePromotionjavadoc line dropped; test helper reuse.Left for later (not in this series)
HoodieSchemaUtils#projectSchemastill round-trips throughHoodieAvroUtils#projectSchema; a native version needs a nested-projection benchmark first.HoodieSchemaUtils#asNullablestill rewrites through the InternalSchema (HUDI-8841 reusedColumnUpdateChangebecause it existed); a field-wiseHoodieSchema.createNullablewrap would keep non-null defaults, ENUM and union order, but that is a behavior change. The BLOB field-id collision the round trip exposes is [BUG] InternalSchema BLOB reference field ids collide with table field ids #19833.HoodieAvroUtils#recordNeedsRewriteForExtendedAvroTypePromotionto HoodieSchema needs a logical-type accessor onHoodieSchema.HoodieRealtimeRecordReaderUtils#generateProjectionSchemaduplicatesHoodieSchemaUtils#generateProjectionSchema(same algorithm and exception text).HoodieSchemaRepair#hasTimestampMillisFieldvsHoodieTableMetadataUtil#isTimestampMillisField.AvroSchemaEvolutionUtilshas no Avro in any signature; rename after the RFC-104 docs settle.VariantSchemaUtilsis HoodieSchema-typed but lives incommon.avro;HoodieProjectionMaskis a hudi-hadoop-mr concern incommon.schema.HoodieSchemaUtils#createNewSchemaFieldis an alias ofHoodieSchemaField#of(13 files, several in active variant work).HoodieMetadataPayload: derivingHOODIE_METADATA_AVRO_SCHEMAfromHOODIE_METADATA_SCHEMA.toAvroSchema()would make the feat(schema): Phase 24 - Restore O(1) reference equality comparison i… #17672 fast path independent of interning order.Impact
Internal API only, no behavior change. Removed: class
AvroSchemaUtils,HoodieSchemaUtils#createNullableSchema(no callers). Moved:getNonNullTypeFromUnion->HoodieAvroUtils(public; the other two become private/package-private there). Parameter names, not order, on the 3-argHoodieSchemaCompatibility#isSchemaCompatible.Risk Level
Low-medium. The checker change is on write-path validation; the collapsed cases match
canPromotepredicate by predicate and are pinned by tests in both directions.asNullablehas one production caller and is pinned by tests.Documentation Update
none
Contributor's checklist