From 2ecbeec184e2c96c537eddbc00f04a5791271f84 Mon Sep 17 00:00:00 2001 From: Jay Ho Date: Wed, 5 Jul 2023 15:18:06 -0700 Subject: [PATCH 01/10] 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 (cherry picked from commit 9d02bfd6ae80137a657bdc080a4e64d02e230518) --- ...rrowFlightJdbcTimeStampVectorAccessor.java | 24 ++++----- .../driver/jdbc/utils/DateTimeUtils.java | 16 +++--- ...FlightJdbcTimeStampVectorAccessorTest.java | 52 +++++++++++++------ .../driver/jdbc/utils/DateTimeUtilsTest.java | 6 +-- 4 files changed, 57 insertions(+), 41 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 debdd0fcb4b..cbfdf2a09f4 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 @@ -24,7 +24,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; @@ -60,7 +61,7 @@ public ArrowFlightJdbcTimeStampVectorAccessor( this.timeZone = getTimeZoneForVector(vector); this.timeUnit = getTimeUnitForVector(vector); - this.longToLocalDateTime = getLongToLocalDateTimeForVector(vector, this.timeZone); + this.longToLocalDateTime = getLongToUTCDateTimeForVector(vector); } @Override @@ -85,13 +86,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; } @@ -143,9 +142,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(); @@ -172,7 +170,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 9363e3486c5..9cc84f7f256 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 @@ -19,9 +19,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; @@ -37,15 +39,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 2e329f148e6..3d6b8f0b6ff 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,10 @@ 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.TimeZone; import java.util.concurrent.TimeUnit; @@ -183,16 +186,18 @@ public void testShouldGetTimestampReturnValidTimestampWithCalendar( Calendar calendar = Calendar.getInstance(timeZone); TimeZone timeZoneForVector = getTimeZoneForVector(vector); + timeZoneForVector = timeZoneForVector == null ? TimeZone.getDefault() : timeZoneForVector; - accessorIterator.iterate( - vector, - (accessor, currentRow) -> { - final Timestamp resultWithoutCalendar = accessor.getTimestamp(null); - final Timestamp result = accessor.getTimestamp(calendar); + TimeZone finalTimeZoneForResultWithoutCalendar = timeZoneForVector; - long offset = - (long) timeZone.getOffset(resultWithoutCalendar.getTime()) - - timeZoneForVector.getOffset(resultWithoutCalendar.getTime()); + 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()); assertThat(resultWithoutCalendar.getTime() - result.getTime(), is(offset)); assertThat(accessor.wasNull(), is(false)); @@ -231,6 +236,9 @@ public void testShouldGetDateReturnValidDateWithCalendar(Supplier Date: Wed, 5 Jul 2023 16:27:42 -0700 Subject: [PATCH 02/10] Modified comments (cherry picked from commit 77e037b817fa14c05d36703c0e34ddce174e53fe) --- .../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 9cc84f7f256..69b7b953886 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 @@ -34,7 +34,7 @@ private DateTimeUtils() { // Prevent instantiation. } - /** 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) { calendar = Calendar.getInstance(); From 3e756e7d6822fe94dd45feaa9798b1a91650e60d Mon Sep 17 00:00:00 2001 From: Jay Date: Thu, 6 Jul 2023 09:43:40 -0700 Subject: [PATCH 03/10] - Refactored duplicate code in ArrowFlightJdbcTimeStampVectorAccessorTest - Made timezone converting logic more readable in ArrowFlightJdbcTimeStampVectorAccessor - Fixed checkstyle issues (cherry picked from commit 588743330ff5bff1544de42457d7e2aaa5144cdb) --- ...rrowFlightJdbcTimeStampVectorAccessor.java | 12 ++-- ...FlightJdbcTimeStampVectorAccessorTest.java | 66 ++++++++----------- 2 files changed, 33 insertions(+), 45 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 cbfdf2a09f4..876aaa3c2cf 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 @@ -16,6 +16,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; @@ -25,7 +26,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; @@ -85,13 +85,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/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 3d6b8f0b6ff..cae8f246198 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 @@ -16,6 +16,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; @@ -185,23 +186,16 @@ public void testShouldGetTimestampReturnValidTimestampWithCalendar( 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()); - assertThat(resultWithoutCalendar.getTime() - result.getTime(), is(offset)); - assertThat(accessor.wasNull(), is(false)); - }); + compareOffset(timeZone, finalTimeZoneForResultWithoutCalendar, result.getTime(), resultWithoutCalendar.getTime(), + accessor); + }); } @ParameterizedTest @@ -235,10 +229,8 @@ public void testShouldGetDateReturnValidDateWithCalendar(Supplier Date: Thu, 6 Jul 2023 10:08:05 -0700 Subject: [PATCH 04/10] - renamed compareOffset to a more descriptive name - Refactored getTimestampForVector() to be more concise and readable (cherry picked from commit 1ff3fc69a2680804c5de6305341657e11a87a31e) --- ...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 cae8f246198..1de3e2f4062 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 @@ -193,8 +193,8 @@ public void testShouldGetTimestampReturnValidTimestampWithCalendar( 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); }); } @@ -238,8 +238,8 @@ public void testShouldGetDateReturnValidDateWithCalendar(Supplier Date: Mon, 24 Jul 2023 10:00:38 -0700 Subject: [PATCH 05/10] 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 (cherry picked from commit f408486335eff9438f164d11adb02f4c2268488a) --- .../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 876aaa3c2cf..ed68bb883d3 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 @@ -16,7 +16,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; @@ -85,9 +84,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 1de3e2f4062..1d6e94d3d39 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,11 +183,13 @@ public void testShouldGetTimestampReturnValidTimestampWithoutCalendar( public void testShouldGetTimestampReturnValidTimestampWithCalendar( Supplier vectorSupplier) throws Exception { setup(vectorSupplier); + 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); @@ -196,6 +198,7 @@ public void testShouldGetTimestampReturnValidTimestampWithCalendar( assertOffsetIsConsistentWithAccessorGetters(timeZone, finalTimeZoneForResultWithoutCalendar, result.getTime(), resultWithoutCalendar.getTime(), accessor); }); + TimeZone.setDefault(null); } @ParameterizedTest From 9c03e42b25c6d853b60589b639be49db373dd0d3 Mon Sep 17 00:00:00 2001 From: Jay Date: Tue, 8 Aug 2023 22:03:21 -0700 Subject: [PATCH 06/10] Refactored getLongToUTCDateTimeForVector (cherry picked from commit beb4ea6c91dc61f211f724010dac02b70cc8d51b) --- ...rrowFlightJdbcTimeStampVectorAccessor.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 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 ed68bb883d3..af3477b113d 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 @@ -41,11 +41,11 @@ 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); } @@ -60,7 +60,7 @@ public ArrowFlightJdbcTimeStampVectorAccessor( this.timeZone = getTimeZoneForVector(vector); this.timeUnit = getTimeUnitForVector(vector); - this.longToLocalDateTime = getLongToUTCDateTimeForVector(vector); + this.longToUTCDateTime = getLongToUTCDateTimeForVector(vector); } @Override @@ -83,7 +83,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; @@ -146,7 +146,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 = @@ -154,15 +154,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); + return seconds -> DateUtility.getLocalDateTimeFromEpochMilli( + TimeUnit.SECONDS.toMillis(seconds)); default: throw new UnsupportedOperationException("Invalid Arrow time unit"); } From ce7982d4b21bf5d9b45813a9f77cd41d322b9712 Mon Sep 17 00:00:00 2001 From: Sergio Cruz Santos Date: Thu, 4 Jul 2024 15:49:31 +0200 Subject: [PATCH 07/10] Executed spotless after cherry picks --- ...rrowFlightJdbcTimeStampVectorAccessor.java | 9 ++- .../driver/jdbc/utils/DateTimeUtils.java | 4 +- ...FlightJdbcTimeStampVectorAccessorTest.java | 79 +++++++++++-------- .../driver/jdbc/utils/DateTimeUtilsTest.java | 4 +- 4 files changed, 58 insertions(+), 38 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 af3477b113d..5d9df655620 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 @@ -95,7 +95,10 @@ private LocalDateTime getLocalDateTime(Calendar calendar) { sourceTimeZone = defaultTimeZone; } - return localDateTime.atZone(sourceTimeZone).withZoneSameInstant(defaultTimeZone).toLocalDateTime(); + return localDateTime + .atZone(sourceTimeZone) + .withZoneSameInstant(defaultTimeZone) + .toLocalDateTime(); } @Override @@ -160,8 +163,8 @@ protected static LongToUTCDateTime getLongToUTCDateTimeForVector(TimeStampVector case MILLISECOND: return DateUtility::getLocalDateTimeFromEpochMilli; case SECOND: - return seconds -> DateUtility.getLocalDateTimeFromEpochMilli( - TimeUnit.SECONDS.toMillis(seconds)); + return seconds -> + DateUtility.getLocalDateTimeFromEpochMilli(TimeUnit.SECONDS.toMillis(seconds)); default: throw new UnsupportedOperationException("Invalid Arrow time unit"); } 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 69b7b953886..9a5fe6e1a12 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 @@ -40,8 +40,8 @@ public static long applyCalendarOffset(long milliseconds, Calendar calendar) { calendar = Calendar.getInstance(); } Instant currInstant = Instant.ofEpochMilli(milliseconds); - LocalDateTime getTimestampWithoutTZ = LocalDateTime.ofInstant(currInstant, - TimeZone.getTimeZone("UTC").toZoneId()); + 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 1d6e94d3d39..08289acc9af 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 @@ -29,7 +29,6 @@ import java.time.Instant; import java.time.LocalDateTime; import java.time.ZonedDateTime; -import java.util.Arrays; import java.util.Calendar; import java.util.TimeZone; import java.util.concurrent.TimeUnit; @@ -188,16 +187,22 @@ public void testShouldGetTimestampReturnValidTimestampWithCalendar( TimeZone timeZone = TimeZone.getTimeZone(AMERICA_SAO_PAULO); Calendar calendar = Calendar.getInstance(timeZone); - TimeZone finalTimeZoneForResultWithoutCalendar = ofNullable(getTimeZoneForVector(vector)) - .orElse(TimeZone.getTimeZone("Asia/Hong_Kong")); + TimeZone finalTimeZoneForResultWithoutCalendar = + ofNullable(getTimeZoneForVector(vector)).orElse(TimeZone.getTimeZone("Asia/Hong_Kong")); - accessorIterator.iterate(vector, (accessor, currentRow) -> { - final Timestamp resultWithoutCalendar = accessor.getTimestamp(null); - final Timestamp result = accessor.getTimestamp(calendar); - - assertOffsetIsConsistentWithAccessorGetters(timeZone, finalTimeZoneForResultWithoutCalendar, result.getTime(), - resultWithoutCalendar.getTime(), accessor); - }); + accessorIterator.iterate( + vector, + (accessor, currentRow) -> { + final Timestamp resultWithoutCalendar = accessor.getTimestamp(null); + final Timestamp result = accessor.getTimestamp(calendar); + + assertOffsetIsConsistentWithAccessorGetters( + timeZone, + finalTimeZoneForResultWithoutCalendar, + result.getTime(), + resultWithoutCalendar.getTime(), + accessor); + }); TimeZone.setDefault(null); } @@ -232,8 +237,8 @@ public void testShouldGetDateReturnValidDateWithCalendar(Supplier Date: Thu, 4 Jul 2024 15:54:41 +0200 Subject: [PATCH 08/10] Removed unused variables --- .../impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java | 4 ---- 1 file changed, 4 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 5d9df655620..20a4d610c45 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 @@ -40,7 +40,6 @@ public class ArrowFlightJdbcTimeStampVectorAccessor extends ArrowFlightJdbcAcces private final TimeZone timeZone; private final Getter getter; - private final TimeUnit timeUnit; private final LongToUTCDateTime longToUTCDateTime; private final Holder holder; @@ -59,7 +58,6 @@ public ArrowFlightJdbcTimeStampVectorAccessor( this.getter = createGetter(vector); this.timeZone = getTimeZoneForVector(vector); - this.timeUnit = getTimeUnitForVector(vector); this.longToUTCDateTime = getLongToUTCDateTimeForVector(vector); } @@ -150,8 +148,6 @@ protected static TimeUnit getTimeUnitForVector(TimeStampVector vector) { } protected static LongToUTCDateTime getLongToUTCDateTimeForVector(TimeStampVector vector) { - String timeZoneID = "UTC"; - ArrowType.Timestamp arrowType = (ArrowType.Timestamp) vector.getField().getFieldType().getType(); From df8252874a2b8a2d3ee94d697494246e8e4b9354 Mon Sep 17 00:00:00 2001 From: Sergio Cruz Santos Date: Thu, 4 Jul 2024 16:20:40 +0200 Subject: [PATCH 09/10] Fixed millis truncation. https://github.com/apache/arrow/pull/36519#discussion_r1346213335 --- .../calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 08289acc9af..3be42df69fd 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 @@ -328,7 +328,7 @@ private Timestamp getTimestampForVector(int currentRow, String timeZone) { LocalDateTime.ofInstant( Instant.ofEpochMilli(millis), TimeZone.getTimeZone("UTC").toZoneId()) .atZone(TimeZone.getTimeZone(timeZone).toZoneId()); - expectedTimestamp = new Timestamp(sourceTZDateTime.toEpochSecond() * 1000); + expectedTimestamp = new Timestamp(sourceTZDateTime.toInstant().toEpochMilli()); } return expectedTimestamp; } From 9f9a239609d128577224cdc6995322e258e27832 Mon Sep 17 00:00:00 2001 From: Sergio Cruz Santos Date: Fri, 5 Jul 2024 13:45:58 +0200 Subject: [PATCH 10/10] GH-36518: [Java] Fix ArrowFlightJdbcTimeStampVectorAccessor to return Timestamp objects with date and time that corresponds with local time instead of UTC date and time Fixed truncation of millis at DateTimeUtils Fixed date values returned when the vector was DateVector which was affected by changes at common method. --- .../ArrowFlightJdbcDateVectorAccessor.java | 47 ++++++++++++++----- .../driver/jdbc/utils/DateTimeUtils.java | 2 +- 2 files changed, 35 insertions(+), 14 deletions(-) diff --git a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java index ebe40162095..2b1f0a2e5b1 100644 --- a/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java +++ b/java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java @@ -19,21 +19,22 @@ 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; import java.sql.Date; import java.sql.Timestamp; +import java.time.LocalDateTime; +import java.time.ZoneId; import java.util.Calendar; import java.util.concurrent.TimeUnit; import java.util.function.IntSupplier; import org.apache.arrow.driver.jdbc.accessor.ArrowFlightJdbcAccessor; import org.apache.arrow.driver.jdbc.accessor.ArrowFlightJdbcAccessorFactory; -import org.apache.arrow.driver.jdbc.utils.DateTimeUtils; import org.apache.arrow.vector.DateDayVector; import org.apache.arrow.vector.DateMilliVector; import org.apache.arrow.vector.ValueVector; +import org.apache.arrow.vector.util.DateUtility; /** Accessor for the Arrow types: {@link DateDayVector} and {@link DateMilliVector}. */ public class ArrowFlightJdbcDateVectorAccessor extends ArrowFlightJdbcAccessor { @@ -87,17 +88,12 @@ public Object getObject() { @Override public Date getDate(Calendar calendar) { - fillHolder(); - if (this.wasNull) { + final LocalDateTime localDateTime = getLocalDateTime(calendar); + if (localDateTime == null) { return null; } - long value = holder.value; - long milliseconds = this.timeUnit.toMillis(value); - - long millisWithCalendar = DateTimeUtils.applyCalendarOffset(milliseconds, calendar); - - return new Date(getTimestampValue(millisWithCalendar).getTime()); + return new Date(Timestamp.valueOf(localDateTime).getTime()); } private void fillHolder() { @@ -108,11 +104,36 @@ private void fillHolder() { @Override public Timestamp getTimestamp(Calendar calendar) { - Date date = getDate(calendar); - if (date == null) { + final LocalDateTime localDateTime = getLocalDateTime(calendar); + if (localDateTime == null) { + return null; + } + + return Timestamp.valueOf(localDateTime); + } + + private LocalDateTime getLocalDateTime(Calendar calendar) { + getter.get(getCurrentRow(), holder); + this.wasNull = holder.isSet == 0; + this.wasNullConsumer.setWasNull(this.wasNull); + if (this.wasNull) { return null; } - return new Timestamp(date.getTime()); + + final LocalDateTime localDateTime = + DateUtility.getLocalDateTimeFromEpochMilli(this.timeUnit.toMillis(holder.value)); + final ZoneId defaultTimeZone = Calendar.getInstance().getTimeZone().toZoneId(); + final ZoneId sourceTimeZone; + if (calendar != null) { + sourceTimeZone = calendar.getTimeZone().toZoneId(); + } else { + sourceTimeZone = defaultTimeZone; + } + + 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 9a5fe6e1a12..cf1cc8b2a76 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 @@ -43,7 +43,7 @@ public static long applyCalendarOffset(long milliseconds, Calendar calendar) { LocalDateTime getTimestampWithoutTZ = LocalDateTime.ofInstant(currInstant, TimeZone.getTimeZone("UTC").toZoneId()); ZonedDateTime parsedTime = getTimestampWithoutTZ.atZone(calendar.getTimeZone().toZoneId()); - return parsedTime.toEpochSecond() * 1000; + return parsedTime.toInstant().toEpochMilli(); } /**