From 45ba8e068c2afd03973945bbe021ca976f6c2459 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 18 Sep 2026 07:25:55 +0000 Subject: [PATCH] GH-3039: Keep protobuf oneof fields optional when unwrapping wrappers Unwrap mode was forcing every primitive to REQUIRED, which is invalid for oneof members because only one alternative can be written. Keep oneof fields optional and add schema plus validated round-trip coverage. Co-authored-by: Sankalp Thakur --- .../parquet/proto/ProtoSchemaConverter.java | 9 +++-- .../proto/ProtoSchemaConverterTest.java | 23 +++++++++++++ .../parquet/proto/ProtoWriteSupportTest.java | 33 +++++++++++++++++++ 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoSchemaConverter.java b/parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoSchemaConverter.java index ff27b263e9..7add82dd17 100644 --- a/parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoSchemaConverter.java +++ b/parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoSchemaConverter.java @@ -149,7 +149,8 @@ public ProtoSchemaConverter(boolean parquetSpecsCompliant, int maxRecursion) { * @param maxRecursion The maximum recursion depth messages are allowed to go before terminating as * bytes instead of their actual schema. * @param unwrapProtoWrappers If set to true, unwrap common Proto wrappers like Timestamp and DoubleValue - * with corresponding OPTIONAL logical annotations. Primitive types become REQUIRED. + * with corresponding OPTIONAL logical annotations. Primitive types become REQUIRED, + * except oneof members, which remain optional because only one can be set. */ public ProtoSchemaConverter(boolean parquetSpecsCompliant, int maxRecursion, boolean unwrapProtoWrappers) { this.parquetSpecsCompliant = parquetSpecsCompliant; @@ -275,7 +276,11 @@ private Builder>, GroupBuilder> addF // the old schema style did not include the LIST wrapper around repeated fields return addRepeatedPrimitive(parquetType.primitiveType, parquetType.logicalTypeAnnotation, builder); } - Repetition repetition = unwrapProtoWrappers ? Repetition.REQUIRED : getRepetition(descriptor); + // Unwrap mode marks scalars REQUIRED (an unwrapped wrapper value is always present), + // but oneof members must stay optional: at most one alternative is written. + Repetition repetition = unwrapProtoWrappers && descriptor.getContainingOneof() == null + ? Repetition.REQUIRED + : getRepetition(descriptor); return builder.primitive(parquetType.primitiveType, repetition).as(parquetType.logicalTypeAnnotation); } diff --git a/parquet-protobuf/src/test/java/org/apache/parquet/proto/ProtoSchemaConverterTest.java b/parquet-protobuf/src/test/java/org/apache/parquet/proto/ProtoSchemaConverterTest.java index a4539e3397..f5e2d602c5 100644 --- a/parquet-protobuf/src/test/java/org/apache/parquet/proto/ProtoSchemaConverterTest.java +++ b/parquet-protobuf/src/test/java/org/apache/parquet/proto/ProtoSchemaConverterTest.java @@ -393,6 +393,29 @@ public void testProto3ConvertDateTimeMessageUnwrapped() throws Exception { testConversion(TestProto3.DateTimeMessage.class, expectedSchema, false, true); } + @Test + public void testProto3ConvertOneOf() { + String expectedSchema = JOINER.join( + "message TestProto3.OneOfTestMessage {", + " optional int32 first = 1;", + " optional int32 second = 2;", + "}"); + + testConversion(TestProto3.OneOfTestMessage.class, expectedSchema); + } + + @Test + public void testProto3ConvertOneOfUnwrapped() { + String expectedSchema = JOINER.join( + "message TestProto3.OneOfTestMessage {", + " optional int32 first = 1;", + " optional int32 second = 2;", + "}"); + + testConversion(TestProto3.OneOfTestMessage.class, expectedSchema, true, true); + testConversion(TestProto3.OneOfTestMessage.class, expectedSchema, false, true); + } + @Test public void testProto3ConvertWrappedMessageUnwrapped() throws Exception { String expectedSchema = "message TestProto3.WrappedMessage {\n" + " optional double wrappedDouble = 1;\n" diff --git a/parquet-protobuf/src/test/java/org/apache/parquet/proto/ProtoWriteSupportTest.java b/parquet-protobuf/src/test/java/org/apache/parquet/proto/ProtoWriteSupportTest.java index b29eea8f1d..77f96c6b5d 100644 --- a/parquet-protobuf/src/test/java/org/apache/parquet/proto/ProtoWriteSupportTest.java +++ b/parquet-protobuf/src/test/java/org/apache/parquet/proto/ProtoWriteSupportTest.java @@ -37,6 +37,7 @@ import org.apache.parquet.proto.test.TestProtobuf; import org.apache.parquet.proto.test.Trees; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import org.mockito.InOrder; import org.mockito.Mockito; @@ -1051,6 +1052,38 @@ public void testMessageOneOfRoundTrip() throws IOException { assertThat(gotBackThird.getTheOneofCase()).isEqualTo(TestProto3.OneOfTestMessage.TheOneofCase.FIRST); } + @Test + public void testMessageOneOfUnwrappedRoundTrip(@TempDir java.nio.file.Path tempDir) throws IOException { + TestProto3.OneOfTestMessage secondSet = + TestProto3.OneOfTestMessage.newBuilder().setSecond(99).build(); + TestProto3.OneOfTestMessage nothingSet = + TestProto3.OneOfTestMessage.newBuilder().build(); + TestProto3.OneOfTestMessage firstSet = + TestProto3.OneOfTestMessage.newBuilder().setFirst(42).build(); + + Path tmpFilePath = new Path(tempDir.resolve("oneof-unwrap.parquet").toUri()); + try (ParquetWriter writer = ProtoParquetWriter.builder(tmpFilePath) + .withMessage(TestProto3.OneOfTestMessage.class) + .config(ProtoWriteSupport.PB_UNWRAP_PROTO_WRAPPERS, "true") + .withValidation(true) + .build()) { + writer.write(secondSet); + writer.write(nothingSet); + writer.write(firstSet); + } + + List gotBack = + TestUtils.readMessages(tmpFilePath, TestProto3.OneOfTestMessage.class); + + assertThat(gotBack).hasSize(3); + assertThat(gotBack.get(0).getSecond()).isEqualTo(99); + assertThat(gotBack.get(0).getTheOneofCase()).isEqualTo(TestProto3.OneOfTestMessage.TheOneofCase.SECOND); + assertThat(gotBack.get(1).getTheOneofCase()) + .isEqualTo(TestProto3.OneOfTestMessage.TheOneofCase.THEONEOF_NOT_SET); + assertThat(gotBack.get(2).getFirst()).isEqualTo(42); + assertThat(gotBack.get(2).getTheOneofCase()).isEqualTo(TestProto3.OneOfTestMessage.TheOneofCase.FIRST); + } + @Test public void testMessageRecursion() { RecordConsumer readConsumerMock = Mockito.mock(RecordConsumer.class);