voonhous commented on code in PR #19620:
URL: https://github.com/apache/hudi/pull/19620#discussion_r3783456205
##########
hudi-common/src/main/java/org/apache/hudi/common/avro/VariantSchemaUtils.java:
##########
@@ -166,30 +175,44 @@ public static HoodieSchema
toShreddedReadSchema(HoodieSchema requestedSchema, Ho
/**
* Walks {@code base} against its matching {@code other} fields by name,
replacing every shredded
- * variant position with the other side's schema. Recurses through records,
array elements and map
- * values, since the row writer shreds variants at any depth
- * ({@code HoodieRowParquetWriteSupport.processNestedDataType}). {@code
baseIsFile} says which of
- * the two is the file side, which is what {@link #isShreddedVariantTarget}
needs to anchor
- * detection. Returns {@code base} when nothing matches.
+ * variant position with the other side's schema. {@code baseIsFile} says
which of the two is the
+ * file side, which is what {@link #isShreddedVariantTarget} needs to anchor
detection. Returns
+ * {@code base} when nothing matches.
+ *
+ * <p>The walk recurses through records, array elements and map values
because the row writer
+ * shreds at any depth its write schema asks it to
+ * ({@code HoodieRowParquetWriteSupport.processNestedDataType}). Note what
can actually produce
+ * such a file today: the forced-shredding test hook is top-level only in
BOTH write supports
Review Comment:
Good catch, the note was wrong. `generateShreddedSchema` parses the
forced-shredding DDL at the top of every call and `processNestedDataType` calls
back into it for nested structs, so `struct<v variant>` plus the property does
shred at depth on the row path, and `makeWriter` picks that up via the
`isVariantShreddingStruct` branch before the plain-struct one. Scoped the claim
to `HoodieAvroWriteSupport.applyForcedShreddingSchema`, which really is a flat
loop over top-level fields, and fixed the two copies in
`HoodieVariantReconstruction.buildRebuilder` and the nested reconstruction
test. f364d37
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/io/storage/hadoop/TestHoodieVariantReconstruction.java:
##########
@@ -186,6 +186,53 @@ void
returnsNullForFooterDerivedPlainUnshreddedShape(@TempDir Path tmp) {
storageWithReadingShredded(tmp, false)));
}
+ @Test
+ void engagesOnTwoFieldShreddedShapeWithNoValueColumn(@TempDir Path tmp) {
Review Comment:
Right, that test only pins detection. Added
`createThenReconstructRebuildsAValueLessShreddedGroup` to
`TestHoodieVariantReconstructionRoundTrip`, where the real provider runs: it
shreds `{"a":"x","b":5}` against `a string, b long` (residual `value` comes
back null, asserted), drops the `value` column to get the `{metadata,
typed_value}` shape a spec-conforming external writer would emit, then goes
through `create` + `reconstruct` and compares JSON against the original. That
is the path where `buildVariantSchema` assigns `variantIdx = -1` and shifts
`topLevelMetadataIdx`/`typedIdx` down, so `AvroVariantRow.fieldNameFor` and
`ShreddingUtils.rebuild`'s residual check are both exercised on the value-less
ordinals. f364d37
--
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]
