[ 
https://issues.apache.org/jira/browse/PARQUET-1917?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17206167#comment-17206167
 ] 

ASF GitHub Bot commented on PARQUET-1917:
-----------------------------------------

belugabehr commented on a change in pull request #820:
URL: https://github.com/apache/parquet-mr/pull/820#discussion_r498818951



##########
File path: 
parquet-protobuf/src/test/java/org/apache/parquet/proto/ProtoWriteSupportTest.java
##########
@@ -913,4 +918,62 @@ public void testMessageWithExtensions() throws Exception {
 
     instance.write(msg.build());
   }
+
+  @Test
+  public void testMessageOneOf() {
+    RecordConsumer readConsumerMock =  Mockito.mock(RecordConsumer.class);
+    ProtoWriteSupport<TestProto3.OneOfTestMessage> spyWriter = 
createReadConsumerInstance(TestProto3.OneOfTestMessage.class, readConsumerMock);
+    final int theInt = 99;
+
+    TestProto3.OneOfTestMessage.Builder msg = 
TestProto3.OneOfTestMessage.newBuilder();
+    msg.setSecond(theInt);
+    spyWriter.write(msg.build());
+
+    InOrder inOrder = Mockito.inOrder(readConsumerMock);
+
+    inOrder.verify(readConsumerMock).startMessage();
+    inOrder.verify(readConsumerMock).startField("second", 1);
+    inOrder.verify(readConsumerMock).addInteger(theInt);
+    inOrder.verify(readConsumerMock).endField("second", 1);
+    inOrder.verify(readConsumerMock).endMessage();
+    Mockito.verifyNoMoreInteractions(readConsumerMock);
+  }
+
+  /**
+   * Ensure that a message with a oneOf gets written out correctly and can be
+   * read back as expected.
+   */
+  @Test
+  public void testMessageOneOfRoundTrip() throws IOException {
+
+    TestProto3.OneOfTestMessage.Builder msgBuilder = 
TestProto3.OneOfTestMessage.newBuilder();
+    msgBuilder.setSecond(99);
+    TestProto3.OneOfTestMessage theMessage = msgBuilder.build();
+
+    TestProto3.OneOfTestMessage.Builder msgBuilder2 = 
TestProto3.OneOfTestMessage.newBuilder();
+    TestProto3.OneOfTestMessage theMessageNothingSet = msgBuilder2.build();
+
+    TestProto3.OneOfTestMessage.Builder msgBuilder3 = 
TestProto3.OneOfTestMessage.newBuilder();
+    msgBuilder3.setFirst(42);
+    TestProto3.OneOfTestMessage theMessageFirstSet = msgBuilder3.build();
+
+    //Write them out and read them back
+    Path tmpFilePath = TestUtils.writeMessages(theMessage, 
theMessageNothingSet, theMessageFirstSet);
+    List<TestProto3.OneOfTestMessage> gotBack = 
TestUtils.readMessages(tmpFilePath, TestProto3.OneOfTestMessage.class);
+
+    //First message

Review comment:
       Bit of a nit here, but if you find yourself putting in comments like 
"this is test 1, this is test 2, this is test 3, etc." that is a clear sign 
that you need to break the test into multiple test methods.
   
   If a test fails, it will be much easier to figure out exactly what broke 
instead of having to step through all this code.  It also ensure that the tests 
are testing what is intended and that there aren't any side-effects/artifacts 
of the other test scenarios interfering.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[email protected]


> [parquet-proto] default values are stored in oneOf fields that aren't set
> -------------------------------------------------------------------------
>
>                 Key: PARQUET-1917
>                 URL: https://issues.apache.org/jira/browse/PARQUET-1917
>             Project: Parquet
>          Issue Type: Bug
>    Affects Versions: 1.12.0
>            Reporter: Aaron Blake Niskode-Dossett
>            Priority: Major
>
> SCHEMA
> --------
> {noformat}
> message Person {
>   int32 foo = 1;
>   oneof optional_bar {
>     int32 bar_int = 200;
>     int32 bar_int2 = 201;
>     string bar_string = 300;
>   }
> }{noformat}
>  
> CODE
> --------
> I set values for foo and bar_string
>  
> {noformat}
> for (int i = 0; i < 3; i += 1) {
>                 com.etsy.grpcparquet.Person message = Person.newBuilder()
>                         .setFoo(i)
>                         .setBarString("hello world")
>                         .build();
>                 message.writeDelimitedTo(out);
>             }{noformat}
> And then I write the protobuf file out to parquet.
>  
> RESULT
> -----------
> {noformat}
> $ parquet-tools show example.parquet                                          
>                                                                               
> +-------+-----------+------------+--------------+
> |   foo |   bar_int |   bar_int2 | bar_string   |
> |-------+-----------+------------+--------------|
> |     0 |         0 |          0 | hello world  |
> |     1 |         0 |          0 | hello world  |
> |     2 |         0 |          0 | hello world  |
> +-------+-----------+------------+--------------+{noformat}
>  
> bar_int and bar_int2 should be EMPTY for all three rows since only bar_string 
> is set in the oneof.  0 is the default value for int, but it should not be 
> stored.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Reply via email to