Datetime fixes for regression (apache#36)

* Add toString to Time obj in Time#toString

* Improve Time toString

* Fix maven plugins

* Revert "Update java/flight/flight-jdbc-driver/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java"

This reverts commit 00808c0.

* Revert "Merge pull request apache#29 from rafael-telles/Timestamp_fix"

This reverts commit 7924e7b, reversing
changes made to f6ac593.

* Fix DateTime for negative epoch

* Remove unwanted change

* Fix negative timestamp shift

* Fix coverage

* Refator DateTimeUtilsTest

Author: vfraga

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightJdbcTime.java

@@ -0,0 +1,106 @@
+/*
+ * 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 KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.driver.jdbc;
+
+import static org.apache.calcite.avatica.util.DateTimeUtils.MILLIS_PER_DAY;
+
+import java.sql.Time;
+import java.time.LocalTime;
+import java.time.temporal.ChronoField;
+import java.util.List;
+import java.util.Objects;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.arrow.util.VisibleForTesting;
+
+import com.google.common.collect.ImmutableList;
+
+/**
+ * Wrapper class for Time objects to include the milliseconds part in ISO 8601 format in this#toString.
+ */
+public class ArrowFlightJdbcTime extends Time {
+  private static final List<String> LEADING_ZEROES = ImmutableList.of("", "0", "00");
+
+  // Desired length of the millisecond portion should be 3
+  private static final int DESIRED_MILLIS_LENGTH = 3;
+
+  // Millis of the date time object.
+  private final int millisReprValue;
+
+  /**
+   * Constructs this object based on epoch millis.
+   *
+   * @param milliseconds milliseconds representing Time.
+   */
+  public ArrowFlightJdbcTime(final long milliseconds) {
+    super(milliseconds);
+    millisReprValue = getMillisReprValue(milliseconds);
+  }
+
+  @VisibleForTesting
+  ArrowFlightJdbcTime(final LocalTime time) {
+    // Although the constructor is deprecated, this is the exact same code as Time#valueOf(LocalTime)
+    super(time.getHour(), time.getMinute(), time.getSecond());
+    millisReprValue = time.get(ChronoField.MILLI_OF_SECOND);
+  }
+
+  private int getMillisReprValue(long milliseconds) {
+    // Extract the millisecond part from epoch nano day
+    if (milliseconds > MILLIS_PER_DAY) {
+      // Convert to Epoch Day
+      milliseconds %= MILLIS_PER_DAY;
+    } else if (milliseconds < 0) {
+      // LocalTime#ofNanoDay only accepts positive values
+      milliseconds -= ((milliseconds / MILLIS_PER_DAY) - 1) * MILLIS_PER_DAY;
+    }
+    return LocalTime.ofNanoOfDay(TimeUnit.MILLISECONDS.toNanos(milliseconds))
+        .get(ChronoField.MILLI_OF_SECOND);
+  }
+
+  @Override
+  public String toString() {
+    final StringBuilder time = new StringBuilder().append(super.toString());
+
+    if (millisReprValue > 0) {
+      final String millisString = Integer.toString(millisReprValue);
+
+      // dot to separate the fractional seconds
+      time.append(".");
+
+      final int millisLength = millisString.length();
+      if (millisLength < DESIRED_MILLIS_LENGTH) {
+        // add necessary leading zeroes
+        time.append(LEADING_ZEROES.get(DESIRED_MILLIS_LENGTH - millisLength));
+      }
+      time.append(millisString);
+    }
+
+    return time.toString();
+  }
+
+  // Spotbugs requires these methods to be overridden
+  @Override
+  public boolean equals(Object obj) {
+    return super.equals(obj);
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(super.hashCode(), this.millisReprValue);
+  }
+}

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java

@@ -20,6 +20,7 @@
 import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcDateVectorGetter.Getter;
 import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcDateVectorGetter.Holder;
 import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcDateVectorGetter.createGetter;
+import static org.apache.arrow.driver.jdbc.utils.DateTimeUtils.getTimestampValue;
 import static org.apache.calcite.avatica.util.DateTimeUtils.MILLIS_PER_DAY;
 import static org.apache.calcite.avatica.util.DateTimeUtils.unixDateToString;
 
@@ -94,7 +95,9 @@ public Date getDate(Calendar calendar) {
     long value = holder.value;
     long milliseconds = this.timeUnit.toMillis(value);
 
-    return new Date(DateTimeUtils.applyCalendarOffset(milliseconds, calendar));
+    long millisWithCalendar = DateTimeUtils.applyCalendarOffset(milliseconds, calendar);
+
+    return new Date(getTimestampValue(millisWithCalendar).getTime());
   }
 
   private void fillHolder() {

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java

@@ -24,6 +24,8 @@
 import java.sql.Date;
 import java.sql.Time;
 import java.sql.Timestamp;
+import java.time.LocalDateTime;
+import java.time.temporal.ChronoUnit;
 import java.util.Calendar;
 import java.util.TimeZone;
 import java.util.concurrent.TimeUnit;
@@ -33,6 +35,7 @@
 import org.apache.arrow.driver.jdbc.accessor.ArrowFlightJdbcAccessorFactory;
 import org.apache.arrow.vector.TimeStampVector;
 import org.apache.arrow.vector.types.pojo.ArrowType;
+import org.apache.arrow.vector.util.DateUtility;
 
 /**
  * Accessor for the Arrow types extending from {@link TimeStampVector}.
@@ -42,8 +45,16 @@ public class ArrowFlightJdbcTimeStampVectorAccessor extends ArrowFlightJdbcAcces
   private final TimeZone timeZone;
   private final Getter getter;
   private final TimeUnit timeUnit;
+  private final LongToLocalDateTime longToLocalDateTime;
   private final Holder holder;
 
+  /**
+   * Functional interface used to convert a number (in any time resolution) to LocalDateTime.
+   */
+  interface LongToLocalDateTime {
+    LocalDateTime fromLong(long value);
+  }
+
   /**
    * Instantiate a ArrowFlightJdbcTimeStampVectorAccessor for given vector.
    */
@@ -56,6 +67,7 @@ public ArrowFlightJdbcTimeStampVectorAccessor(TimeStampVector vector,
 
     this.timeZone = getTimeZoneForVector(vector);
     this.timeUnit = getTimeUnitForVector(vector);
+    this.longToLocalDateTime = getLongToLocalDateTimeForVector(vector, this.timeZone);
   }
 
   @Override
@@ -68,56 +80,55 @@ public Object getObject() {
     return this.getTimestamp(null);
   }
 
-  private Long getLocalDateTimeMillis() {
+  private LocalDateTime getLocalDateTime(Calendar calendar) {
     getter.get(getCurrentRow(), holder);
     this.wasNull = holder.isSet == 0;
     this.wasNullConsumer.setWasNull(this.wasNull);
     if (this.wasNull) {
       return null;
     }
 
-    final long value = holder.value;
-    final long millis = this.timeUnit.toMillis(value);
+    long value = holder.value;
+
+    LocalDateTime localDateTime = this.longToLocalDateTime.fromLong(value);
 
-    return millis + this.timeZone.getOffset(millis);
+    if (calendar != null) {
+      TimeZone timeZone = calendar.getTimeZone();
+      long millis = this.timeUnit.toMillis(value);
+      localDateTime = localDateTime
+          .minus(timeZone.getOffset(millis) - this.timeZone.getOffset(millis), ChronoUnit.MILLIS);
+    }
+    return localDateTime;
   }
 
   @Override
   public Date getDate(Calendar calendar) {
-    Long millis = getLocalDateTimeMillis();
-    if (millis == null) {
+    LocalDateTime localDateTime = getLocalDateTime(calendar);
+    if (localDateTime == null) {
       return null;
     }
 
-    return new Date(applyCalendarOffset(calendar, millis));
+    return new Date(Timestamp.valueOf(localDateTime).getTime());
   }
 
   @Override
   public Time getTime(Calendar calendar) {
-    Long millis = getLocalDateTimeMillis();
-    if (millis == null) {
+    LocalDateTime localDateTime = getLocalDateTime(calendar);
+    if (localDateTime == null) {
       return null;
     }
 
-    return new Time(applyCalendarOffset(calendar, millis));
+    return new Time(Timestamp.valueOf(localDateTime).getTime());
   }
 
   @Override
   public Timestamp getTimestamp(Calendar calendar) {
-    Long millis = getLocalDateTimeMillis();
-    if (millis == null) {
+    LocalDateTime localDateTime = getLocalDateTime(calendar);
+    if (localDateTime == null) {
       return null;
     }
 
-    return new Timestamp(applyCalendarOffset(calendar, millis));
-  }
-
-  private long applyCalendarOffset(final Calendar calendar, final long millis) {
-    if (calendar == null) {
-      return millis - Calendar.getInstance(TimeZone.getDefault()).getTimeZone().getOffset(millis);
-    }
-
-    return millis - calendar.getTimeZone().getOffset(millis) + this.timeZone.getOffset(millis);
+    return Timestamp.valueOf(localDateTime);
   }
 
   protected static TimeUnit getTimeUnitForVector(TimeStampVector vector) {
@@ -138,6 +149,28 @@ protected static TimeUnit getTimeUnitForVector(TimeStampVector vector) {
     }
   }
 
+  protected static LongToLocalDateTime getLongToLocalDateTimeForVector(TimeStampVector vector,
+                                                                       TimeZone timeZone) {
+    String timeZoneID = timeZone.getID();
+
+    ArrowType.Timestamp arrowType =
+        (ArrowType.Timestamp) vector.getField().getFieldType().getType();
+
+    switch (arrowType.getUnit()) {
+      case NANOSECOND:
+        return nanoseconds -> DateUtility.getLocalDateTimeFromEpochNano(nanoseconds, timeZoneID);
+      case MICROSECOND:
+        return microseconds -> DateUtility.getLocalDateTimeFromEpochMicro(microseconds, timeZoneID);
+      case MILLISECOND:
+        return milliseconds -> DateUtility.getLocalDateTimeFromEpochMilli(milliseconds, timeZoneID);
+      case SECOND:
+        return seconds -> DateUtility.getLocalDateTimeFromEpochMilli(
+            TimeUnit.SECONDS.toMillis(seconds), timeZoneID);
+      default:
+        throw new UnsupportedOperationException("Invalid Arrow time unit");
+    }
+  }
+
   protected static TimeZone getTimeZoneForVector(TimeStampVector vector) {
     ArrowType.Timestamp arrowType =
         (ArrowType.Timestamp) vector.getField().getFieldType().getType();

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorAccessor.java

@@ -20,15 +20,14 @@
 import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcTimeVectorGetter.Getter;
 import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcTimeVectorGetter.Holder;
 import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcTimeVectorGetter.createGetter;
-import static org.apache.calcite.avatica.util.DateTimeUtils.MILLIS_PER_DAY;
-import static org.apache.calcite.avatica.util.DateTimeUtils.unixTimeToString;
 
 import java.sql.Time;
 import java.sql.Timestamp;
 import java.util.Calendar;
 import java.util.concurrent.TimeUnit;
 import java.util.function.IntSupplier;
 
+import org.apache.arrow.driver.jdbc.ArrowFlightJdbcTime;
 import org.apache.arrow.driver.jdbc.accessor.ArrowFlightJdbcAccessor;
 import org.apache.arrow.driver.jdbc.accessor.ArrowFlightJdbcAccessorFactory;
 import org.apache.arrow.driver.jdbc.utils.DateTimeUtils;
@@ -126,7 +125,7 @@ public Time getTime(Calendar calendar) {
     long value = holder.value;
     long milliseconds = this.timeUnit.toMillis(value);
 
-    return new Time(DateTimeUtils.applyCalendarOffset(milliseconds, calendar));
+    return new ArrowFlightJdbcTime(DateTimeUtils.applyCalendarOffset(milliseconds, calendar));
   }
 
   private void fillHolder() {
@@ -144,16 +143,6 @@ public Timestamp getTimestamp(Calendar calendar) {
     return new Timestamp(time.getTime());
   }
 
-  @Override
-  public String getString() {
-    fillHolder();
-    if (wasNull) {
-      return null;
-    }
-    long milliseconds = timeUnit.toMillis(holder.value);
-    return unixTimeToString((int) (milliseconds % MILLIS_PER_DAY));
-  }
-
   protected static TimeUnit getTimeUnitForVector(ValueVector vector) {
     if (vector instanceof TimeNanoVector) {
       return TimeUnit.NANOSECONDS;

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java

@@ -17,8 +17,15 @@
 
 package org.apache.arrow.driver.jdbc.utils;
 
+import static org.apache.calcite.avatica.util.DateTimeUtils.MILLIS_PER_DAY;
+
+import java.sql.Timestamp;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.LocalTime;
 import java.util.Calendar;
 import java.util.TimeZone;
+import java.util.concurrent.TimeUnit;
 
 /**
  * Datetime utility functions.
@@ -33,8 +40,37 @@ private DateTimeUtils() {
    */
   public static long applyCalendarOffset(long milliseconds, Calendar calendar) {
     if (calendar == null) {
-      return milliseconds - Calendar.getInstance(TimeZone.getDefault()).getTimeZone().getOffset(milliseconds);
+      calendar = Calendar.getInstance();
+    }
+
+    final TimeZone tz = calendar.getTimeZone();
+    final TimeZone defaultTz = TimeZone.getDefault();
+
+    if (tz != defaultTz) {
+      milliseconds -= tz.getOffset(milliseconds) - defaultTz.getOffset(milliseconds);
+    }
+
+    return milliseconds;
+  }
+
+
+  /**
+   * Converts Epoch millis to a {@link Timestamp} object.
+   *
+   * @param millisWithCalendar the Timestamp in Epoch millis
+   * @return a {@link Timestamp} object representing the given Epoch millis
+   */
+  public static Timestamp getTimestampValue(long millisWithCalendar) {
+    long milliseconds = millisWithCalendar;
+    if (milliseconds < 0) {
+      // LocalTime#ofNanoDay only accepts positive values
+      milliseconds -= ((milliseconds / MILLIS_PER_DAY) - 1) * MILLIS_PER_DAY;
     }
-    return milliseconds - calendar.getTimeZone().getOffset(milliseconds);
+
+    return Timestamp.valueOf(
+        LocalDateTime.of(
+            LocalDate.ofEpochDay(millisWithCalendar / MILLIS_PER_DAY),
+            LocalTime.ofNanoOfDay(TimeUnit.MILLISECONDS.toNanos(milliseconds % MILLIS_PER_DAY)))
+    );
   }
 }