Address review comments

Author: divjotarora

parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveComparator.java

@@ -385,10 +385,10 @@ int compareBinary(Binary b1, Binary b2) {
       long nanos1 = bb1.getLong(0);
       long nanos2 = bb2.getLong(0);
       if (nanos1 < 0 || nanos1 > NANOSECONDS_PER_DAY) {
-        throw new IllegalArgumentException("Invalid nanos value: " + nanos1);
+        throw new IllegalArgumentException("Invalid nanos value (must be positive and less than 1 day): " + nanos1);
       }
       if (nanos2 < 0 || nanos2 > NANOSECONDS_PER_DAY) {
-        throw new IllegalArgumentException("Invalid nanos value: " + nanos2);
+        throw new IllegalArgumentException("Invalid nanos value (must be positive and less than 1 day): " + nanos2);
       }
       return Long.compare(nanos1, nanos2);
     }

parquet-column/src/test/java/org/apache/parquet/schema/TestPrimitiveComparator.java

@@ -39,6 +39,7 @@
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
 import java.util.List;
+import org.apache.parquet.TestUtils;
 import org.apache.parquet.example.data.simple.NanoTime;
 import org.apache.parquet.io.api.Binary;
 import org.junit.Test;
@@ -392,12 +393,10 @@ public void testInt96TimestampComparatorRejectsInvalidNanos() {
     Binary valid = int96(0, 0);
     for (long invalidNanos : new long[] {-1L, Long.MIN_VALUE, 86_400_000_000_001L, Long.MAX_VALUE}) {
       Binary invalid = int96(0, invalidNanos);
-      try {
-        BINARY_AS_INT96_TIMESTAMP_COMPARATOR.compare(valid, invalid);
-        fail("Expected IllegalArgumentException for nanos=" + invalidNanos);
-      } catch (IllegalArgumentException e) {
-        // expected
-      }
+      TestUtils.assertThrows(
+          "Expected IllegalArgumentException for nanos=" + invalidNanos,
+          IllegalArgumentException.class,
+          () -> BINARY_AS_INT96_TIMESTAMP_COMPARATOR.compare(valid, invalid));
     }
   }
 

parquet-hadoop/src/test/java/org/apache/parquet/format/converter/TestParquetMetadataConverter.java

@@ -1183,8 +1183,8 @@ public void testSkippedV2Stats() {
         Types.optional(PrimitiveTypeName.INT96)
             .columnOrder(ColumnOrder.undefined())
             .named(""),
-        new NanoTime(2458850, 0L).toBinary(),
-        new NanoTime(2458881, 39_600_000_000_000L).toBinary()
+        new NanoTime(2458850, 0L),
+        new NanoTime(2458881, 39_600_000_000_000L)
     );
   }
 
@@ -1226,8 +1226,8 @@ public void testV2OnlyStats() {
         new BigInteger("9864675"));
     testV2OnlyStats(
       Types.optional(PrimitiveTypeName.INT96).named(""),
-      new NanoTime(2458850, 0L).toBinary(), // 2020-01-01
-      new NanoTime(2458881, 39_600_000_000_000L).toBinary()); // 2020-02-01T11:00
+      new NanoTime(2458850, 0L), // 2020-01-01
+      new NanoTime(2458881, 39_600_000_000_000L)); // 2020-02-01T11:00
   }
 
   private void testV2OnlyStats(PrimitiveType type, Object min, Object max) {
@@ -1266,8 +1266,7 @@ public void testV2StatsEqualMinMax() {
             .named(""),
         new BigInteger("-8752832"),
         new BigInteger("-8752832"));
-    Binary int96 = new NanoTime(2458850, 43_200_000_000_000L)
-      .toBinary(); // 2020-01-01T12:00:00
+    NanoTime int96 = new NanoTime(2458850, 43_200_000_000_000L); // 2020-01-01T12:00:00
     testV2StatsEqualMinMax(
         Types.optional(PrimitiveTypeName.INT96).named(""),
         int96,
@@ -1291,8 +1290,8 @@ private static <T> Statistics<?> createStats(PrimitiveType type, T min, T max) {
       return createStatsTyped(type, (Long) min, (Long) max);
     } else if (c == BigInteger.class) {
       return createStatsTyped(type, (BigInteger) min, (BigInteger) max);
-    } else if (min instanceof Binary) {
-      return createStatsTyped(type, (Binary) min, (Binary) max);
+    } else if (min instanceof NanoTime) {
+      return createStatsTyped(type, (NanoTime) min, (NanoTime) max);
     }
     fail("Not implemented");
     return null;
@@ -1327,12 +1326,14 @@ private static Statistics<?> createStatsTyped(PrimitiveType type, BigInteger min
     return stats;
   }
 
-  private static Statistics<?> createStatsTyped(PrimitiveType type, Binary min, Binary max) {
+  private static Statistics<?> createStatsTyped(PrimitiveType type, NanoTime min, NanoTime max) {
+    Binary minBinary = min.toBinary();
+    Binary maxBinary = max.toBinary();
     Statistics<?> stats = Statistics.createStats(type);
-    stats.updateStats(max);
-    stats.updateStats(min);
-    assertEquals(min, stats.genericGetMin());
-    assertEquals(max, stats.genericGetMax());
+    stats.updateStats(maxBinary);
+    stats.updateStats(minBinary);
+    assertEquals(minBinary, stats.genericGetMin());
+    assertEquals(maxBinary, stats.genericGetMax());
     return stats;
   }
 
@@ -1438,8 +1439,19 @@ public void testColumnOrders() throws IOException {
   }
 
   @Test
-  public void testLegacyINT96ColumnOrder() throws IOException {
-    // Create a footer for a single INT96 column schema that does not specify a column order.
+  public void testUndefinedINT96ColumnOrder() throws IOException {
+    assertInt96ColumnOrderIgnored(null);
+  }
+
+  @Test
+  public void testTypeDefinedOrderINT96ColumnOrder() throws IOException {
+    org.apache.parquet.format.ColumnOrder typeOrder = new org.apache.parquet.format.ColumnOrder();
+    typeOrder.setTYPE_ORDER(new org.apache.parquet.format.TypeDefinedOrder());
+    assertInt96ColumnOrderIgnored(typeOrder);
+  }
+
+  private void assertInt96ColumnOrderIgnored(org.apache.parquet.format.ColumnOrder footerColumnOrder)
+      throws IOException {
     MessageType schema = parseMessageType("message test {"
         + "  optional int96 int96_col;"
         + "}");
@@ -1449,8 +1461,12 @@ public void testLegacyINT96ColumnOrder() throws IOException {
     ParquetMetadataConverter converter = new ParquetMetadataConverter();
     FileMetaData formatMetadata = converter.toParquetMetadata(1, metadata);
 
-    formatMetadata.unsetColumn_orders();
-    assertFalse(formatMetadata.isSetColumn_orders());
+    if (footerColumnOrder == null) {
+      formatMetadata.unsetColumn_orders();
+      assertFalse(formatMetadata.isSetColumn_orders());
+    } else {
+      formatMetadata.setColumn_orders(Collections.singletonList(footerColumnOrder));
+    }
 
     MessageType resultSchema =
         converter.fromParquetMetadata(formatMetadata).getFileMetaData().getSchema();