fix(avro): detect the two-field shredded variant shape and harden the variant merge path - #19620
fix(avro): detect the two-field shredded variant shape and harden the variant merge path#19620voonhous wants to merge 4 commits into
Conversation
…ility note
Two things the self-review turned up, both in code the round-3 commits
touched but no test or comment covered.
stripVariantShredding used to re-add untouched fields by reference and
then hand them to Schema.setFields, which throws "Field already used" for
any record of {other column + shredded variant}. That is the same defect
apache#18938 fixed in the sibling HoodieVariantReconstruction.create; the twin
here was missed. The recursive rewrite copies every field via withSchema
and so fixes it incidentally - now pinned by a test at the production
call site, HoodieAvroWriteSupport.generateEffectiveSchema with shredding
disabled, which is the clustering/compaction path that reaches it.
The nested comments claimed the row writer's depth recursion as the
producer of nested shredded files. That overstates reachability: the
forced-shredding hook is top-level only in BOTH write supports, so no DDL
or table property reaches depth and only a hand-authored write schema
can. Say that plainly instead, and note why the nested coverage is unit
level rather than end to end.
The shredded and unshredded small-file merge tests were near-identical copies, and both were pinned to AVRO. Fold them into one sweep over (record type, layout) and pull the two repeated pins into helpers. What each leg is for: - AVRO + shredded is the apache#19567 bug; it goes red without the HoodieMergeHelper alignment (verified by reverting that hunk: rows 1-2 come back [1,null,1000], [2,null,1000]). - AVRO + unshredded is the no-op guard, kept because a reviewer asked for the twin, now without the 65-line copy. - SPARK is new coverage. HoodieSparkParquetReader.getSchema returns a nullable UNION rather than a RECORD, so alignShreddedVariants bails at its RECORD/RECORD guard and that reader is untouched by the fix. The leg passes identically with and without the fix; it is swept so that the guard which scopes the fix to the AVRO reader is pinned rather than merely assumed. assertVariantLayout and assertSingleFileGroup also replace the inline pins in the evolving-schema test; the layout pin was copy-pasted 11 times across this file. Net -56 lines. Verified on spark4.1/scala-2.13: 15 succeeded, 0 failed, 2 canceled (the Spark 3.x-only tests).
…ilding schemas that do not change Two things found self-reviewing apache#19582 after it merged. isShreddedVariantShape demanded exactly three fields with a bytes `value`, but HoodieSchema.Variant.determineIfShredded - the answer used whenever the logical type survives - calls anything carrying typed_value shredded. The shredding spec lets a writer omit `value` when every row is typed, and because the parquet footer always strips the logical type, the shape check is the only detector that runs on real files. So a two-field group read at the unshredded schema and lost its payload silently: apache#19567 again by another shape. Accept {metadata, typed_value} with `value` optional; the requested-side variant anchor is what keeps plain user structs out, so this costs no false positives, and a struct with a fourth field is still rejected. Second, both recursion loops copied every field into a new list and then threw the list away when nothing matched. That is the path every non-variant table takes, on every runMerge and every avro parquet read, and it costs an Avro Field plus a defaultVal() lookup per field per level. Build the list lazily, backfilling only once a replacement actually appears. Measured on a 500-column nested schema: alignShreddedVariants 730us -> 335us per runMerge, create() 834us -> 365us per read. Tests: the two-field shape engages and rebuilds, and a four-field struct that merely carries typed_value stays untouched.
|
@wombatu-kun Can you please help to review this, just some cleaning up and hardening ontop of #19582. |
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the follow-up! This PR relaxes isShreddedVariantShape to accept the two-field {metadata, typed_value} shredded form (matching determineIfShredded), converts the record-rebuild loops in VariantSchemaUtils to lazy field copying, and adds reconstruction/merge test coverage. I traced the relaxed shape detection through isShreddedVariantTarget → buildRebuilder → toShreddedReadSchema and into the real Spark4VariantShreddingProvider (where the no-value case yields variantIdx=-1, which Spark's ShreddingUtils.rebuild guards), and verified the lazy newFields/copyFieldsBefore rewrite is behavior-preserving (identity preserved on no-change, correct prefix copy on first change). 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 displaced Javadoc comment to fix; the logic and naming changes are clean.
cc @yihua
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19620 +/- ##
============================================
- Coverage 77.54% 76.94% -0.60%
+ Complexity 32928 32660 -268
============================================
Files 2524 2524
Lines 139513 139539 +26
Branches 16781 16818 +37
============================================
- Hits 108183 107375 -808
- Misses 23744 24507 +763
- Partials 7586 7657 +71
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…er the value-less group
- The "forced shredding is top-level only in both write supports" note was wrong for the
row path: processNestedDataType recurses into structs and generateShreddedSchema re-reads
the DDL on every entry, so struct<v variant> plus the force-shredding property does shred
at depth. Scoped the claim to HoodieAvroWriteSupport.applyForcedShreddingSchema in all
three places that carried it.
- Added a value-less round-trip case in TestHoodieVariantReconstructionRoundTrip. The
detection test in hudi-hadoop-common uses a stub provider that ignores the shredded
schema, so nothing exercised Spark4VariantShreddingProvider.buildVariantSchema with the
variantIdx = -1 that a {metadata, typed_value} group produces.
- Moved the stranded listDataParquetFiles javadoc back onto its method.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the follow-up! This PR relaxes isShreddedVariantShape to accept the two-field {metadata, typed_value} shredded variant shape (value optional), converts the field-rebuild loops in stripRecordVariantShredding/swapShreddedVariantFields to lazy copies, and adds coverage for the value-less group and the disable-shredding strip path.
I traced the lazy field-building (output-identical to the prior eager version — same fresh-withSchema copies, correct size invariant, no off-by-one at i=0), the relaxed shape check across every field-count/value-presence case (the requested-side VARIANT anchor in isShreddedVariantTarget keeps false positives out), and the value-less rebuild end-to-end through buildVariantSchema → ShreddingUtils.rebuild (variantIdx=-1 correctly skips the residual read, and detection stays consistent between toShreddedReadSchema and buildRebuilder). No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
| * {@code Schema.Field} still bound to the source record throws, so any table with a shredded | ||
| * variant AND at least one other column failed here. #18938 fixed exactly that defect in the | ||
| * sibling HoodieVariantReconstruction and left this twin behind. Nested variants must be | ||
| * stripped too, since the row writer shreds at any depth. |
There was a problem hiding this comment.
generateEffectiveSchema is the AVRO write path, where applyForcedShreddingSchema walks top-level fields only, so the row writer is not what puts a nested shredded variant in front of it. Worth the same scoping the VariantSchemaUtils and HoodieVariantReconstruction comments now carry.
| } | ||
|
|
||
| @Test | ||
| void ignoresFourFieldStructThatMerelyCarriesTypedValue(@TempDir Path tmp) { |
There was a problem hiding this comment.
This four-field struct is killed by the field-count guard before the new no-value arm of isShreddedVariantShape, which nothing else reaches either - mutating its fieldCount == 2 to true leaves every test green. Worth a sibling case with {metadata, typed_value, extra}.
| * exercise the shredded path can silently degenerate into the unshredded one, or the reverse, | ||
| * and the branch it was written for goes uncovered. | ||
| */ | ||
| private def assertVariantLayout(tablePath: String, shredded: Boolean, leg: String): Unit = { |
There was a problem hiding this comment.
The CDC test's layout block is this helper's body verbatim, with tablePath, shredded and leg all already in scope there. Worth folding that third site in too, since it is a drop-in call.
| } | ||
|
|
||
| @Test | ||
| void createThenReconstructRebuildsAValueLessShreddedGroup(@TempDir Path tmp) throws Exception { |
There was a problem hiding this comment.
No mvn test -pl list in bot.yml or the Azure pipeline names hudi-spark-datasource/hudi-spark4-common, so this class is compiled but never executed - the variantIdx = -1 path it pins cannot go red in CI. Worth adding the module to the spark4.2 java-test lanes, or saying in the class javadoc that it is a local-only guard.
5 participants
Describe the issue this Pull Request addresses
Follow-up to #19582 (fixes #19567), which merged as e1dd86e. Reviewing that PR again after it merged turned up one live detection gap, one latent crash it had fixed silently, a hot-path allocation, and two coverage holes. Nothing here changes the shape of the fix that merged; it closes the edges around it.
The detection gap is the substantive one.
isShreddedVariantShapedemanded a record of exactly three fields with a bytesvalue, butHoodieSchema.Variant.determineIfShredded- the answer used whenever the variant logical type survives - calls anything carryingtyped_valueshredded, whatever else is present. The two disagree, and it is the shape check that matters in practice: the parquet footer always strips the logical type, so on real files the shape check is the only detector that runs. The shredding spec lets a writer omitvaluewhen every row is typed, so a{metadata, typed_value}group read at the unshredded schema and lost its payload with no error - #19567 again, by another shape. Hudi's own writer always emits three fields, so the exposure is files written by another engine.Summary and Changelog
isShreddedVariantShapenow accepts{metadata: bytes, typed_value}withvalueoptional, matchingdetermineIfShredded. A struct carrying a fourth field is still rejected, and the requested-side variant anchor still does the false-positive work, so relaxing the count costs nothing: a plain user struct of the same shape is only a target when the requested column is a variant.stripVariantShreddingis now pinned by a test at the call site that can actually crash. Before fix(avro): detect shredded variant base files by shape so reconstruct… #19582 it re-added untouched fields by reference and handed them toSchema.setFields, which throwsField already usedfor any record of{other column + shredded variant}- the same defect feat(variant): support reading shredded variant base files via the AVRO reader #18938 fixed in the siblingHoodieVariantReconstruction.create, left behind in the twin. fix(avro): detect shredded variant base files by shape so reconstruct… #19582's recursive rewrite fixed it incidentally and silently; the guard lives onHoodieAvroWriteSupport.generateEffectiveSchemawith shredding disabled, which is the clustering/compaction path that reaches it.runMergeand every avro parquet read, at the cost of an AvroFieldplus adefaultVal()lookup per field per level. The list is now built lazily, backfilling only once a replacement appears.assertVariantLayout/assertSingleFileGroup; the layout pin alone had been copy-pasted 11 times in that file.Impact
Restores the shredded-variant payload for base files whose variant group omits the optional
valuecolumn, which previously read back null through the AVRO path. No behaviour change for files Hudi wrote itself, which always carry all three fields.Worth recording for anyone tracing this code, since it is not obvious and is not stated anywhere today:
alignShreddedVariantscan never fire forHoodieSparkParquetReader, because itsgetSchema()returns a nullable union rather than a record and the alignment bails at its RECORD/RECORD guard. The SPARK record type is therefore untouched by #19582 - verified by running the merge test on both record types with and without that fix: AVRO fails without it ([1,null,1000], [2,null,1000]) and passes with it, while SPARK passes identically either way. That scoping is a consequence of the guard rather than a deliberate decision, so the new SPARK leg of the sweep pins it instead of leaving it assumed.Measured on a 500-column nested non-variant schema:
alignShreddedVariants730us -> 335us perrunMerge,HoodieVariantReconstruction.create834us -> 365us per parquet read. Small against file I/O, but it was pure waste on the common path.Risk Level
low. The detection change only widens, and only for a shape that previously read back corrupt; the anchor that prevents false positives is untouched. The lazy build is a refactor of two loops whose output is asserted identical, including the same-instance contract the merge path depends on (
assertSameinTestHoodieSchemaCompatibility). Verified on spark4.1/scala-2.13 end to end, plus hudi-common 371/371,TestHoodieVariantReconstruction12/12 and the hudi-hadoop-common parquet suites.Documentation Update
none
Contributor's checklist