Address code review comments

parquet-arrow/src/main/java/org/apache/parquet/arrow/schema/SchemaConverter.java

@@ -242,11 +242,10 @@ public TypeMapping visit(Date type) {
       public TypeMapping visit(Time type) {
         int bitWidth = type.getBitWidth();
         TimeUnit timeUnit = type.getUnit();
-        // TODO: what is Arrow time semantic? UTC adjusted or not?
         if (bitWidth == 32 && timeUnit == TimeUnit.MILLISECOND) {
-          return primitive(INT32, timeType(true, MILLIS));
+          return primitive(INT32, timeType(false, MILLIS));
         } else if (bitWidth == 64 && timeUnit == TimeUnit.MICROSECOND) {
-          return primitive(INT64, timeType(true, MICROS));
+          return primitive(INT64, timeType(false, MICROS));
         }
         throw new UnsupportedOperationException("Unsupported type " + type);
       }

parquet-arrow/src/test/java/org/apache/parquet/arrow/schema/TestSchemaConverter.java

@@ -19,6 +19,9 @@
 package org.apache.parquet.arrow.schema;
 
 import static java.util.Arrays.asList;
+import static org.apache.parquet.schema.LogicalTypeAnnotation.TimeUnit.MICROS;
+import static org.apache.parquet.schema.LogicalTypeAnnotation.TimeUnit.MILLIS;
+import static org.apache.parquet.schema.LogicalTypeAnnotation.timeType;
 import static org.apache.parquet.schema.OriginalType.DATE;
 import static org.apache.parquet.schema.OriginalType.DECIMAL;
 import static org.apache.parquet.schema.OriginalType.INTERVAL;
@@ -168,7 +171,7 @@ private static Field field(String name, ArrowType type, Field... children) {
     .addField(Types.optional(INT64).as(DECIMAL).precision(15).scale(5).named("k1"))
     .addField(Types.optional(BINARY).as(DECIMAL).precision(25).scale(5).named("k2"))
     .addField(Types.optional(INT32).as(DATE).named("l"))
-    .addField(Types.optional(INT32).as(TIME_MILLIS).named("m"))
+    .addField(Types.optional(INT32).as(timeType(false, MILLIS)).named("m"))
     .addField(Types.optional(INT64).as(TIMESTAMP_MILLIS).named("n"))
     .addField(Types.optional(FIXED_LEN_BYTE_ARRAY).length(12).as(INTERVAL).named("o"))
     .addField(Types.optional(FIXED_LEN_BYTE_ARRAY).length(12).as(INTERVAL).named("o1"))
@@ -364,15 +367,17 @@ public void testArrowTimeMillisecondToParquet() {
     MessageType expected = converter.fromArrow(new Schema(asList(
       field("a", new ArrowType.Time(TimeUnit.MILLISECOND, 32))
     ))).getParquetSchema();
-    Assert.assertEquals(expected, Types.buildMessage().addField(Types.optional(INT32).as(TIME_MILLIS).named("a")).named("root"));
+    Assert.assertEquals(expected,
+      Types.buildMessage().addField(Types.optional(INT32).as(timeType(false, MILLIS)).named("a")).named("root"));
   }
 
   @Test
   public void testArrowTimeMicrosecondToParquet() {
     MessageType expected = converter.fromArrow(new Schema(asList(
       field("a", new ArrowType.Time(TimeUnit.MICROSECOND, 64))
     ))).getParquetSchema();
-    Assert.assertEquals(expected, Types.buildMessage().addField(Types.optional(INT64).as(TIME_MICROS).named("a")).named("root"));
+    Assert.assertEquals(expected,
+      Types.buildMessage().addField(Types.optional(INT64).as(timeType(false, MICROS)).named("a")).named("root"));
   }
 
   @Test(expected = UnsupportedOperationException.class)

parquet-avro/src/main/java/org/apache/parquet/avro/AvroSchemaConverter.java

@@ -409,13 +409,13 @@ private LogicalTypeAnnotation convertLogicalType(LogicalType logicalType) {
     } else if (logicalType instanceof LogicalTypes.Date) {
       return dateType();
     } else if (logicalType instanceof LogicalTypes.TimeMillis) {
-      return timeType(true, MILLIS);
+      return timeType(false, MILLIS);
     } else if (logicalType instanceof LogicalTypes.TimeMicros) {
-      return timeType(true, MICROS);
+      return timeType(false, MICROS);
     } else if (logicalType instanceof LogicalTypes.TimestampMillis) {
-      return timestampType(true, MILLIS);
+      return timestampType(false, MILLIS);
     } else if (logicalType instanceof LogicalTypes.TimestampMicros) {
-      return timestampType(true, MICROS);
+      return timestampType(false, MICROS);
     }
     return null;
   }
@@ -439,7 +439,6 @@ public Optional<LogicalType> visit(LogicalTypeAnnotation.DateLogicalTypeAnnotati
       public Optional<LogicalType> visit(LogicalTypeAnnotation.TimeLogicalTypeAnnotation logicalTypeAnnotation) {
         LogicalTypeAnnotation.TimeUnit unit = logicalTypeAnnotation.getUnit();
         switch (unit) {
-          // TODO: should we handle UTC parameter? Looks like Avro spec says all timestamps are in UTC normalized form
           case MILLIS:
             return of(LogicalTypes.timeMillis());
           case MICROS:
@@ -452,7 +451,6 @@ public Optional<LogicalType> visit(LogicalTypeAnnotation.TimeLogicalTypeAnnotati
       public Optional<LogicalType> visit(LogicalTypeAnnotation.TimestampLogicalTypeAnnotation logicalTypeAnnotation) {
         LogicalTypeAnnotation.TimeUnit unit = logicalTypeAnnotation.getUnit();
         switch (unit) {
-          // TODO: should we handle UTC parameter? Looks like Avro spec says all timestamps are in UTC normalized form
           case MILLIS:
             return of(LogicalTypes.timestampMillis());
           case MICROS:

parquet-avro/src/test/java/org/apache/parquet/avro/TestAvroSchemaConverter.java

@@ -1,14 +1,14 @@
-/* 
+/*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
  * distributed with this work for additional information
  * regarding copyright ownership.  The ASF licenses this file
  * to you under the Apache License, Version 2.0 (the
  * "License"); you may not use this file except in compliance
  * with the License.  You may obtain a copy of the License at
- * 
+ *
  *   http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing,
  * software distributed under the License is distributed on an
  * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
@@ -616,7 +616,7 @@ public void testTimeMillisType() throws Exception {
 
     testRoundTripConversion(expected,
         "message myrecord {\n" +
-            "  required int32 time (TIME_MILLIS);\n" +
+            "  required int32 time (TIME(MILLIS,false));\n" +
             "}\n");
 
     for (PrimitiveTypeName primitive : new PrimitiveTypeName[]
@@ -646,7 +646,7 @@ public void testTimeMicrosType() throws Exception {
 
     testRoundTripConversion(expected,
         "message myrecord {\n" +
-            "  required int64 time (TIME_MICROS);\n" +
+            "  required int64 time (TIME(MICROS,false));\n" +
             "}\n");
 
     for (PrimitiveTypeName primitive : new PrimitiveTypeName[]
@@ -676,7 +676,7 @@ public void testTimestampMillisType() throws Exception {
 
     testRoundTripConversion(expected,
         "message myrecord {\n" +
-            "  required int64 timestamp (TIMESTAMP_MILLIS);\n" +
+            "  required int64 timestamp (TIMESTAMP(MILLIS,false));\n" +
             "}\n");
 
     for (PrimitiveTypeName primitive : new PrimitiveTypeName[]
@@ -706,7 +706,7 @@ public void testTimestampMicrosType() throws Exception {
 
     testRoundTripConversion(expected,
         "message myrecord {\n" +
-            "  required int64 timestamp (TIMESTAMP_MICROS);\n" +
+            "  required int64 timestamp (TIMESTAMP(MICROS,false));\n" +
             "}\n");
 
     for (PrimitiveTypeName primitive : new PrimitiveTypeName[]

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

@@ -73,7 +73,7 @@ public GroupType(Repetition repetition, String name, OriginalType originalType,
    * @param logicalTypeAnnotation (optional) the logical type to help with cross schema conversion (LIST, MAP, ...)
    * @param fields the contained fields
    */
-  public GroupType(Repetition repetition, String name, LogicalTypeAnnotation logicalTypeAnnotation, Type... fields) {
+  GroupType(Repetition repetition, String name, LogicalTypeAnnotation logicalTypeAnnotation, Type... fields) {
     this(repetition, name, logicalTypeAnnotation, Arrays.asList(fields));
   }
 

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

@@ -150,7 +150,7 @@ String typeParametersAsString() {
     return "";
   }
 
-  public boolean isValidColumnOrder(ColumnOrder columnOrder) {
+  boolean isValidColumnOrder(ColumnOrder columnOrder) {
     return columnOrder.getColumnOrderName() == UNDEFINED || columnOrder.getColumnOrderName() == TYPE_DEFINED_ORDER;
   }
 
@@ -162,7 +162,7 @@ public String toString() {
     return sb.toString();
   }
 
-  public PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
+  PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
     throw new UnsupportedOperationException("Stringifier is not supported for the logical type: " + this);
   }
 
@@ -306,7 +306,7 @@ public int hashCode() {
     }
 
     @Override
-    public PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
+    PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
       return PrimitiveStringifier.UTF8_STRINGIFIER;
     }
   }
@@ -410,7 +410,7 @@ public int hashCode() {
     }
 
     @Override
-    public PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
+    PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
       return PrimitiveStringifier.UTF8_STRINGIFIER;
     }
   }
@@ -475,7 +475,7 @@ public int hashCode() {
     }
 
     @Override
-    public PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
+    PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
       return stringifier;
     }
   }
@@ -513,7 +513,7 @@ public int hashCode() {
     }
 
     @Override
-    public PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
+    PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
       return PrimitiveStringifier.DATE_STRINGIFIER;
     }
   }
@@ -588,7 +588,7 @@ public int hashCode() {
     }
 
     @Override
-    public PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
+    PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
       return PrimitiveStringifier.TIME_STRINGIFIER;
     }
   }
@@ -658,7 +658,7 @@ public int hashCode() {
     }
 
     @Override
-    public PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
+    PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
       switch (unit) {
         case MICROS:
           return PrimitiveStringifier.TIMESTAMP_MICROS_STRINGIFIER;
@@ -745,7 +745,7 @@ public int hashCode() {
     }
 
     @Override
-    public PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
+    PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
       return isSigned ? PrimitiveStringifier.DEFAULT_STRINGIFIER : PrimitiveStringifier.UNSIGNED_STRINGIFIER;
     }
   }
@@ -783,7 +783,7 @@ public int hashCode() {
     }
 
     @Override
-    public PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
+    PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
       return PrimitiveStringifier.UTF8_STRINGIFIER;
     }
   }
@@ -821,7 +821,7 @@ public int hashCode() {
     }
 
     @Override
-    public PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
+    PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
       return PrimitiveStringifier.DEFAULT_STRINGIFIER;
     }
   }
@@ -866,12 +866,12 @@ public int hashCode() {
     }
 
     @Override
-    public PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
+    PrimitiveStringifier valueStringifier(PrimitiveType primitiveType) {
       return PrimitiveStringifier.INTERVAL_STRINGIFIER;
     }
 
     @Override
-    public boolean isValidColumnOrder(ColumnOrder columnOrder) {
+    boolean isValidColumnOrder(ColumnOrder columnOrder) {
       return columnOrder.getColumnOrderName() == UNDEFINED;
     }
   }