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);