From 9d02bfd6ae80137a657bdc080a4e64d02e230518 Mon Sep 17 00:00:00 2001 From: Jay Ho Date: Wed, 5 Jul 2023 15:18:06 -0700 Subject: [PATCH 1/6] Updated all time related logic to be in line with JDBC API - Old behaviour treats the calendar object passed into the corresponding methods to be the timezone to convert the data into, which is incorrect according to what is defined in the JDBC API - Fixed: Correct behaviour is that the calendar object is used to extend the data into the timezone of the calendar object, essentially asserting that the data is at the timezone defined in the calendar object - Fixed: for vectors that do store timezone information (ie. TimestampVector), the getter methods will use the timezone defined in vector as the timezone assertion and ignores the calendar object if one was passed in --- ...rrowFlightJdbcTimeStampVectorAccessor.java | 23 +++++------ .../driver/jdbc/utils/DateTimeUtils.java | 16 ++++---- ...FlightJdbcTimeStampVectorAccessorTest.java | 40 +++++++++++++++---- .../driver/jdbc/utils/DateTimeUtilsTest.java | 4 +- 4 files changed, 52 insertions(+), 31 deletions(-) diff --git a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java index a23883baf1e..563e93b50ad 100644 --- a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java +++ b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java @@ -25,7 +25,8 @@ import java.sql.Time; import java.sql.Timestamp; import java.time.LocalDateTime; -import java.time.temporal.ChronoUnit; +import java.time.ZoneId; +import java.time.ZonedDateTime; import java.util.Calendar; import java.util.TimeZone; import java.util.concurrent.TimeUnit; @@ -67,7 +68,7 @@ public ArrowFlightJdbcTimeStampVectorAccessor(TimeStampVector vector, this.timeZone = getTimeZoneForVector(vector); this.timeUnit = getTimeUnitForVector(vector); - this.longToLocalDateTime = getLongToLocalDateTimeForVector(vector, this.timeZone); + this.longToLocalDateTime = getLongToUTCDateTimeForVector(vector); } @Override @@ -92,12 +93,11 @@ private LocalDateTime getLocalDateTime(Calendar calendar) { LocalDateTime localDateTime = this.longToLocalDateTime.fromLong(value); - 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); - } + ZoneId sourceTimeZone = this.timeZone != null ? this.timeZone.toZoneId() : + calendar == null ? TimeZone.getDefault().toZoneId() : calendar.getTimeZone().toZoneId(); + ZonedDateTime sourceTZDateTime = localDateTime.atZone(sourceTimeZone); + localDateTime = sourceTZDateTime.withZoneSameInstant(TimeZone.getDefault().toZoneId()).toLocalDateTime(); + return localDateTime; } @@ -149,9 +149,8 @@ protected static TimeUnit getTimeUnitForVector(TimeStampVector vector) { } } - protected static LongToLocalDateTime getLongToLocalDateTimeForVector(TimeStampVector vector, - TimeZone timeZone) { - String timeZoneID = timeZone.getID(); + protected static LongToLocalDateTime getLongToUTCDateTimeForVector(TimeStampVector vector) { + String timeZoneID = "UTC"; ArrowType.Timestamp arrowType = (ArrowType.Timestamp) vector.getField().getFieldType().getType(); @@ -177,7 +176,7 @@ protected static TimeZone getTimeZoneForVector(TimeStampVector vector) { String timezoneName = arrowType.getTimezone(); if (timezoneName == null) { - return TimeZone.getTimeZone("UTC"); + return null; } return TimeZone.getTimeZone(timezoneName); diff --git a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java index dd94a09256d..a8748e3979d 100644 --- a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java +++ b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java @@ -20,9 +20,11 @@ import static org.apache.calcite.avatica.util.DateTimeUtils.MILLIS_PER_DAY; import java.sql.Timestamp; +import java.time.Instant; import java.time.LocalDate; import java.time.LocalDateTime; import java.time.LocalTime; +import java.time.ZonedDateTime; import java.util.Calendar; import java.util.TimeZone; import java.util.concurrent.TimeUnit; @@ -42,15 +44,11 @@ public static long applyCalendarOffset(long milliseconds, Calendar calendar) { if (calendar == null) { 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; + Instant currInstant = Instant.ofEpochMilli(milliseconds); + LocalDateTime getTimestampWithoutTZ = LocalDateTime.ofInstant(currInstant, + TimeZone.getTimeZone("UTC").toZoneId()); + ZonedDateTime parsedTime = getTimestampWithoutTZ.atZone(calendar.getTimeZone().toZoneId()); + return parsedTime.toEpochSecond() * 1000; } diff --git a/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java b/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java index 38d842724b9..456d448a17d 100644 --- a/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java +++ b/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java @@ -25,7 +25,9 @@ import java.sql.Date; import java.sql.Time; import java.sql.Timestamp; +import java.time.Instant; import java.time.LocalDateTime; +import java.time.ZonedDateTime; import java.util.Arrays; import java.util.Calendar; import java.util.Collection; @@ -174,13 +176,18 @@ public void testShouldGetTimestampReturnValidTimestampWithCalendar() throws Exce Calendar calendar = Calendar.getInstance(timeZone); TimeZone timeZoneForVector = getTimeZoneForVector(vector); + timeZoneForVector = timeZoneForVector == null ? TimeZone.getDefault() : timeZoneForVector; + + TimeZone finalTimeZoneForResultWithoutCalendar = timeZoneForVector; accessorIterator.iterate(vector, (accessor, currentRow) -> { final Timestamp resultWithoutCalendar = accessor.getTimestamp(null); final Timestamp result = accessor.getTimestamp(calendar); + final TimeZone timeZoneForResult = getTimeZoneForVector(vector) == null ? timeZone : + finalTimeZoneForResultWithoutCalendar; - long offset = timeZone.getOffset(resultWithoutCalendar.getTime()) - - timeZoneForVector.getOffset(resultWithoutCalendar.getTime()); + long offset = timeZoneForResult.getOffset(result.getTime()) - + finalTimeZoneForResultWithoutCalendar.getOffset(resultWithoutCalendar.getTime()); collector.checkThat(resultWithoutCalendar.getTime() - result.getTime(), is(offset)); collector.checkThat(accessor.wasNull(), is(false)); @@ -207,13 +214,19 @@ public void testShouldGetDateReturnValidDateWithCalendar() throws Exception { Calendar calendar = Calendar.getInstance(timeZone); TimeZone timeZoneForVector = getTimeZoneForVector(vector); + timeZoneForVector = timeZoneForVector == null ? TimeZone.getDefault() : timeZoneForVector; + + TimeZone finalTimeZoneForResultWithoutCalendar = timeZoneForVector; accessorIterator.iterate(vector, (accessor, currentRow) -> { final Date resultWithoutCalendar = accessor.getDate(null); final Date result = accessor.getDate(calendar); - long offset = timeZone.getOffset(resultWithoutCalendar.getTime()) - - timeZoneForVector.getOffset(resultWithoutCalendar.getTime()); + final TimeZone timeZoneForResult = getTimeZoneForVector(vector) == null ? timeZone : + finalTimeZoneForResultWithoutCalendar; + + long offset = timeZoneForResult.getOffset(result.getTime()) - + finalTimeZoneForResultWithoutCalendar.getOffset(resultWithoutCalendar.getTime()); collector.checkThat(resultWithoutCalendar.getTime() - result.getTime(), is(offset)); collector.checkThat(accessor.wasNull(), is(false)); @@ -240,13 +253,19 @@ public void testShouldGetTimeReturnValidTimeWithCalendar() throws Exception { Calendar calendar = Calendar.getInstance(timeZone); TimeZone timeZoneForVector = getTimeZoneForVector(vector); + timeZoneForVector = timeZoneForVector == null ? TimeZone.getDefault() : timeZoneForVector; + + TimeZone finalTimeZoneForResultWithoutCalendar = timeZoneForVector; accessorIterator.iterate(vector, (accessor, currentRow) -> { final Time resultWithoutCalendar = accessor.getTime(null); final Time result = accessor.getTime(calendar); - long offset = timeZone.getOffset(resultWithoutCalendar.getTime()) - - timeZoneForVector.getOffset(resultWithoutCalendar.getTime()); + final TimeZone timeZoneForResult = getTimeZoneForVector(vector) == null ? timeZone : + finalTimeZoneForResultWithoutCalendar; + + long offset = timeZoneForResult.getOffset(result.getTime()) - + finalTimeZoneForResultWithoutCalendar.getOffset(resultWithoutCalendar.getTime()); collector.checkThat(resultWithoutCalendar.getTime() - result.getTime(), is(offset)); collector.checkThat(accessor.wasNull(), is(false)); @@ -270,8 +289,13 @@ private Timestamp getTimestampForVector(int currentRow) { } else if (object instanceof Long) { TimeUnit timeUnit = getTimeUnitForVector(vector); long millis = timeUnit.toMillis((Long) object); - long offset = TimeZone.getTimeZone(timeZone).getOffset(millis); - expectedTimestamp = new Timestamp(millis + offset); + + Instant currInstant = Instant.ofEpochMilli(millis); + LocalDateTime getTimestampWithoutTZ = LocalDateTime.ofInstant(currInstant, + TimeZone.getTimeZone("UTC").toZoneId()); + + ZonedDateTime sourceTZDateTime = getTimestampWithoutTZ.atZone(TimeZone.getTimeZone(timeZone).toZoneId()); + expectedTimestamp = new Timestamp(sourceTZDateTime.toEpochSecond() * 1000); } return expectedTimestamp; } diff --git a/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtilsTest.java b/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtilsTest.java index adb892fcdc7..15e8fa2645e 100644 --- a/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtilsTest.java +++ b/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtilsTest.java @@ -67,9 +67,9 @@ public void testShouldGetOffsetWithDifferentTimeZone() { TimeZone.setDefault(alternateTimezone); try { // Trying to guarantee timezone returns to its original value - final long expectedEpochMillis = epochMillis + offset; + final long expectedEpochMillis = epochMillis - offset; final long actualEpochMillis = DateTimeUtils.applyCalendarOffset(epochMillis, Calendar.getInstance( - defaultTimezone)); + alternateTimezone)); collector.checkThat(actualEpochMillis, is(expectedEpochMillis)); } finally { From 77e037b817fa14c05d36703c0e34ddce174e53fe Mon Sep 17 00:00:00 2001 From: Jay Ho Date: Wed, 5 Jul 2023 16:27:42 -0700 Subject: [PATCH 2/6] Modified comments --- .../java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java index a8748e3979d..b4da2b3cc1b 100644 --- a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java +++ b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java @@ -38,7 +38,7 @@ private DateTimeUtils() { } /** - * Subtracts given Calendar's TimeZone offset from epoch milliseconds. + * Apply calendar timezone to epoch milliseconds */ public static long applyCalendarOffset(long milliseconds, Calendar calendar) { if (calendar == null) { From 588743330ff5bff1544de42457d7e2aaa5144cdb Mon Sep 17 00:00:00 2001 From: Jay Date: Thu, 6 Jul 2023 09:43:40 -0700 Subject: [PATCH 3/6] - Refactored duplicate code in ArrowFlightJdbcTimeStampVectorAccessorTest - Made timezone converting logic more readable in ArrowFlightJdbcTimeStampVectorAccessor - Fixed checkstyle issues --- ...rrowFlightJdbcTimeStampVectorAccessor.java | 12 ++-- .../driver/jdbc/utils/DateTimeUtils.java | 2 +- ...FlightJdbcTimeStampVectorAccessorTest.java | 60 ++++++++----------- 3 files changed, 31 insertions(+), 43 deletions(-) diff --git a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java index 563e93b50ad..22f18464306 100644 --- a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java +++ b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java @@ -17,6 +17,7 @@ package org.apache.arrow.driver.jdbc.accessor.impl.calendar; +import static java.util.Objects.nonNull; import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcTimeStampVectorGetter.Getter; import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcTimeStampVectorGetter.Holder; import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcTimeStampVectorGetter.createGetter; @@ -26,7 +27,6 @@ import java.sql.Timestamp; import java.time.LocalDateTime; import java.time.ZoneId; -import java.time.ZonedDateTime; import java.util.Calendar; import java.util.TimeZone; import java.util.concurrent.TimeUnit; @@ -92,13 +92,11 @@ private LocalDateTime getLocalDateTime(Calendar calendar) { long value = holder.value; LocalDateTime localDateTime = this.longToLocalDateTime.fromLong(value); + ZoneId defaultTimeZone = TimeZone.getDefault().toZoneId(); + ZoneId sourceTimeZone = nonNull(this.timeZone) ? this.timeZone.toZoneId() : + nonNull(calendar) ? calendar.getTimeZone().toZoneId() : defaultTimeZone; - ZoneId sourceTimeZone = this.timeZone != null ? this.timeZone.toZoneId() : - calendar == null ? TimeZone.getDefault().toZoneId() : calendar.getTimeZone().toZoneId(); - ZonedDateTime sourceTZDateTime = localDateTime.atZone(sourceTimeZone); - localDateTime = sourceTZDateTime.withZoneSameInstant(TimeZone.getDefault().toZoneId()).toLocalDateTime(); - - return localDateTime; + return localDateTime.atZone(sourceTimeZone).withZoneSameInstant(defaultTimeZone).toLocalDateTime(); } @Override diff --git a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java index b4da2b3cc1b..c2f19610a79 100644 --- a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java +++ b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java @@ -38,7 +38,7 @@ private DateTimeUtils() { } /** - * Apply calendar timezone to epoch milliseconds + * Apply calendar timezone to epoch milliseconds. */ public static long applyCalendarOffset(long milliseconds, Calendar calendar) { if (calendar == null) { diff --git a/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java b/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java index 456d448a17d..b1d2bfb7116 100644 --- a/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java +++ b/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java @@ -17,6 +17,7 @@ package org.apache.arrow.driver.jdbc.accessor.impl.calendar; +import static java.util.Optional.ofNullable; import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcTimeStampVectorAccessor.getTimeUnitForVector; import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcTimeStampVectorAccessor.getTimeZoneForVector; import static org.hamcrest.CoreMatchers.equalTo; @@ -175,22 +176,15 @@ public void testShouldGetTimestampReturnValidTimestampWithCalendar() throws Exce TimeZone timeZone = TimeZone.getTimeZone(AMERICA_SAO_PAULO); Calendar calendar = Calendar.getInstance(timeZone); - TimeZone timeZoneForVector = getTimeZoneForVector(vector); - timeZoneForVector = timeZoneForVector == null ? TimeZone.getDefault() : timeZoneForVector; - - TimeZone finalTimeZoneForResultWithoutCalendar = timeZoneForVector; + TimeZone finalTimeZoneForResultWithoutCalendar = ofNullable(getTimeZoneForVector(vector)) + .orElse(TimeZone.getDefault()); accessorIterator.iterate(vector, (accessor, currentRow) -> { final Timestamp resultWithoutCalendar = accessor.getTimestamp(null); final Timestamp result = accessor.getTimestamp(calendar); - final TimeZone timeZoneForResult = getTimeZoneForVector(vector) == null ? timeZone : - finalTimeZoneForResultWithoutCalendar; - - long offset = timeZoneForResult.getOffset(result.getTime()) - - finalTimeZoneForResultWithoutCalendar.getOffset(resultWithoutCalendar.getTime()); - collector.checkThat(resultWithoutCalendar.getTime() - result.getTime(), is(offset)); - collector.checkThat(accessor.wasNull(), is(false)); + compareOffset(timeZone, finalTimeZoneForResultWithoutCalendar, result.getTime(), resultWithoutCalendar.getTime(), + accessor); }); } @@ -213,23 +207,15 @@ public void testShouldGetDateReturnValidDateWithCalendar() throws Exception { TimeZone timeZone = TimeZone.getTimeZone(AMERICA_SAO_PAULO); Calendar calendar = Calendar.getInstance(timeZone); - TimeZone timeZoneForVector = getTimeZoneForVector(vector); - timeZoneForVector = timeZoneForVector == null ? TimeZone.getDefault() : timeZoneForVector; - - TimeZone finalTimeZoneForResultWithoutCalendar = timeZoneForVector; + TimeZone finalTimeZoneForResultWithoutCalendar = ofNullable(getTimeZoneForVector(vector)) + .orElse(TimeZone.getDefault()); accessorIterator.iterate(vector, (accessor, currentRow) -> { final Date resultWithoutCalendar = accessor.getDate(null); final Date result = accessor.getDate(calendar); - final TimeZone timeZoneForResult = getTimeZoneForVector(vector) == null ? timeZone : - finalTimeZoneForResultWithoutCalendar; - - long offset = timeZoneForResult.getOffset(result.getTime()) - - finalTimeZoneForResultWithoutCalendar.getOffset(resultWithoutCalendar.getTime()); - - collector.checkThat(resultWithoutCalendar.getTime() - result.getTime(), is(offset)); - collector.checkThat(accessor.wasNull(), is(false)); + compareOffset(timeZone, finalTimeZoneForResultWithoutCalendar, result.getTime(), resultWithoutCalendar.getTime(), + accessor); }); } @@ -252,23 +238,15 @@ public void testShouldGetTimeReturnValidTimeWithCalendar() throws Exception { TimeZone timeZone = TimeZone.getTimeZone(AMERICA_SAO_PAULO); Calendar calendar = Calendar.getInstance(timeZone); - TimeZone timeZoneForVector = getTimeZoneForVector(vector); - timeZoneForVector = timeZoneForVector == null ? TimeZone.getDefault() : timeZoneForVector; - - TimeZone finalTimeZoneForResultWithoutCalendar = timeZoneForVector; + TimeZone finalTimeZoneForResultWithoutCalendar = ofNullable(getTimeZoneForVector(vector)) + .orElse(TimeZone.getDefault()); accessorIterator.iterate(vector, (accessor, currentRow) -> { final Time resultWithoutCalendar = accessor.getTime(null); final Time result = accessor.getTime(calendar); - final TimeZone timeZoneForResult = getTimeZoneForVector(vector) == null ? timeZone : - finalTimeZoneForResultWithoutCalendar; - - long offset = timeZoneForResult.getOffset(result.getTime()) - - finalTimeZoneForResultWithoutCalendar.getOffset(resultWithoutCalendar.getTime()); - - collector.checkThat(resultWithoutCalendar.getTime() - result.getTime(), is(offset)); - collector.checkThat(accessor.wasNull(), is(false)); + compareOffset(timeZone, finalTimeZoneForResultWithoutCalendar, result.getTime(), resultWithoutCalendar.getTime(), + accessor); }); } @@ -343,4 +321,16 @@ private void assertGetStringIsConsistentWithVarCharAccessor(Calendar calendar) t }); } } + + private void compareOffset(TimeZone timeZone, TimeZone finalTimeZoneForResultWithoutCalendar, long result, + long resultWithoutCalendar, ArrowFlightJdbcTimeStampVectorAccessor accessor) { + final TimeZone timeZoneForResult = getTimeZoneForVector(vector) == null ? timeZone : + finalTimeZoneForResultWithoutCalendar; + + long offset = timeZoneForResult.getOffset(result) - + finalTimeZoneForResultWithoutCalendar.getOffset(resultWithoutCalendar); + + collector.checkThat(resultWithoutCalendar - result, is(offset)); + collector.checkThat(accessor.wasNull(), is(false)); + } } From 1ff3fc69a2680804c5de6305341657e11a87a31e Mon Sep 17 00:00:00 2001 From: Jay Date: Thu, 6 Jul 2023 10:08:05 -0700 Subject: [PATCH 4/6] - renamed compareOffset to a more descriptive name - Refactored getTimestampForVector() to be more concise and readable --- ...FlightJdbcTimeStampVectorAccessorTest.java | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java b/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java index b1d2bfb7116..d9c0ccddb43 100644 --- a/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java +++ b/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java @@ -183,8 +183,8 @@ public void testShouldGetTimestampReturnValidTimestampWithCalendar() throws Exce final Timestamp resultWithoutCalendar = accessor.getTimestamp(null); final Timestamp result = accessor.getTimestamp(calendar); - compareOffset(timeZone, finalTimeZoneForResultWithoutCalendar, result.getTime(), resultWithoutCalendar.getTime(), - accessor); + assertOffsetIsConsistentWithAccessorGetters(timeZone, finalTimeZoneForResultWithoutCalendar, result.getTime(), + resultWithoutCalendar.getTime(), accessor); }); } @@ -214,8 +214,8 @@ public void testShouldGetDateReturnValidDateWithCalendar() throws Exception { final Date resultWithoutCalendar = accessor.getDate(null); final Date result = accessor.getDate(calendar); - compareOffset(timeZone, finalTimeZoneForResultWithoutCalendar, result.getTime(), resultWithoutCalendar.getTime(), - accessor); + assertOffsetIsConsistentWithAccessorGetters(timeZone, finalTimeZoneForResultWithoutCalendar, result.getTime(), + resultWithoutCalendar.getTime(), accessor); }); } @@ -245,8 +245,8 @@ public void testShouldGetTimeReturnValidTimeWithCalendar() throws Exception { final Time resultWithoutCalendar = accessor.getTime(null); final Time result = accessor.getTime(calendar); - compareOffset(timeZone, finalTimeZoneForResultWithoutCalendar, result.getTime(), resultWithoutCalendar.getTime(), - accessor); + assertOffsetIsConsistentWithAccessorGetters(timeZone, finalTimeZoneForResultWithoutCalendar, result.getTime(), + resultWithoutCalendar.getTime(), accessor); }); } @@ -268,11 +268,9 @@ private Timestamp getTimestampForVector(int currentRow) { TimeUnit timeUnit = getTimeUnitForVector(vector); long millis = timeUnit.toMillis((Long) object); - Instant currInstant = Instant.ofEpochMilli(millis); - LocalDateTime getTimestampWithoutTZ = LocalDateTime.ofInstant(currInstant, - TimeZone.getTimeZone("UTC").toZoneId()); - - ZonedDateTime sourceTZDateTime = getTimestampWithoutTZ.atZone(TimeZone.getTimeZone(timeZone).toZoneId()); + ZonedDateTime sourceTZDateTime = LocalDateTime + .ofInstant(Instant.ofEpochMilli(millis), TimeZone.getTimeZone("UTC").toZoneId()) + .atZone(TimeZone.getTimeZone(timeZone).toZoneId()); expectedTimestamp = new Timestamp(sourceTZDateTime.toEpochSecond() * 1000); } return expectedTimestamp; @@ -322,8 +320,10 @@ private void assertGetStringIsConsistentWithVarCharAccessor(Calendar calendar) t } } - private void compareOffset(TimeZone timeZone, TimeZone finalTimeZoneForResultWithoutCalendar, long result, - long resultWithoutCalendar, ArrowFlightJdbcTimeStampVectorAccessor accessor) { + private void assertOffsetIsConsistentWithAccessorGetters(TimeZone timeZone, + TimeZone finalTimeZoneForResultWithoutCalendar, long result, + long resultWithoutCalendar, + ArrowFlightJdbcTimeStampVectorAccessor accessor) { final TimeZone timeZoneForResult = getTimeZoneForVector(vector) == null ? timeZone : finalTimeZoneForResultWithoutCalendar; From f408486335eff9438f164d11adb02f4c2268488a Mon Sep 17 00:00:00 2001 From: Jay Date: Mon, 24 Jul 2023 10:00:38 -0700 Subject: [PATCH 5/6] ArrowFlightJdbcTimeStampVectorAccessor: Made assigning to sourceTimeZone an if statement and changed all nonNull usage to compare with null explicitly ArrowFlightJdbcTimeStampVectorAccessorTest: Modified test so system timezone is compared more explicitly --- .../ArrowFlightJdbcTimeStampVectorAccessor.java | 14 ++++++++++---- ...ArrowFlightJdbcTimeStampVectorAccessorTest.java | 5 ++++- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java index 22f18464306..486d2b51655 100644 --- a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java +++ b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java @@ -17,7 +17,6 @@ package org.apache.arrow.driver.jdbc.accessor.impl.calendar; -import static java.util.Objects.nonNull; import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcTimeStampVectorGetter.Getter; import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcTimeStampVectorGetter.Holder; import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcTimeStampVectorGetter.createGetter; @@ -92,9 +91,16 @@ private LocalDateTime getLocalDateTime(Calendar calendar) { long value = holder.value; LocalDateTime localDateTime = this.longToLocalDateTime.fromLong(value); - ZoneId defaultTimeZone = TimeZone.getDefault().toZoneId(); - ZoneId sourceTimeZone = nonNull(this.timeZone) ? this.timeZone.toZoneId() : - nonNull(calendar) ? calendar.getTimeZone().toZoneId() : defaultTimeZone; + ZoneId defaultTimeZone = Calendar.getInstance().getTimeZone().toZoneId(); + ZoneId sourceTimeZone; + + if (this.timeZone != null) { + sourceTimeZone = this.timeZone.toZoneId(); + } else if (calendar != null) { + sourceTimeZone = calendar.getTimeZone().toZoneId(); + } else { + sourceTimeZone = defaultTimeZone; + } return localDateTime.atZone(sourceTimeZone).withZoneSameInstant(defaultTimeZone).toLocalDateTime(); } diff --git a/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java b/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java index d9c0ccddb43..83a7e72a1cc 100644 --- a/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java +++ b/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java @@ -173,11 +173,13 @@ public void testShouldGetTimestampReturnValidTimestampWithoutCalendar() throws E @Test public void testShouldGetTimestampReturnValidTimestampWithCalendar() throws Exception { + TimeZone.setDefault(TimeZone.getTimeZone("Asia/Hong_Kong")); + TimeZone timeZone = TimeZone.getTimeZone(AMERICA_SAO_PAULO); Calendar calendar = Calendar.getInstance(timeZone); TimeZone finalTimeZoneForResultWithoutCalendar = ofNullable(getTimeZoneForVector(vector)) - .orElse(TimeZone.getDefault()); + .orElse(TimeZone.getTimeZone("Asia/Hong_Kong")); accessorIterator.iterate(vector, (accessor, currentRow) -> { final Timestamp resultWithoutCalendar = accessor.getTimestamp(null); @@ -186,6 +188,7 @@ public void testShouldGetTimestampReturnValidTimestampWithCalendar() throws Exce assertOffsetIsConsistentWithAccessorGetters(timeZone, finalTimeZoneForResultWithoutCalendar, result.getTime(), resultWithoutCalendar.getTime(), accessor); }); + TimeZone.setDefault(null); } @Test From beb4ea6c91dc61f211f724010dac02b70cc8d51b Mon Sep 17 00:00:00 2001 From: Jay Date: Tue, 8 Aug 2023 22:03:21 -0700 Subject: [PATCH 6/6] Refactored getLongToUTCDateTimeForVector --- ...ArrowFlightJdbcTimeStampVectorAccessor.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java index 486d2b51655..f8688335ba1 100644 --- a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java +++ b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java @@ -45,13 +45,13 @@ public class ArrowFlightJdbcTimeStampVectorAccessor extends ArrowFlightJdbcAcces private final TimeZone timeZone; private final Getter getter; private final TimeUnit timeUnit; - private final LongToLocalDateTime longToLocalDateTime; + private final LongToUTCDateTime longToUTCDateTime; private final Holder holder; /** * Functional interface used to convert a number (in any time resolution) to LocalDateTime. */ - interface LongToLocalDateTime { + interface LongToUTCDateTime { LocalDateTime fromLong(long value); } @@ -67,7 +67,7 @@ public ArrowFlightJdbcTimeStampVectorAccessor(TimeStampVector vector, this.timeZone = getTimeZoneForVector(vector); this.timeUnit = getTimeUnitForVector(vector); - this.longToLocalDateTime = getLongToUTCDateTimeForVector(vector); + this.longToUTCDateTime = getLongToUTCDateTimeForVector(vector); } @Override @@ -90,7 +90,7 @@ private LocalDateTime getLocalDateTime(Calendar calendar) { long value = holder.value; - LocalDateTime localDateTime = this.longToLocalDateTime.fromLong(value); + LocalDateTime localDateTime = this.longToUTCDateTime.fromLong(value); ZoneId defaultTimeZone = Calendar.getInstance().getTimeZone().toZoneId(); ZoneId sourceTimeZone; @@ -153,7 +153,7 @@ protected static TimeUnit getTimeUnitForVector(TimeStampVector vector) { } } - protected static LongToLocalDateTime getLongToUTCDateTimeForVector(TimeStampVector vector) { + protected static LongToUTCDateTime getLongToUTCDateTimeForVector(TimeStampVector vector) { String timeZoneID = "UTC"; ArrowType.Timestamp arrowType = @@ -161,14 +161,14 @@ protected static LongToLocalDateTime getLongToUTCDateTimeForVector(TimeStampVect switch (arrowType.getUnit()) { case NANOSECOND: - return nanoseconds -> DateUtility.getLocalDateTimeFromEpochNano(nanoseconds, timeZoneID); + return DateUtility::getLocalDateTimeFromEpochNano; case MICROSECOND: - return microseconds -> DateUtility.getLocalDateTimeFromEpochMicro(microseconds, timeZoneID); + return DateUtility::getLocalDateTimeFromEpochMicro; case MILLISECOND: - return milliseconds -> DateUtility.getLocalDateTimeFromEpochMilli(milliseconds, timeZoneID); + return DateUtility::getLocalDateTimeFromEpochMilli; case SECOND: return seconds -> DateUtility.getLocalDateTimeFromEpochMilli( - TimeUnit.SECONDS.toMillis(seconds), timeZoneID); + TimeUnit.SECONDS.toMillis(seconds)); default: throw new UnsupportedOperationException("Invalid Arrow time unit"); }