You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "voonhous (via GitHub)" <gi...@apache.org> on 2026/08/13 10:52:29 UTC
[PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #19620:
URL: https://github.com/apache/hudi/pull/19620#discussion_r3793934063
##########
.github/workflows/bot.yml:
##########
@@ -732,9 +732,12 @@ jobs:
# - scalaProfile: "scala-2.13"
# sparkProfile: "spark4.1"
# sparkModules: "hudi-spark-datasource/hudi-spark4.1.x"
+ # hudi-spark4-common runs here (and only here): the other spark lanes either pin a
+ # fixed test-class list or run scala/functional profiles, so without this entry the
+ # module's java unit tests are compiled but never executed in CI.
- scalaProfile: "scala-2.13"
sparkProfile: "spark4.2"
- sparkModules: "hudi-spark-datasource/hudi-spark4.2.x"
+ sparkModules: "hudi-spark-datasource/hudi-spark4-common,hudi-spark-datasource/hudi-spark4.2.x"
Review Comment:
The Summary and Changelog does not mention that this PR now edits bot.yml to add hudi-spark4-common to the spark4.2 java-test lane, which starts running its two test classes on every PR. Worth a bullet there.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19620:
URL: https://github.com/apache/hudi/pull/19620#issuecomment-5279641530
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16273",
"triggerID" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 63e8c5e2a87a5a4445902fdce7ebf69817c82446 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16273)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19620:
URL: https://github.com/apache/hudi/pull/19620#discussion_r3794165927
##########
.github/workflows/bot.yml:
##########
@@ -732,9 +732,12 @@ jobs:
# - scalaProfile: "scala-2.13"
# sparkProfile: "spark4.1"
# sparkModules: "hudi-spark-datasource/hudi-spark4.1.x"
+ # hudi-spark4-common runs here (and only here): the other spark lanes either pin a
+ # fixed test-class list or run scala/functional profiles, so without this entry the
+ # module's java unit tests are compiled but never executed in CI.
- scalaProfile: "scala-2.13"
sparkProfile: "spark4.2"
- sparkModules: "hudi-spark-datasource/hudi-spark4.2.x"
+ sparkModules: "hudi-spark-datasource/hudi-spark4-common,hudi-spark-datasource/hudi-spark4.2.x"
Review Comment:
Added to the Summary and Changelog.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19620:
URL: https://github.com/apache/hudi/pull/19620#issuecomment-5311889148
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16273",
"triggerID" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"triggerType" : "PUSH"
}, {
"hash" : "f364d3715fc1a6542d713fd65aef078977ecd875",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16303",
"triggerID" : "f364d3715fc1a6542d713fd65aef078977ecd875",
"triggerType" : "PUSH"
}, {
"hash" : "4da5636287c544ed9285968123632137697838d8",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16327",
"triggerID" : "4da5636287c544ed9285968123632137697838d8",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f364d3715fc1a6542d713fd65aef078977ecd875 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16303)
* 4da5636287c544ed9285968123632137697838d8 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16327)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19620:
URL: https://github.com/apache/hudi/pull/19620#issuecomment-5294798304
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16273",
"triggerID" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"triggerType" : "PUSH"
}, {
"hash" : "f364d3715fc1a6542d713fd65aef078977ecd875",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16303",
"triggerID" : "f364d3715fc1a6542d713fd65aef078977ecd875",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f364d3715fc1a6542d713fd65aef078977ecd875 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16303)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19620:
URL: https://github.com/apache/hudi/pull/19620#issuecomment-5311844801
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16273",
"triggerID" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"triggerType" : "PUSH"
}, {
"hash" : "f364d3715fc1a6542d713fd65aef078977ecd875",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16303",
"triggerID" : "f364d3715fc1a6542d713fd65aef078977ecd875",
"triggerType" : "PUSH"
}, {
"hash" : "4da5636287c544ed9285968123632137697838d8",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "4da5636287c544ed9285968123632137697838d8",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f364d3715fc1a6542d713fd65aef078977ecd875 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16303)
* 4da5636287c544ed9285968123632137697838d8 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19620:
URL: https://github.com/apache/hudi/pull/19620#issuecomment-5312528749
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16273",
"triggerID" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"triggerType" : "PUSH"
}, {
"hash" : "f364d3715fc1a6542d713fd65aef078977ecd875",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16303",
"triggerID" : "f364d3715fc1a6542d713fd65aef078977ecd875",
"triggerType" : "PUSH"
}, {
"hash" : "4da5636287c544ed9285968123632137697838d8",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16327",
"triggerID" : "4da5636287c544ed9285968123632137697838d8",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 4da5636287c544ed9285968123632137697838d8 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16327)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "hudi-agent (via GitHub)" <gi...@apache.org>.
hudi-agent commented on code in PR #19620:
URL: https://github.com/apache/hudi/pull/19620#discussion_r3774828812
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/schema/TestVariantDataType.scala:
##########
@@ -1253,6 +1187,34 @@ class TestVariantDataType extends HoodieSparkSqlTestBase {
/**
Review Comment:
π€ nit: the existing `/** Lists data parquet files β¦ */` Javadoc was left in place when `assertVariantLayout` and `assertSingleFileGroup` were inserted above `listDataParquetFiles` β it now sits directly above `assertVariantLayout` and documents the wrong method. Could you move it down to sit directly above `listDataParquetFiles` where it belongs?
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag quality.</i></sub>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19620:
URL: https://github.com/apache/hudi/pull/19620#discussion_r3793816514
##########
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:
Folded in 4da5636.
##########
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:
Went with the CI option in 4da5636: added the module to the spark4.2 entry of test-spark-java17-java-tests-part1. That is the only lane where java UTs run off an exclusion filter (part2 pins three named classes, part3 runs the functional profiles), and the module was already built there via -am since spark4.2.x depends on it, so listing it in the test -pl is all that was missing. Its surefire skip property (skip.hudi-spark4.unit.tests) defaults to skipTests, which the test step does not set, so both test classes in the module now execute.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19620:
URL: https://github.com/apache/hudi/pull/19620#issuecomment-5279785851
## [Codecov](https://app.codecov.io/gh/apache/hudi/pull/19620?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `24.24242%` with `25 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 74.90%. Comparing base ([`e1dd86e`](https://app.codecov.io/gh/apache/hudi/commit/e1dd86e968ebff25c43ad5d9774c3cf19fc6bb43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`63e8c5e`](https://app.codecov.io/gh/apache/hudi/commit/63e8c5e2a87a5a4445902fdce7ebf69817c82446?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/19620?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...rg/apache/hudi/common/avro/VariantSchemaUtils.java](https://app.codecov.io/gh/apache/hudi/pull/19620?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Favro%2FVariantSchemaUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL2F2cm8vVmFyaWFudFNjaGVtYVV0aWxzLmphdmE=) | 24.24% | [19 Missing and 6 partials :warning: ](https://app.codecov.io/gh/apache/hudi/pull/19620?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19620 +/- ##
============================================
- Coverage 74.92% 74.90% -0.02%
+ Complexity 31830 31826 -4
============================================
Files 2524 2524
Lines 139500 139516 +16
Branches 16779 16784 +5
============================================
- Hits 104514 104510 -4
- Misses 27261 27279 +18
- Partials 7725 7727 +2
```
| [Components](https://app.codecov.io/gh/apache/hudi/pull/19620/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [hudi-common](https://app.codecov.io/gh/apache/hudi/pull/19620/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.72% <24.24%> (-0.06%)` | :arrow_down: |
| [hudi-client](https://app.codecov.io/gh/apache/hudi/pull/19620/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.22% <ΓΈ> (ΓΈ)` | |
| [hudi-flink](https://app.codecov.io/gh/apache/hudi/pull/19620/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `85.74% <ΓΈ> (ΓΈ)` | |
| [hudi-spark-datasource](https://app.codecov.io/gh/apache/hudi/pull/19620/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.14% <ΓΈ> (ΓΈ)` | |
| [hudi-utilities](https://app.codecov.io/gh/apache/hudi/pull/19620/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.65% <ΓΈ> (ΓΈ)` | |
| [hudi-cli](https://app.codecov.io/gh/apache/hudi/pull/19620/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `15.32% <ΓΈ> (ΓΈ)` | |
| [hudi-hadoop](https://app.codecov.io/gh/apache/hudi/pull/19620/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.99% <ΓΈ> (ΓΈ)` | |
| [hudi-sync](https://app.codecov.io/gh/apache/hudi/pull/19620/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.58% <ΓΈ> (ΓΈ)` | |
| [hudi-io](https://app.codecov.io/gh/apache/hudi/pull/19620/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.23% <ΓΈ> (ΓΈ)` | |
| [hudi-timeline-service](https://app.codecov.io/gh/apache/hudi/pull/19620/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `77.57% <ΓΈ> (ΓΈ)` | |
| [hudi-cloud](https://app.codecov.io/gh/apache/hudi/pull/19620/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.06% <ΓΈ> (ΓΈ)` | |
| [hudi-kafka-connect](https://app.codecov.io/gh/apache/hudi/pull/19620/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.20% <ΓΈ> (ΓΈ)` | |
| [Flag](https://app.codecov.io/gh/apache/hudi/pull/19620/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [hadoop-mr-java-client](https://app.codecov.io/gh/apache/hudi/pull/19620/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.84% <24.24%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/19620?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [...io/storage/hadoop/HoodieVariantReconstruction.java](https://app.codecov.io/gh/apache/hudi/pull/19620?src=pr&el=tree&filepath=hudi-hadoop-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fio%2Fstorage%2Fhadoop%2FHoodieVariantReconstruction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1oYWRvb3AtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWRpL2lvL3N0b3JhZ2UvaGFkb29wL0hvb2RpZVZhcmlhbnRSZWNvbnN0cnVjdGlvbi5qYXZh) | `92.55% <ΓΈ> (ΓΈ)` | |
| [...rg/apache/hudi/common/avro/VariantSchemaUtils.java](https://app.codecov.io/gh/apache/hudi/pull/19620?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Favro%2FVariantSchemaUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL2F2cm8vVmFyaWFudFNjaGVtYVV0aWxzLmphdmE=) | `59.50% <24.24%> (-12.88%)` | :arrow_down: |
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19620:
URL: https://github.com/apache/hudi/pull/19620#issuecomment-5279576870
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 63e8c5e2a87a5a4445902fdce7ebf69817c82446 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #19620:
URL: https://github.com/apache/hudi/pull/19620#discussion_r3780557806
##########
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:
`generateShreddedSchema` re-reads the forced-shredding DDL on every recursive entry from `processNestedDataType`, and `makeWriter` hands a nested shredded struct a real shredded writer, so `struct<v variant>` plus `hoodie.parquet.variant.force.shredding.schema.for.test` does reach depth through the row writer. Worth scoping the claim to `HoodieAvroWriteSupport.applyForcedShreddingSchema` here and in its two copies (`HoodieVariantReconstruction.buildRebuilder` and the nested reconstruction test).
##########
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:
`TestVariantShreddingProvider` ignores its `shreddedSchema` argument and reads `metadata`/`typed_value` by name, so this pins detection only - nothing here exercises `Spark4VariantShreddingProvider.buildVariantSchema` with the `variantIdx = -1` that a value-less group produces. Worth a value-less case in `TestHoodieVariantReconstructionRoundTrip`, which is the one test that runs the real provider through reconstruction.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19620:
URL: https://github.com/apache/hudi/pull/19620#issuecomment-5293113452
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16273",
"triggerID" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"triggerType" : "PUSH"
}, {
"hash" : "f364d3715fc1a6542d713fd65aef078977ecd875",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "f364d3715fc1a6542d713fd65aef078977ecd875",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 63e8c5e2a87a5a4445902fdce7ebf69817c82446 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16273)
* f364d3715fc1a6542d713fd65aef078977ecd875 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19620:
URL: https://github.com/apache/hudi/pull/19620#discussion_r3793815750
##########
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:
Reworded in 4da5636 to carry the same scoping as the other two comments: applyForcedShreddingSchema is top-level only, so the nested shredded schema this strip sees comes from a clustering/compaction schema read back from a file the ROW path (or another engine) wrote, not from this path's own hook.
##########
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:
Right, the four-field case never gets past the count guard, so the no-value arm was mutation-blind. Added ignoresThreeFieldStructWhoseThirdFieldIsNotValue with {metadata, typed_value, extra} in 4da5636: it survives the count guard and pins that arm, so mutating fieldCount == 2 to true now goes red (create would throw the reading-disabled exception instead of returning null).
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19620:
URL: https://github.com/apache/hudi/pull/19620#issuecomment-5293162435
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16273",
"triggerID" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"triggerType" : "PUSH"
}, {
"hash" : "f364d3715fc1a6542d713fd65aef078977ecd875",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16303",
"triggerID" : "f364d3715fc1a6542d713fd65aef078977ecd875",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 63e8c5e2a87a5a4445902fdce7ebf69817c82446 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16273)
* f364d3715fc1a6542d713fd65aef078977ecd875 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16303)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on PR #19620:
URL: https://github.com/apache/hudi/pull/19620#issuecomment-5279483320
@wombatu-kun Can you please help to review this, just some cleaning up and hardening ontop of #19582.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous merged PR #19620:
URL: https://github.com/apache/hudi/pull/19620
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19620:
URL: https://github.com/apache/hudi/pull/19620#discussion_r3783457703
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/schema/TestVariantDataType.scala:
##########
@@ -1253,6 +1187,34 @@ class TestVariantDataType extends HoodieSparkSqlTestBase {
/**
Review Comment:
Done, moved it back above `listDataParquetFiles`. 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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19620:
URL: https://github.com/apache/hudi/pull/19620#issuecomment-5280521998
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16273",
"triggerID" : "63e8c5e2a87a5a4445902fdce7ebf69817c82446",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 63e8c5e2a87a5a4445902fdce7ebf69817c82446 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16273)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(avro): detect the two-field shredded variant shape and harden the variant merge path [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #19620:
URL: https://github.com/apache/hudi/pull/19620#discussion_r3780561024
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/schema/TestVariantDataType.scala:
##########
@@ -1253,6 +1187,34 @@ class TestVariantDataType extends HoodieSparkSqlTestBase {
/**
Review Comment:
+1
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org