wombatu-kun commented on code in PR #19620:
URL: https://github.com/apache/hudi/pull/19620#discussion_r3791657901
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/avro/TestHoodieAvroWriteSupportShredding.java:
##########
@@ -68,4 +72,43 @@ void forcedShreddingDdlTreatsDecimalParensAsOneField() {
.collect(Collectors.toList());
assertEquals(Arrays.asList("a", "b", "c"), shreddedFieldNames);
}
+
+ /**
+ * Disabling shredding over an already-shredded schema - the
clustering/compaction case
+ * {@link HoodieAvroWriteSupport#generateEffectiveSchema} calls out - has to
strip typed_value
+ * without tripping Avro's "Field already used". Rebuilding a record while
reusing a
+ * {@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.
Review 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.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/schema/TestVariantDataType.scala:
##########
@@ -1250,6 +1184,34 @@ class TestVariantDataType extends HoodieSparkSqlTestBase
{
})
}
+ /**
+ * Pins the on-disk layout of the `v` column across every base file. Without
it a leg meant to
+ * 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 = {
Review 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.
##########
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) {
+ // The shredding spec lets a writer omit `value` when every row is typed,
and
+ // HoodieSchema.Variant.determineIfShredded calls anything with a
typed_value shredded. Shape
+ // detection is the only detector that runs on real files, since the
footer strips the logical
+ // type, so requiring `value` here dropped such a column's payload
silently - #19567 again by
+ // another shape. Hudi's own writer always emits three fields, so this is
about files written
+ // elsewhere.
+ HoodieSchema twoFieldShredded = HoodieSchema.createRecord("v",
"org.apache.hudi.test", null, Arrays.asList(
+ HoodieSchemaField.of("metadata",
HoodieSchema.create(HoodieSchemaType.BYTES)),
+ HoodieSchemaField.of("typed_value",
HoodieSchema.createNullable(HoodieSchemaType.INT))));
+ HoodieSchema fileSchema = recordWithIdAndVariant(twoFieldShredded);
+ HoodieSchema requestedSchema =
recordWithIdAndVariant(HoodieSchema.createVariant());
+ HoodieStorage storage = storageWithReadingShredded(tmp, true);
+
storage.getConf().set(HoodieStorageConfig.PARQUET_VARIANT_SHREDDING_PROVIDER_CLASS.key(),
+ TestVariantShreddingProvider.class.getName());
+
+ HoodieVariantReconstruction reconstruction =
HoodieVariantReconstruction.create(
+ fileSchema, requestedSchema, storage);
+ assertNotNull(reconstruction, "A shredded group with no value column must
still engage");
+
+ GenericRecord shredded = new GenericData.Record(
+
reconstruction.intermediateSchema().getField("v").get().schema().getNonNullType().toAvroSchema());
+ shredded.put("metadata", ByteBuffer.wrap(new byte[] {1}));
+ shredded.put("typed_value", 42);
+ GenericRecord input = new
GenericData.Record(reconstruction.intermediateSchema().toAvroSchema());
+ input.put("id", "record-1");
+ input.put("v", shredded);
+
+ GenericRecord variant = (GenericRecord)
reconstruction.reconstruct(input).get(1);
+ assertEquals(ByteBuffer.wrap(new byte[] {42}), variant.get("value"));
+ }
+
+ @Test
+ void ignoresFourFieldStructThatMerelyCarriesTypedValue(@TempDir Path tmp) {
Review 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}`.
##########
hudi-spark-datasource/hudi-spark4-common/src/test/java/org/apache/hudi/io/storage/hadoop/TestHoodieVariantReconstructionRoundTrip.java:
##########
@@ -104,6 +105,70 @@ void
createThenReconstructRebuildsVariantAndPassesThroughNonVariant(@TempDir Pat
assertEquals(original.toJson(ZoneOffset.UTC),
rebuilt.toJson(ZoneOffset.UTC));
}
+ @Test
+ void createThenReconstructRebuildsAValueLessShreddedGroup(@TempDir Path tmp)
throws Exception {
Review 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.
--
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]
