voonhous opened a new pull request, #19620:
URL: https://github.com/apache/hudi/pull/19620
### Describe the issue this Pull Request addresses
Follow-up to #19582 (fixes #19567), which merged as e1dd86e9. 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. `isShreddedVariantShape` demanded
a record of exactly three fields with a bytes `value`, but
`HoodieSchema.Variant.determineIfShredded` - the answer used whenever the
variant logical type survives - calls anything carrying `typed_value` shredded,
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 omit `value` when 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
- `isShreddedVariantShape` now accepts `{metadata: bytes, typed_value}` with
`value` optional, matching `determineIfShredded`. 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.
- `stripVariantShredding` is now pinned by a test at the call site that can
actually crash. Before #19582 it re-added untouched fields by reference and
handed them to `Schema.setFields`, which throws `Field already used` for any
record of `{other column + shredded variant}` - the same defect #18938 fixed in
the sibling `HoodieVariantReconstruction.create`, left behind in the twin.
#19582's recursive rewrite fixed it incidentally and silently; the guard lives
on `HoodieAvroWriteSupport.generateEffectiveSchema` with shredding disabled,
which is the clustering/compaction path that reaches it.
- Both recursion loops built a full replacement field list and then
discarded it when nothing matched. That is the path every non-variant table
takes, on every `runMerge` and every avro parquet read, at the cost of an Avro
`Field` plus a `defaultVal()` lookup per field per level. The list is now built
lazily, backfilling only once a replacement appears.
- The two COW small-file merge tests were near-identical copies pinned to
AVRO. They are folded into one sweep over (record type, layout), which removes
the copy, keeps the unshredded twin, and adds SPARK coverage that nothing had.
Two repeated pins became `assertVariantLayout` / `assertSingleFileGroup`; the
layout pin alone had been copy-pasted 11 times in that file.
- The comments claiming nested reachability were overstated. They cited the
row writer's depth recursion as the producer, but the forced-shredding hook is
top-level only in BOTH write supports, so no DDL or table property reaches
depth - only a hand-authored write schema can. The comments now say that, and
say why the nested coverage is unit level rather than end to end.
### Impact
Restores the shredded-variant payload for base files whose variant group
omits the optional `value` column, 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: `alignShreddedVariants` can never fire for
`HoodieSparkParquetReader`, because its `getSchema()` 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: `alignShreddedVariants`
730us -> 335us per `runMerge`, `HoodieVariantReconstruction.create` 834us ->
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 (`assertSame` in
`TestHoodieSchemaCompatibility`). Verified on spark4.1/scala-2.13 end to end,
plus hudi-common 371/371, `TestHoodieVariantReconstruction` 12/12 and the
hudi-hadoop-common parquet suites.
### 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
