Repository navigation
GH-2142, GH-3751: Backport proto-bytes fixes to 1.18.x (#3750, #3752) - #3844
Open
puskarpeter wants to merge 2 commits into
Open
puskarpeter wants to merge 2 commits into
puskarpeter wants to merge 2 commits into
Conversation
…ache#3750) ### Rationale for this change Protobuf allows empty message definitions, but Parquet forbids empty groups. Converting a message that merely *contains* a field of an empty message type produces a schema with an empty group, which writer construction rejects with `InvalidSchemaException: Cannot write a schema with an empty group`. Such fields appear in real-world schemas (deprecated stubs, marker/placeholder messages), and a single one makes the whole message type unwritable. ### What changes are included in this PR? `ProtoSchemaConverter.addMessageField` terminates a field whose message type has no fields as a `BINARY` column holding the serialized message — the same mechanism PARQUET-1711 uses for recursion beyond `maxRecursion`. Since an empty message serializes to zero bytes, the column is cheap, and field **presence** still round-trips (null = unset vs empty bytes = set): - singular field → `optional binary stub` (or `required`, per the field); - repeated field, parquet-specs mode → LIST-wrapped binary via the existing `addRepeatedPrimitive`, so element cardinality survives; - repeated field, old style → `repeated binary`; - map value type → `optional binary value` inside the `key_value` group (keys stay typed). `ProtoWriteSupport.createMessageWriter`'s existing truncated-field check (primitive BINARY where a message field was declared → `BinaryWriter`) is generalized to look through the LIST/MAP wrapper (`getGroupType` → `getContentType`), so the writer tree lines up with these schemas. A message that is empty at the **root** is still rejected — there is no parent field to hold the bytes, and a Parquet file with zero columns is not representable. **Read side** (second commit, after review): `ProtoMessageConverter` used to cast every message field's Parquet type to a group, so a message field stored as `BINARY` failed with a `ClassCastException` while the converter tree was built — which also means files with PARQUET-1711 recursion truncation have been unreadable by `ProtoParquetReader` since 1.13.0 (apache#995 shipped with an explicit "TODO: ReadSupport"). A new `ProtoBinaryMessageConverter` parses the bytes back through `parentBuilder.newBuilderForField(field)` and hands the message to the existing parent container, so singular fields, LIST elements, old-style repeated fields and map values all round-trip without touching `ListConverter`/`MapConverter`. Parse failures surface as `ParquetDecodingException`. ### Are these changes tested? Yes. New `ProtoEmptyMessageTest` (new test messages `Stub`/`StubBox` in `Trees.proto`) writes through the real write path (`ProtoParquetWriter` → `MessageColumnIO`, both specs-compliant and old style) and reads back with `GroupReadSupport` and with `ProtoParquetReader`: - singular / repeated / map-value empty-message fields round-trip with correct cardinality and zero-byte values; - presence round-trips (set empty message vs unset field); - `ProtoParquetReader` reads the written messages back equal to the originals (specs-compliant and old style), and the same read path round-trips a `BinaryTree` truncated at `maxRecursion`; - an empty root message still fails with `InvalidSchemaException` ("Cannot write a schema with an empty group"). `ProtoSchemaConverterTest.testEmptyMessageFields` pins the converted schema. The full parquet-protobuf suite passes (117 tests). ### Are there any user-facing changes? Message types that previously could not be written to Parquet at all now can; fields of empty message types appear as (possibly LIST/MAP-wrapped) `binary` columns and read back into the original messages with `ProtoParquetReader`. Files with `maxRecursion` truncation, previously unreadable by `ProtoParquetReader`, now read back as well. No change for schemas that were previously writable. Error behavior for an empty root message is unchanged. Closes apache#2142
…oto fields (apache#3752) 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>
Contributor
Author
|
@wgtmac Here is the promised backport of the truncation fixes to 1.18.x :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Backport of #3750 and #3752 to 1.18.x. Both are clean cherry-picks of the master commits (c3def13 and 398b63e), no conflicts. They are bundled because #3752 builds on #3750 and cannot be applied without it.
Both fix bugs that affect 1.18.x users today:
InvalidSchemaException: Cannot write a schema with an empty group), and files withmaxRecursiontruncation cannot be read back byProtoParquetReader.ClassCastExceptionfor repeated recursive fields in specs-compliant mode, and corrupts the file in the old schema style.Could you please cherry-pick the two commits onto
parquet-1.18.xas they are instead of squash-merging, so each fix keeps its own commit like on master?What changes are included in this PR?
Are these changes tested?
Yes, the tests from both PRs are included. The full parquet-protobuf suite passes on this branch, built with
--release 11.Are there any user-facing changes?
Same as #3750 and #3752.