Skip to content

GH-3751: Preserve field repetition when truncating recursive proto fields - #3752

Open
puskarpeter wants to merge 1 commit into
apache:masterfrom
puskarpeter:recursion-truncation-repetition
Open

puskarpeter wants to merge 1 commit into
apache:masterfrom
puskarpeter:recursion-truncation-repetition

Conversation

@puskarpeter

@puskarpeter puskarpeter commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

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 in #3750:

  • 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. The read side
is likewise covered by the binary-to-message converter from #3750.

This also makes the "Message fields stored as proto bytes" section of the parquet-protobuf README
(added in #3750) accurate for the recursion case: it describes the truncated column as keeping the
field's repetition, which until now only held for empty message types.

Are these changes tested?

Yes. New ProtoRecursionTruncationTest (6 tests) writes recursive data deeper than maxRecursion
through the real write path (ProtoParquetWriterMessageColumnIO) 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;
  • ProtoParquetReader round trip: a WideTree deeper than maxRecursion (both modes) and a
    map-recursive Struct read back equal to the original messages — with the binary-to-message
    read path from GH-2142: Write fields of empty message types as proto bytes #3750, truncation is now lossless end to end.

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

…oto fields

The maxRecursion truncation (PARQUET-1711) replaced recursive fields
with a hardcoded optional binary, while ProtoWriteSupport still wraps
repeated/map fields' writers in ArrayWriter/RepeatedWriter/MapWriter.
Writing data that nests deeper than maxRecursion through a repeated
recursive field crashed with a ClassCastException in parquet-specs mode
and corrupted the file in the old style (inconsistent repetition
levels: reads fail with ParquetDecodingException or return a wrong
tree). A map field exhausting the recursion budget collapsed entirely
- keys included - into one binary, and writing data through it crashed
the same way.

Truncate to proto bytes preserving the field's shape instead, reusing
the terminate-as-bytes path introduced for empty message types
(apacheGH-2142): LIST-wrapped binary for repeated fields in specs mode,
repeated binary in the old style, and the MAP structure kept with the
recursive value truncated inside key_value (the map branch now runs
before the recursion check). Truncated optional fields are unchanged;
proto2 required fields now keep their required repetition. Each
truncated cell round-trips as the serialized subtree, and with the
binary-to-message read path from apacheGH-2142 the truncated messages read
back losslessly through ProtoParquetReader.

Signed-off-by: Puškár, Peter <peter.puskar@firma.seznam.cz>
@puskarpeter
puskarpeter force-pushed the recursion-truncation-repetition branch from f9cf76f to 8eaa12b Compare September 23, 2026 15:38
@puskarpeter
puskarpeter marked this pull request as ready for review September 23, 2026 15:38
@puskarpeter

Copy link
Copy Markdown
Contributor Author

@wgtmac Now that #3750 is merged I rebased this one on master and marked it ready. It is the follow-up mentioned in #3751: the same terminate-as-proto-bytes path, applied to recursion truncation so repeated and
map fields keep their shape instead of collapsing into a single optional binary. Only ProtoSchemaConverter changes, the writer and reader side are already covered by #3750.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

parquet-protobuf: recursion truncation breaks repeated and map fields — ClassCastException in specs-compliant mode, file corruption in the old style

2 participants