puskarpeter opened a new issue, #3751:
URL: https://github.com/apache/parquet-java/issues/3751
### Describe the bug
`ProtoSchemaConverter` (PARQUET-1711) terminates recursive proto message
fields at
`parquet.proto.maxRecursion` depth by replacing them with the serialized
proto bytes. The
truncation hardcodes the replacement column as `optional binary`, ignoring
the original field's
repetition:
```java
// ProtoSchemaConverter.addMessageField
if (seen.get(typeName).size() > maxRecursion) {
return builder.primitive(BINARY,
Type.Repetition.OPTIONAL).as((LogicalTypeAnnotation) null);
}
```
`ProtoWriteSupport.MessageWriter` however still wraps every repeated field's
writer in
`ArrayWriter` (specs-compliant) or `RepeatedWriter` (old style), and map
fields in `MapWriter` —
all of which emit record structure the `optional binary` column cannot hold.
The mock-based unit
tests (`ProtoWriteSupportTest.testRepeatedRecursion` / `testMapRecursion`)
never validate against a
real `MessageColumnIO`, so the mismatch was never caught.
Consequences, each reproduced by an end-to-end write test included in the
follow-up PR:
1. **Specs-compliant mode, repeated recursive field, data deeper than
maxRecursion:** the write
crashes with
```
java.lang.ClassCastException: class
org.apache.parquet.io.PrimitiveColumnIO cannot be cast to
class org.apache.parquet.io.GroupColumnIO
```
(`ArrayWriter` calls `startGroup()`/`startField("list", 0)` against a
primitive column). This is
a data-dependent landmine: schema creation and shallow rows succeed; the
job dies only when a
row's data actually nests past the limit.
2. **Old style (writeSpecsCompliant=false), repeated recursive field with
more than one element at
the truncation depth:** no exception — the write emits inconsistent
repetition levels for the
second and following elements, **corrupting the file**. Depending on the
data, reading it back
either fails with `ParquetDecodingException` (`Can not read value at ...
in block`,
EOF/BufferUnderflow underneath) or silently returns a wrong tree
(elements lost or attached to
phantom duplicate parent nodes).
3. **Specs-compliant map field at which the recursion budget runs out** (e.g.
`google.protobuf.Struct` maps reached through `list_value` branches): the
whole MAP — including
its keys — collapses into a single unreadable binary in the schema, and
writing data through
that branch crashes with the same `ClassCastException` (`MapWriter`
navigating `key_value`
groups over a primitive column). On map paths where the budget happens to
trip at a singular
field first (like `Struct`'s main `fields → struct_value` chain), the
schema was already fine —
the collapse is branch-dependent.
### Reproducer
Any repeated self-recursive message nested deeper than maxRecursion, e.g.
the existing test proto
`Trees.WideTree`:
```java
Trees.WideTree deep = ...; // chain of children 5 levels deep, 2 children
per node
Configuration conf = new Configuration();
ProtoWriteSupport.setWriteSpecsCompliant(conf, true);
ProtoSchemaConverter.setMaxRecursion(conf, 2);
try (ParquetWriter<Message> w = ProtoParquetWriter.<Message>builder(path)
.withMessage(Trees.WideTree.class).withConf(conf).build()) {
w.write(deep); // ClassCastException
}
```
Affects all released versions since 1.13.0 (PARQUET-1711) through current
master (verified on
1.17.1 and master @ e02f65e2).
### Proposed fix (PR follows)
Preserve the field's shape when truncating, mirroring how ordinary repeated
primitives are handled:
- `ProtoSchemaConverter.addMessageField`:
- repeated + specs-compliant → LIST-wrapped binary via the existing
`addRepeatedPrimitive`
(`optional group x (LIST) { repeated group list { required binary
element } }`);
- otherwise `builder.primitive(BINARY, getRepetition(descriptor))`
(`repeated binary` in the old
style; truncated optional fields unchanged, proto2 required fields now
keep `required`);
- specs-compliant map fields keep their MAP structure unconditionally; a
recursive **value** type
is truncated to `optional binary` inside `key_value` when `addMapField`
recurses into the value
field (same recursion budget, applied one level deeper where it belongs).
- `ProtoWriteSupport.createMessageWriter`: look through the LIST/MAP wrapper
when detecting a
truncated-to-binary message field (`getContentType`, introduced by the fix
for #2142, which
terminates empty message types through the same mechanism) so
`BinaryWriter` is selected for
truncated elements/values; the existing
`ArrayWriter`/`RepeatedWriter`/`MapWriter` wrapping then
lines up with the schema.
With the fix, each repeated element / map value at the truncation depth
round-trips as one binary
containing the serialized subtree (`X.parseFrom(bytes)` reconstructs it),
keys of truncated-value
maps stay queryable, and truncated optional fields are byte-for-byte
unchanged.
Existing expected-schema tests were regenerated (`WideTree.par`,
`Value.par`, `Struct.par`,
inline schemas, and the `testDeepRecursion` Struct fan-out series changes
from `2n+4` to `2n+5`
because a truncated map now retains its key column). New
`ProtoRecursionTruncationTest` (5 tests)
writes through a real `MessageColumnIO`: repeated recursion in both modes, a
map field exhausting
the recursion budget (fails with the ClassCastException before the fix), and
the already-working
map-main-path and optional cases as regression guards.
Note on schema compatibility: files previously written with a truncated
*optional* field are
unchanged. A repeated/map truncated field changes its schema shape — but
writing more than one
element at the truncation depth crashed (specs) or corrupted the file (old
style) before, so no
valid existing files carry the old shape with meaningful multi-element data.
### Related
- PARQUET-1711 / #995 — introduced maxRecursion truncation (optional-field
case only).
- #2708 / PARQUET-2181 — read-side ClassCastException in parquet-cli on
proto files; note that
`ProtoParquetReader` itself also cannot read back **any** truncated field
(including the optional
case that writes fine): `ProtoMessageConverter.newScalarConverter` has no
binary→message path and
throws `ClassCastException` at converter-tree construction. That read-side
gap is orthogonal to
this write-side fix and probably deserves its own issue.
- #2142 — empty message types cannot be written at all; the companion PR
fixes it with the same
terminate-as-proto-bytes mechanism, and this fix builds on it.
--
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]
