shangxinli commented on a change in pull request #778:
URL: https://github.com/apache/parquet-mr/pull/778#discussion_r422564200



##########
File path: 
parquet-column/src/test/java/org/apache/parquet/schema/TestPrimitiveStringifier.java
##########
@@ -309,6 +308,35 @@ public void testDecimalStringifier() {
     checkThrowingUnsupportedException(stringifier, Integer.TYPE, Long.TYPE, 
Binary.class);
   }
 
+  @Test
+  public void testUUIDStringifier() {
+    PrimitiveStringifier stringifier = PrimitiveStringifier.UUID_STRINGIFIER;
+
+    assertEquals("00112233-4455-6677-8899-aabbccddeeff", stringifier.stringify(
+        toBinary(0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 
0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff)));
+    assertEquals("00000000-0000-0000-0000-000000000000", stringifier.stringify(
+        toBinary(0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
0x00, 0x00, 0x00, 0x00, 0x00, 0x00)));
+    assertEquals("ffffffff-ffff-ffff-ffff-ffffffffffff", stringifier.stringify(
+        toBinary(0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 
0xff, 0xff, 0xff, 0xff, 0xff, 0xff)));
+
+    assertEquals("0eb1497c-19b6-42bc-b028-b4b612bed141", stringifier.stringify(

Review comment:
       By duplicate coverage, I meant the #2, #3 tests seems repeating the same 
test as #1. The value is different, but when the test executes, they would 
execute the same code path. So I think they won't provide extra coverage. 
   
   From test perspective, negative test does provide values. In this case, we 
can test the exception is thrown as expected if it is too short. 
   
   For "if the array is longer than 16 it would simply stringify the first 16 
and skip the others", that could cause silent errors, right? 
   




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


Reply via email to