puskarpeter opened a new pull request, #3752:
URL: https://github.com/apache/parquet-java/pull/3752
> [!NOTE]
> **Draft — stacked on #3750.** This fix reuses the terminate-as-proto-bytes
machinery introduced
> by the empty-message fix (GH-2142), so only the top commit belongs to this
PR; the base commit
> is #3750's. I will rebase onto master and mark this ready for review once
#3750 merges.
### Rationale for this change
PARQUET-1711 truncates recursive proto fields at
`parquet.proto.maxRecursion` depth by replacing
them with the serialized proto bytes, but hardcodes the replacement column
as `optional binary`,
ignoring the field's actual repetition. `ProtoWriteSupport` still wraps
repeated fields' writers in
`ArrayWriter`/`RepeatedWriter` and map fields in `MapWriter`, which emit
record structure the
`optional binary` column cannot hold. As a result (see #3751):
- specs-compliant mode: writing data that actually nests past `maxRecursion`
through a *repeated*
recursive field crashes with `ClassCastException: PrimitiveColumnIO cannot
be cast to
GroupColumnIO` — a data-dependent failure that passes schema creation and
shallow rows;
- old style: writing more than one repeated element at the truncation depth
emits inconsistent
repetition levels and **corrupts the file** — depending on the data,
reading it back either
fails with `ParquetDecodingException` or silently returns a wrong tree
(elements lost or
attached to phantom duplicate nodes);
- when the recursion budget runs out at a *map* field itself (e.g.
`google.protobuf.Struct` maps
reached through `list_value` branches), the whole MAP — including its keys
— collapses into one
binary in the schema, and writing data through it crashes with the same
`ClassCastException`.
The existing mock-based tests
(`ProtoWriteSupportTest.testRepeatedRecursion`/`testMapRecursion`)
never validate against a real `MessageColumnIO`, which is why the mismatch
went unnoticed.
### What changes are included in this PR?
`ProtoSchemaConverter.addMessageField` keeps the field's shape when
truncating, reusing the
terminate-as-proto-bytes path introduced for empty message types (#3750,
which this builds on):
- repeated + specs-compliant → LIST-wrapped binary via the existing
`addRepeatedPrimitive`;
- otherwise `builder.primitive(BINARY, getRepetition(descriptor))` —
`repeated binary` in the old
style; truncated optional fields stay `optional binary`, byte-for-byte
identical to before
(proto2 `required` fields in a recursion cycle now keep their `required`
repetition instead of
being forced `optional`);
- the specs-compliant map branch now runs *before* the recursion check, so
the MAP structure
(typed key) is always preserved and a recursive value type is truncated to
`optional binary`
inside `key_value` when `addMapField` recurses into the value field — same
recursion budget,
applied at the level where the recursion actually is.
The writer side needs no further changes: the `getContentType` check from
#3750 already selects
`BinaryWriter` behind LIST/MAP wrappers, so the existing
`ArrayWriter`/`RepeatedWriter`/`MapWriter` wrapping then lines up with the
schema.
### Are these changes tested?
Yes. New `ProtoRecursionTruncationTest` (5 tests) writes recursive data
deeper than `maxRecursion`
through the real write path (`ProtoParquetWriter` → `MessageColumnIO`) and
reads it back:
- repeated recursion (`Trees.WideTree`), specs-compliant **and** old style —
previously the
`ClassCastException` and the file-corrupting write, respectively; now
every element at the
truncation depth round-trips as the serialized subtree;
- a map field that exhausts the recursion budget (`google.protobuf.Struct`
behind `list_value`) —
previously the whole-MAP collapse plus the same `ClassCastException` when
data reached it; now
keys stay typed and queryable, values round-trip as serialized protos;
- map-value recursion on the main `Struct` path and optional recursion
(`Trees.BinaryTree`) as
regression guards for the shapes that already worked.
Expected-schema fixtures were regenerated for the new truncation shape:
`WideTree.par`,
`Value.par`, `Struct.par`, the inline schemas in `ProtoSchemaConverterTest`,
and the
`testDeepRecursion` Struct fan-out series (now `2n+5` — a truncated map
keeps its key column). The
full parquet-protobuf suite passes.
### Are there any user-facing changes?
Schemas containing *repeated* or *map* recursive fields change shape at the
truncation depth
(LIST-of-binary / MAP-with-binary-value instead of a single `optional
binary`) — but writing more
than one element at that depth previously crashed (specs mode) or corrupted
the file (old style),
so no valid existing files carry meaningful multi-element data in the old
shape. Truncated
optional fields are unchanged; proto2 `required` fields in a recursion cycle
now map to `required
binary` (previously forced `optional`). Data past the truncation depth now
round-trips losslessly:
each binary cell is the serialized subtree, recoverable with
`X.parseFrom(bytes)`.
Closes #3751
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
