From be59f2fa9208c2618d7863a9e9c26c7a1ebe83c6 Mon Sep 17 00:00:00 2001 From: Diego Fernandez Date: Mon, 23 Dec 2024 17:53:55 -0700 Subject: [PATCH 01/13] Improve TZ support for JDBC driver --- .../ArrowFlightJdbcDateVectorAccessor.java | 30 +++++++- ...rrowFlightJdbcTimeStampVectorAccessor.java | 76 ++++++++++++++++++- .../ArrowFlightJdbcTimeVectorAccessor.java | 32 ++++++-- .../arrow/driver/jdbc/utils/SqlTypes.java | 7 +- 4 files changed, 130 insertions(+), 15 deletions(-) diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java index ebe401620..db6f8c275 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java @@ -24,10 +24,13 @@ import static org.apache.calcite.avatica.util.DateTimeUtils.unixDateToString; import java.sql.Date; +import java.sql.SQLException; import java.sql.Timestamp; +import java.time.LocalDate; 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; @@ -35,7 +38,9 @@ import org.apache.arrow.vector.DateMilliVector; import org.apache.arrow.vector.ValueVector; -/** Accessor for the Arrow types: {@link DateDayVector} and {@link DateMilliVector}. */ +/** + * Accessor for the Arrow types: {@link DateDayVector} and {@link DateMilliVector}. + */ public class ArrowFlightJdbcDateVectorAccessor extends ArrowFlightJdbcAccessor { private final Getter getter; @@ -45,9 +50,9 @@ public class ArrowFlightJdbcDateVectorAccessor extends ArrowFlightJdbcAccessor { /** * Instantiate an accessor for a {@link DateDayVector}. * - * @param vector an instance of a DateDayVector. + * @param vector an instance of a DateDayVector. * @param currentRowSupplier the supplier to track the lines. - * @param setCursorWasNull the consumer to set if value was null. + * @param setCursorWasNull the consumer to set if value was null. */ public ArrowFlightJdbcDateVectorAccessor( DateDayVector vector, @@ -62,7 +67,7 @@ public ArrowFlightJdbcDateVectorAccessor( /** * Instantiate an accessor for a {@link DateMilliVector}. * - * @param vector an instance of a DateMilliVector. + * @param vector an instance of a DateMilliVector. * @param currentRowSupplier the supplier to track the lines. */ public ArrowFlightJdbcDateVectorAccessor( @@ -85,6 +90,19 @@ public Object getObject() { return this.getDate(null); } + @Override + public T getObject(final Class type) throws SQLException { + final Object value; + if (type == LocalDate.class) { + value = getLocalDate(); + } else if (type == Date.class) { + value = getObject(); + } else { + throw new SQLException("invalid class"); + } + return !type.isPrimitive() && wasNull ? null : type.cast(value); + } + @Override public Date getDate(Calendar calendar) { fillHolder(); @@ -134,4 +152,8 @@ protected static TimeUnit getTimeUnitForVector(ValueVector vector) { throw new IllegalArgumentException("Invalid Arrow vector"); } + + private LocalDate getLocalDate() { + return getDate(null).toLocalDate(); + } } diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java index debdd0fcb..ae8a72eb5 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java @@ -21,21 +21,29 @@ import static org.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcTimeStampVectorGetter.createGetter; import java.sql.Date; +import java.sql.SQLException; import java.sql.Time; import java.sql.Timestamp; +import java.time.Instant; import java.time.LocalDateTime; +import java.time.OffsetDateTime; +import java.time.ZoneOffset; +import java.time.ZonedDateTime; import java.time.temporal.ChronoUnit; import java.util.Calendar; import java.util.TimeZone; 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.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}. */ +/** + * Accessor for the Arrow types extending from {@link TimeStampVector}. + */ public class ArrowFlightJdbcTimeStampVectorAccessor extends ArrowFlightJdbcAccessor { private final TimeZone timeZone; @@ -43,13 +51,18 @@ public class ArrowFlightJdbcTimeStampVectorAccessor extends ArrowFlightJdbcAcces private final TimeUnit timeUnit; private final LongToLocalDateTime longToLocalDateTime; private final Holder holder; + private final boolean isZoned; - /** Functional interface used to convert a number (in any time resolution) to LocalDateTime. */ + /** + * 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. */ + /** + * Instantiate a ArrowFlightJdbcTimeStampVectorAccessor for given vector. + */ public ArrowFlightJdbcTimeStampVectorAccessor( TimeStampVector vector, IntSupplier currentRowSupplier, @@ -58,6 +71,7 @@ public ArrowFlightJdbcTimeStampVectorAccessor( this.holder = new Holder(); this.getter = createGetter(vector); + this.isZoned = getVectorIsZoned(vector); this.timeZone = getTimeZoneForVector(vector); this.timeUnit = getTimeUnitForVector(vector); this.longToLocalDateTime = getLongToLocalDateTimeForVector(vector, this.timeZone); @@ -68,11 +82,57 @@ public Class getObjectClass() { return Timestamp.class; } + @Override + public T getObject(final Class type) throws SQLException { + final Object value; + if (type == OffsetDateTime.class) { + value = getOffsetDateTime(); + } else if (type == LocalDateTime.class) { + value = getLocalDateTime(null); + } else if (type == ZonedDateTime.class) { + value = getZonedDateTime(); + } else if (type == Instant.class) { + value = getInstant(); + } else if (type == Timestamp.class) { + value = getObject(); + } else { + throw new SQLException("invalid class"); + } + return !type.isPrimitive() && wasNull ? null : type.cast(value); + } + @Override public Object getObject() { return this.getTimestamp(null); } + private ZonedDateTime getZonedDateTime() { + LocalDateTime localDateTime = getLocalDateTime(null); + if (localDateTime == null) { + return null; + } + + return localDateTime.atZone(this.timeZone.toZoneId()); + } + + private OffsetDateTime getOffsetDateTime() { + LocalDateTime localDateTime = getLocalDateTime(null); + if (localDateTime == null) { + return null; + } + ZoneOffset offset = this.timeZone.toZoneId().getRules().getOffset(localDateTime); + return localDateTime.atOffset(offset); + } + + private Instant getInstant() { + LocalDateTime localDateTime = getLocalDateTime(null); + if (localDateTime == null) { + return null; + } + ZoneOffset offset = this.timeZone.toZoneId().getRules().getOffset(localDateTime); + return localDateTime.toInstant(offset); + } + private LocalDateTime getLocalDateTime(Calendar calendar) { getter.get(getCurrentRow(), holder); this.wasNull = holder.isSet == 0; @@ -85,7 +145,8 @@ private LocalDateTime getLocalDateTime(Calendar calendar) { LocalDateTime localDateTime = this.longToLocalDateTime.fromLong(value); - if (calendar != null) { + // Adjust timestamp to desired calendar (if provided) only if the column includes TZ info, otherwise treat as wall-clock time + if (calendar != null && this.isZoned) { TimeZone timeZone = calendar.getTimeZone(); long millis = this.timeUnit.toMillis(value); localDateTime = @@ -177,4 +238,11 @@ protected static TimeZone getTimeZoneForVector(TimeStampVector vector) { return TimeZone.getTimeZone(timezoneName); } + + protected static boolean getVectorIsZoned(TimeStampVector vector) { + ArrowType.Timestamp arrowType = + (ArrowType.Timestamp) vector.getField().getFieldType().getType(); + + return arrowType.getTimezone() != null; + } } diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorAccessor.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorAccessor.java index 2c03ee631..d7a826e0d 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorAccessor.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorAccessor.java @@ -20,11 +20,14 @@ 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 java.sql.SQLException; import java.sql.Time; import java.sql.Timestamp; +import java.time.LocalTime; 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; @@ -48,9 +51,9 @@ public class ArrowFlightJdbcTimeVectorAccessor extends ArrowFlightJdbcAccessor { /** * Instantiate an accessor for a {@link TimeNanoVector}. * - * @param vector an instance of a TimeNanoVector. + * @param vector an instance of a TimeNanoVector. * @param currentRowSupplier the supplier to track the lines. - * @param setCursorWasNull the consumer to set if value was null. + * @param setCursorWasNull the consumer to set if value was null. */ public ArrowFlightJdbcTimeVectorAccessor( TimeNanoVector vector, @@ -65,9 +68,9 @@ public ArrowFlightJdbcTimeVectorAccessor( /** * Instantiate an accessor for a {@link TimeMicroVector}. * - * @param vector an instance of a TimeMicroVector. + * @param vector an instance of a TimeMicroVector. * @param currentRowSupplier the supplier to track the lines. - * @param setCursorWasNull the consumer to set if value was null. + * @param setCursorWasNull the consumer to set if value was null. */ public ArrowFlightJdbcTimeVectorAccessor( TimeMicroVector vector, @@ -82,7 +85,7 @@ public ArrowFlightJdbcTimeVectorAccessor( /** * Instantiate an accessor for a {@link TimeMilliVector}. * - * @param vector an instance of a TimeMilliVector. + * @param vector an instance of a TimeMilliVector. * @param currentRowSupplier the supplier to track the lines. */ public ArrowFlightJdbcTimeVectorAccessor( @@ -98,7 +101,7 @@ public ArrowFlightJdbcTimeVectorAccessor( /** * Instantiate an accessor for a {@link TimeSecVector}. * - * @param vector an instance of a TimeSecVector. + * @param vector an instance of a TimeSecVector. * @param currentRowSupplier the supplier to track the lines. */ public ArrowFlightJdbcTimeVectorAccessor( @@ -121,6 +124,19 @@ public Object getObject() { return this.getTime(null); } + @Override + public T getObject(final Class type) throws SQLException { + final Object value; + if (type == LocalTime.class) { + value = getLocalTime(); + } else if (type == Time.class) { + value = getObject(); + } else { + throw new SQLException("invalid class"); + } + return !type.isPrimitive() && wasNull ? null : type.cast(value); + } + @Override public Time getTime(Calendar calendar) { fillHolder(); @@ -134,6 +150,10 @@ public Time getTime(Calendar calendar) { return new ArrowFlightJdbcTime(DateTimeUtils.applyCalendarOffset(milliseconds, calendar)); } + private LocalTime getLocalTime() { + return getTime(null).toLocalTime(); + } + private void fillHolder() { getter.get(getCurrentRow(), holder); this.wasNull = holder.isSet == 0; diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java index 96cb056db..8ca3b2da8 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java @@ -120,7 +120,12 @@ public static int getSqlTypeIdFromArrowType(ArrowType arrowType) { case Time: return Types.TIME; case Timestamp: - return Types.TIMESTAMP; + String tz = ((ArrowType.Timestamp) arrowType).getTimezone(); + if (tz != null){ + return Types.TIMESTAMP_WITH_TIMEZONE; + } else { + return Types.TIMESTAMP; + } case Bool: return Types.BOOLEAN; case Decimal: From d3321d128d029251593292ffc4c46e05d4a86e10 Mon Sep 17 00:00:00 2001 From: Diego Fernandez Date: Tue, 14 Jan 2025 23:32:51 -0700 Subject: [PATCH 02/13] Add notes and minor cleanup --- .../calendar/ArrowFlightJdbcTimeStampVectorAccessor.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java index ae8a72eb5..33a3b32cc 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java @@ -31,6 +31,7 @@ import java.time.ZonedDateTime; import java.time.temporal.ChronoUnit; import java.util.Calendar; +import java.util.Objects; import java.util.TimeZone; import java.util.concurrent.TimeUnit; import java.util.function.IntSupplier; @@ -71,7 +72,9 @@ public ArrowFlightJdbcTimeStampVectorAccessor( this.holder = new Holder(); this.getter = createGetter(vector); + // whether the vector included TZ info this.isZoned = getVectorIsZoned(vector); + // non-null, either the vector TZ or default to UTC this.timeZone = getTimeZoneForVector(vector); this.timeUnit = getTimeUnitForVector(vector); this.longToLocalDateTime = getLongToLocalDateTimeForVector(vector, this.timeZone); @@ -231,11 +234,7 @@ protected static TimeZone getTimeZoneForVector(TimeStampVector vector) { ArrowType.Timestamp arrowType = (ArrowType.Timestamp) vector.getField().getFieldType().getType(); - String timezoneName = arrowType.getTimezone(); - if (timezoneName == null) { - return TimeZone.getTimeZone("UTC"); - } - + String timezoneName = Objects.requireNonNullElse(arrowType.getTimezone(), "UTC"); return TimeZone.getTimeZone(timezoneName); } From 26e64c6b30f94b21757d859572194c86ffd1846b Mon Sep 17 00:00:00 2001 From: Diego Fernandez Date: Wed, 26 Feb 2025 15:26:16 -0500 Subject: [PATCH 03/13] Reformat code --- .../ArrowFlightJdbcDateVectorAccessor.java | 11 ++++------- .../ArrowFlightJdbcTimeStampVectorAccessor.java | 16 +++++----------- .../ArrowFlightJdbcTimeVectorAccessor.java | 13 ++++++------- .../apache/arrow/driver/jdbc/utils/SqlTypes.java | 2 +- 4 files changed, 16 insertions(+), 26 deletions(-) diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java index db6f8c275..f696e5c0a 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java @@ -30,7 +30,6 @@ 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; @@ -38,9 +37,7 @@ import org.apache.arrow.vector.DateMilliVector; import org.apache.arrow.vector.ValueVector; -/** - * Accessor for the Arrow types: {@link DateDayVector} and {@link DateMilliVector}. - */ +/** Accessor for the Arrow types: {@link DateDayVector} and {@link DateMilliVector}. */ public class ArrowFlightJdbcDateVectorAccessor extends ArrowFlightJdbcAccessor { private final Getter getter; @@ -50,9 +47,9 @@ public class ArrowFlightJdbcDateVectorAccessor extends ArrowFlightJdbcAccessor { /** * Instantiate an accessor for a {@link DateDayVector}. * - * @param vector an instance of a DateDayVector. + * @param vector an instance of a DateDayVector. * @param currentRowSupplier the supplier to track the lines. - * @param setCursorWasNull the consumer to set if value was null. + * @param setCursorWasNull the consumer to set if value was null. */ public ArrowFlightJdbcDateVectorAccessor( DateDayVector vector, @@ -67,7 +64,7 @@ public ArrowFlightJdbcDateVectorAccessor( /** * Instantiate an accessor for a {@link DateMilliVector}. * - * @param vector an instance of a DateMilliVector. + * @param vector an instance of a DateMilliVector. * @param currentRowSupplier the supplier to track the lines. */ public ArrowFlightJdbcDateVectorAccessor( diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java index 33a3b32cc..709cf8007 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java @@ -35,16 +35,13 @@ import java.util.TimeZone; 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.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}. - */ +/** Accessor for the Arrow types extending from {@link TimeStampVector}. */ public class ArrowFlightJdbcTimeStampVectorAccessor extends ArrowFlightJdbcAccessor { private final TimeZone timeZone; @@ -54,16 +51,12 @@ public class ArrowFlightJdbcTimeStampVectorAccessor extends ArrowFlightJdbcAcces private final Holder holder; private final boolean isZoned; - /** - * Functional interface used to convert a number (in any time resolution) to LocalDateTime. - */ + /** 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. - */ + /** Instantiate a ArrowFlightJdbcTimeStampVectorAccessor for given vector. */ public ArrowFlightJdbcTimeStampVectorAccessor( TimeStampVector vector, IntSupplier currentRowSupplier, @@ -148,7 +141,8 @@ private LocalDateTime getLocalDateTime(Calendar calendar) { LocalDateTime localDateTime = this.longToLocalDateTime.fromLong(value); - // Adjust timestamp to desired calendar (if provided) only if the column includes TZ info, otherwise treat as wall-clock time + // Adjust timestamp to desired calendar (if provided) only if the column includes TZ info, + // otherwise treat as wall-clock time if (calendar != null && this.isZoned) { TimeZone timeZone = calendar.getTimeZone(); long millis = this.timeUnit.toMillis(value); diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorAccessor.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorAccessor.java index d7a826e0d..315d36b2f 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorAccessor.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorAccessor.java @@ -27,7 +27,6 @@ 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; @@ -51,9 +50,9 @@ public class ArrowFlightJdbcTimeVectorAccessor extends ArrowFlightJdbcAccessor { /** * Instantiate an accessor for a {@link TimeNanoVector}. * - * @param vector an instance of a TimeNanoVector. + * @param vector an instance of a TimeNanoVector. * @param currentRowSupplier the supplier to track the lines. - * @param setCursorWasNull the consumer to set if value was null. + * @param setCursorWasNull the consumer to set if value was null. */ public ArrowFlightJdbcTimeVectorAccessor( TimeNanoVector vector, @@ -68,9 +67,9 @@ public ArrowFlightJdbcTimeVectorAccessor( /** * Instantiate an accessor for a {@link TimeMicroVector}. * - * @param vector an instance of a TimeMicroVector. + * @param vector an instance of a TimeMicroVector. * @param currentRowSupplier the supplier to track the lines. - * @param setCursorWasNull the consumer to set if value was null. + * @param setCursorWasNull the consumer to set if value was null. */ public ArrowFlightJdbcTimeVectorAccessor( TimeMicroVector vector, @@ -85,7 +84,7 @@ public ArrowFlightJdbcTimeVectorAccessor( /** * Instantiate an accessor for a {@link TimeMilliVector}. * - * @param vector an instance of a TimeMilliVector. + * @param vector an instance of a TimeMilliVector. * @param currentRowSupplier the supplier to track the lines. */ public ArrowFlightJdbcTimeVectorAccessor( @@ -101,7 +100,7 @@ public ArrowFlightJdbcTimeVectorAccessor( /** * Instantiate an accessor for a {@link TimeSecVector}. * - * @param vector an instance of a TimeSecVector. + * @param vector an instance of a TimeSecVector. * @param currentRowSupplier the supplier to track the lines. */ public ArrowFlightJdbcTimeVectorAccessor( diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java index 8ca3b2da8..57facf98c 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java @@ -121,7 +121,7 @@ public static int getSqlTypeIdFromArrowType(ArrowType arrowType) { return Types.TIME; case Timestamp: String tz = ((ArrowType.Timestamp) arrowType).getTimezone(); - if (tz != null){ + if (tz != null) { return Types.TIMESTAMP_WITH_TIMEZONE; } else { return Types.TIMESTAMP; From 24391974f2250dbe91effe6b7a00143a6be9dcbf Mon Sep 17 00:00:00 2001 From: Diego Fernandez Date: Sun, 2 Mar 2025 15:04:20 -0700 Subject: [PATCH 04/13] Fix tests --- .../arrow/driver/jdbc/utils/SqlTypes.java | 8 ++-- .../jdbc/ArrowDatabaseMetadataTest.java | 5 ++- ...FlightJdbcTimeStampVectorAccessorTest.java | 39 +++++++++++++------ .../arrow/driver/jdbc/utils/SqlTypesTest.java | 12 ++++++ 4 files changed, 48 insertions(+), 16 deletions(-) diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java index 57facf98c..574a482f5 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java @@ -22,6 +22,8 @@ import org.apache.arrow.vector.types.FloatingPointPrecision; import org.apache.arrow.vector.types.pojo.ArrowType; +import com.google.common.base.Strings; + /** SQL Types utility functions. */ public class SqlTypes { private static final Map typeIdToName = new HashMap<>(); @@ -121,10 +123,10 @@ public static int getSqlTypeIdFromArrowType(ArrowType arrowType) { return Types.TIME; case Timestamp: String tz = ((ArrowType.Timestamp) arrowType).getTimezone(); - if (tz != null) { - return Types.TIMESTAMP_WITH_TIMEZONE; - } else { + if (Strings.isNullOrEmpty(tz)) { return Types.TIMESTAMP; + } else { + return Types.TIMESTAMP_WITH_TIMEZONE; } case Bool: return Types.BOOLEAN; diff --git a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ArrowDatabaseMetadataTest.java b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ArrowDatabaseMetadataTest.java index 88a172e4f..70d3bcbd3 100644 --- a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ArrowDatabaseMetadataTest.java +++ b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ArrowDatabaseMetadataTest.java @@ -299,8 +299,9 @@ public class ArrowDatabaseMetadataTest { private static Connection connection; static { - List expectedGetColumnsDataTypes = Arrays.asList(3, 93, 4); - List expectedGetColumnsTypeName = Arrays.asList("DECIMAL", "TIMESTAMP", "INTEGER"); + List expectedGetColumnsDataTypes = Arrays.asList(3, 2014, 4); + List expectedGetColumnsTypeName = + Arrays.asList("DECIMAL", "TIMESTAMP_WITH_TIMEZONE", "INTEGER"); List expectedGetColumnsRadix = Arrays.asList(10, null, 10); List expectedGetColumnsColumnSize = Arrays.asList(5, 29, 10); List expectedGetColumnsDecimalDigits = Arrays.asList(2, 9, 0); diff --git a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java index 2e329f148..244a08589 100644 --- a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java +++ b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java @@ -16,8 +16,7 @@ */ package org.apache.arrow.driver.jdbc.accessor.impl.calendar; -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.apache.arrow.driver.jdbc.accessor.impl.calendar.ArrowFlightJdbcTimeStampVectorAccessor.*; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; @@ -183,6 +182,7 @@ public void testShouldGetTimestampReturnValidTimestampWithCalendar( Calendar calendar = Calendar.getInstance(timeZone); TimeZone timeZoneForVector = getTimeZoneForVector(vector); + boolean hasTz = getVectorIsZoned(vector); accessorIterator.iterate( vector, @@ -190,9 +190,14 @@ public void testShouldGetTimestampReturnValidTimestampWithCalendar( final Timestamp resultWithoutCalendar = accessor.getTimestamp(null); final Timestamp result = accessor.getTimestamp(calendar); - long offset = - (long) timeZone.getOffset(resultWithoutCalendar.getTime()) - - timeZoneForVector.getOffset(resultWithoutCalendar.getTime()); + long offset; + if (hasTz) { + offset = + (long) timeZone.getOffset(resultWithoutCalendar.getTime()) + - timeZoneForVector.getOffset(resultWithoutCalendar.getTime()); + } else { + offset = 0; + } assertThat(resultWithoutCalendar.getTime() - result.getTime(), is(offset)); assertThat(accessor.wasNull(), is(false)); @@ -231,6 +236,7 @@ public void testShouldGetDateReturnValidDateWithCalendar(Supplier Date: Sun, 2 Mar 2025 15:54:59 -0700 Subject: [PATCH 05/13] Undo breaking change for ArrowFlightJdbcTimeStampVectorAccessor.getTimestamp --- ...rrowFlightJdbcTimeStampVectorAccessor.java | 13 ++++++- .../arrow/driver/jdbc/utils/SqlTypes.java | 3 +- ...FlightJdbcTimeStampVectorAccessorTest.java | 36 +++++-------------- 3 files changed, 22 insertions(+), 30 deletions(-) diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java index 709cf8007..3ad488a5a 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java @@ -179,8 +179,19 @@ public Timestamp getTimestamp(Calendar calendar) { if (localDateTime == null) { return null; } + // to prevent breaking changes for those using this, apply the offset that was previously + // applied in getLocalDateTime + if (calendar != null && !isZoned) { + TimeZone timeZone = calendar.getTimeZone(); + long millis = Timestamp.valueOf(localDateTime).getTime(); + localDateTime = + localDateTime.minus( + timeZone.getOffset(millis) - this.timeZone.getOffset(millis), ChronoUnit.MILLIS); - return Timestamp.valueOf(localDateTime); + return Timestamp.valueOf(localDateTime); + } else { + return Timestamp.valueOf(localDateTime); + } } protected static TimeUnit getTimeUnitForVector(TimeStampVector vector) { diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java index 574a482f5..1b76ca0c9 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java @@ -16,14 +16,13 @@ */ package org.apache.arrow.driver.jdbc.utils; +import com.google.common.base.Strings; import java.sql.Types; import java.util.HashMap; import java.util.Map; import org.apache.arrow.vector.types.FloatingPointPrecision; import org.apache.arrow.vector.types.pojo.ArrowType; -import com.google.common.base.Strings; - /** SQL Types utility functions. */ public class SqlTypes { private static final Map typeIdToName = new HashMap<>(); diff --git a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java index 244a08589..8aa66e91e 100644 --- a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java +++ b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java @@ -182,7 +182,6 @@ public void testShouldGetTimestampReturnValidTimestampWithCalendar( Calendar calendar = Calendar.getInstance(timeZone); TimeZone timeZoneForVector = getTimeZoneForVector(vector); - boolean hasTz = getVectorIsZoned(vector); accessorIterator.iterate( vector, @@ -190,14 +189,9 @@ public void testShouldGetTimestampReturnValidTimestampWithCalendar( final Timestamp resultWithoutCalendar = accessor.getTimestamp(null); final Timestamp result = accessor.getTimestamp(calendar); - long offset; - if (hasTz) { - offset = - (long) timeZone.getOffset(resultWithoutCalendar.getTime()) - - timeZoneForVector.getOffset(resultWithoutCalendar.getTime()); - } else { - offset = 0; - } + long offset = + (long) timeZone.getOffset(resultWithoutCalendar.getTime()) + - timeZoneForVector.getOffset(resultWithoutCalendar.getTime()); assertThat(resultWithoutCalendar.getTime() - result.getTime(), is(offset)); assertThat(accessor.wasNull(), is(false)); @@ -236,7 +230,6 @@ public void testShouldGetDateReturnValidDateWithCalendar(Supplier Date: Sun, 2 Mar 2025 16:06:20 -0700 Subject: [PATCH 06/13] Also fix getTime and getDate --- ...rrowFlightJdbcTimeStampVectorAccessor.java | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java index 3ad488a5a..bf4428d65 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java @@ -160,7 +160,7 @@ public Date getDate(Calendar calendar) { return null; } - return new Date(Timestamp.valueOf(localDateTime).getTime()); + return new Date(getTimstampWithOffset(calendar, localDateTime).getTime()); } @Override @@ -170,7 +170,7 @@ public Time getTime(Calendar calendar) { return null; } - return new Time(Timestamp.valueOf(localDateTime).getTime()); + return new Time(getTimstampWithOffset(calendar, localDateTime).getTime()); } @Override @@ -179,19 +179,26 @@ public Timestamp getTimestamp(Calendar calendar) { if (localDateTime == null) { return null; } - // to prevent breaking changes for those using this, apply the offset that was previously - // applied in getLocalDateTime + + return getTimstampWithOffset(calendar, localDateTime); + } + + /** + * Apply offset to LocalDateTime to get a Timestamp with legacy behavior. Previously we applied + * the offset to the LocalDateTime even if the underlying Vector did not have a TZ. In order to + * support java.time.* accessors, we fixed this so we only apply the offset if the underlying + * vector includes TZ info. In order to maintain backward compatibility, we apply the offset if + * needed for getDate, getTime, and getTimestamp. + */ + private Timestamp getTimstampWithOffset(Calendar calendar, LocalDateTime localDateTime) { if (calendar != null && !isZoned) { TimeZone timeZone = calendar.getTimeZone(); long millis = Timestamp.valueOf(localDateTime).getTime(); localDateTime = localDateTime.minus( timeZone.getOffset(millis) - this.timeZone.getOffset(millis), ChronoUnit.MILLIS); - - return Timestamp.valueOf(localDateTime); - } else { - return Timestamp.valueOf(localDateTime); } + return Timestamp.valueOf(localDateTime); } protected static TimeUnit getTimeUnitForVector(TimeStampVector vector) { From a2a7930fec80bee38ad0b7e32de826393988ebd8 Mon Sep 17 00:00:00 2001 From: Diego Fernandez Date: Sun, 2 Mar 2025 18:05:58 -0700 Subject: [PATCH 07/13] Add unit tests for java.time objects for TimeStampVectorAccessor --- ...FlightJdbcTimeStampVectorAccessorTest.java | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java index 8aa66e91e..a69141308 100644 --- a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java +++ b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java @@ -24,8 +24,13 @@ import java.sql.Date; import java.sql.Time; import java.sql.Timestamp; +import java.time.Instant; import java.time.LocalDateTime; +import java.time.OffsetDateTime; +import java.time.ZoneId; +import java.time.ZonedDateTime; import java.util.Calendar; +import java.util.Objects; import java.util.TimeZone; import java.util.concurrent.TimeUnit; import java.util.function.Supplier; @@ -198,6 +203,82 @@ public void testShouldGetTimestampReturnValidTimestampWithCalendar( }); } + @ParameterizedTest + @MethodSource("data") + public void testShouldGetObjectReturnValidLocalDateTime( + Supplier vectorSupplier, String vectorType, String timeZone) + throws Exception { + setup(vectorSupplier); + final String expectedTimeZone = Objects.requireNonNullElse(timeZone, "UTC"); + + accessorIterator.iterate( + vector, + (accessor, currentRow) -> { + final LocalDateTime value = accessor.getObject(LocalDateTime.class); + + assertThat( + value, equalTo(getZonedDateTime(currentRow, expectedTimeZone).toLocalDateTime())); + assertThat(accessor.wasNull(), is(false)); + }); + } + + @ParameterizedTest + @MethodSource("data") + public void testShouldGetObjectReturnValidInstant( + Supplier vectorSupplier, String vectorType, String timeZone) + throws Exception { + setup(vectorSupplier); + final String expectedTimeZone = Objects.requireNonNullElse(timeZone, "UTC"); + + accessorIterator.iterate( + vector, + (accessor, currentRow) -> { + final Instant value = accessor.getObject(Instant.class); + + assertThat(value, equalTo(getZonedDateTime(currentRow, expectedTimeZone).toInstant())); + assertThat(accessor.wasNull(), is(false)); + }); + } + + @ParameterizedTest + @MethodSource("data") + public void testShouldGetObjectReturnValidOffsetDateTime( + Supplier vectorSupplier, String vectorType, String timeZone) + throws Exception { + setup(vectorSupplier); + final String expectedTimeZone = Objects.requireNonNullElse(timeZone, "UTC"); + + accessorIterator.iterate( + vector, + (accessor, currentRow) -> { + final OffsetDateTime value = accessor.getObject(OffsetDateTime.class); + final OffsetDateTime vectorValue = + getZonedDateTime(currentRow, expectedTimeZone).toOffsetDateTime(); + assertThat(value, equalTo(vectorValue)); + assertThat(value.getOffset(), equalTo(vectorValue.getOffset())); + assertThat(accessor.wasNull(), is(false)); + }); + } + + @ParameterizedTest + @MethodSource("data") + public void testShouldGetObjectReturnValidZonedDateTime( + Supplier vectorSupplier, String vectorType, String timeZone) + throws Exception { + setup(vectorSupplier); + final String expectedTimeZone = Objects.requireNonNullElse(timeZone, "UTC"); + + accessorIterator.iterate( + vector, + (accessor, currentRow) -> { + final ZonedDateTime value = accessor.getObject(ZonedDateTime.class); + + assertThat(value, equalTo(getZonedDateTime(currentRow, expectedTimeZone))); + assertThat(value.getZone(), equalTo(ZoneId.of(expectedTimeZone))); + assertThat(accessor.wasNull(), is(false)); + }); + } + @ParameterizedTest @MethodSource("data") public void testShouldGetTimestampReturnNull(Supplier vectorSupplier) { @@ -319,6 +400,24 @@ private Timestamp getTimestampForVector(int currentRow, String timeZone) { return expectedTimestamp; } + private ZonedDateTime getZonedDateTime(int currentRow, String timeZone) { + Object object = vector.getObject(currentRow); + TimeZone tz = TimeZone.getTimeZone(timeZone); + ZonedDateTime expectedTimestamp = null; + if (object instanceof LocalDateTime) { + expectedTimestamp = ((LocalDateTime) object).atZone(tz.toZoneId()); + } else if (object instanceof Long) { + TimeUnit timeUnit = getTimeUnitForVector(vector); + long millis = timeUnit.toMillis((Long) object); + long offset = tz.getOffset(millis); + // TODO: should we actually add the offset here? I'm not completely sure how the value is + // stored in the vector + LocalDateTime local = new Timestamp(millis + offset).toLocalDateTime(); + expectedTimestamp = ZonedDateTime.of(local, tz.toZoneId()); + } + return expectedTimestamp; + } + @ParameterizedTest @MethodSource("data") public void testShouldGetObjectClass(Supplier vectorSupplier) throws Exception { From 2ba36cc48a8625386230ecd9a0811740650572e8 Mon Sep 17 00:00:00 2001 From: Diego Fernandez Date: Sun, 2 Mar 2025 18:12:31 -0700 Subject: [PATCH 08/13] Minor test tweaks --- ...wFlightJdbcTimeStampVectorAccessorTest.java | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java index a69141308..074d19668 100644 --- a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java +++ b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java @@ -215,9 +215,10 @@ public void testShouldGetObjectReturnValidLocalDateTime( vector, (accessor, currentRow) -> { final LocalDateTime value = accessor.getObject(LocalDateTime.class); + final LocalDateTime expectedValue = + getZonedDateTime(currentRow, expectedTimeZone).toLocalDateTime(); - assertThat( - value, equalTo(getZonedDateTime(currentRow, expectedTimeZone).toLocalDateTime())); + assertThat(value, equalTo(expectedValue)); assertThat(accessor.wasNull(), is(false)); }); } @@ -234,8 +235,9 @@ public void testShouldGetObjectReturnValidInstant( vector, (accessor, currentRow) -> { final Instant value = accessor.getObject(Instant.class); + final Instant expectedValue = getZonedDateTime(currentRow, expectedTimeZone).toInstant(); - assertThat(value, equalTo(getZonedDateTime(currentRow, expectedTimeZone).toInstant())); + assertThat(value, equalTo(expectedValue)); assertThat(accessor.wasNull(), is(false)); }); } @@ -252,10 +254,11 @@ public void testShouldGetObjectReturnValidOffsetDateTime( vector, (accessor, currentRow) -> { final OffsetDateTime value = accessor.getObject(OffsetDateTime.class); - final OffsetDateTime vectorValue = + final OffsetDateTime expectedValue = getZonedDateTime(currentRow, expectedTimeZone).toOffsetDateTime(); - assertThat(value, equalTo(vectorValue)); - assertThat(value.getOffset(), equalTo(vectorValue.getOffset())); + + assertThat(value, equalTo(expectedValue)); + assertThat(value.getOffset(), equalTo(expectedValue.getOffset())); assertThat(accessor.wasNull(), is(false)); }); } @@ -272,8 +275,9 @@ public void testShouldGetObjectReturnValidZonedDateTime( vector, (accessor, currentRow) -> { final ZonedDateTime value = accessor.getObject(ZonedDateTime.class); + final ZonedDateTime expectedValue = getZonedDateTime(currentRow, expectedTimeZone); - assertThat(value, equalTo(getZonedDateTime(currentRow, expectedTimeZone))); + assertThat(value, equalTo(expectedValue)); assertThat(value.getZone(), equalTo(ZoneId.of(expectedTimeZone))); assertThat(accessor.wasNull(), is(false)); }); From 6435f994f2e80944ba306aadfb76336af803f963 Mon Sep 17 00:00:00 2001 From: Diego Fernandez Date: Sun, 2 Mar 2025 18:26:21 -0700 Subject: [PATCH 09/13] Use Instant to generate ZonedDateTime --- .../ArrowFlightJdbcTimeStampVectorAccessorTest.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java index 074d19668..73927eed5 100644 --- a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java +++ b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java @@ -412,12 +412,8 @@ private ZonedDateTime getZonedDateTime(int currentRow, String timeZone) { expectedTimestamp = ((LocalDateTime) object).atZone(tz.toZoneId()); } else if (object instanceof Long) { TimeUnit timeUnit = getTimeUnitForVector(vector); - long millis = timeUnit.toMillis((Long) object); - long offset = tz.getOffset(millis); - // TODO: should we actually add the offset here? I'm not completely sure how the value is - // stored in the vector - LocalDateTime local = new Timestamp(millis + offset).toLocalDateTime(); - expectedTimestamp = ZonedDateTime.of(local, tz.toZoneId()); + Instant instant = Instant.ofEpochMilli(timeUnit.toMillis((Long) object)); + expectedTimestamp = ZonedDateTime.ofInstant(instant, tz.toZoneId()); } return expectedTimestamp; } From f910063e283529f7664b48936e0af76d66455c7a Mon Sep 17 00:00:00 2001 From: Diego Fernandez Date: Sun, 2 Mar 2025 18:28:02 -0700 Subject: [PATCH 10/13] Add docstring to test method --- .../calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java index 73927eed5..6e29d28e8 100644 --- a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java +++ b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java @@ -404,6 +404,7 @@ private Timestamp getTimestampForVector(int currentRow, String timeZone) { return expectedTimestamp; } + /** ZonedDateTime contains all necessary information to generate any java.time object. */ private ZonedDateTime getZonedDateTime(int currentRow, String timeZone) { Object object = vector.getObject(currentRow); TimeZone tz = TimeZone.getTimeZone(timeZone); From ea11d7b1ca8ca5d728eb0552a7cbf7a9184e275e Mon Sep 17 00:00:00 2001 From: Diego Fernandez Date: Wed, 23 Apr 2025 22:59:52 -0600 Subject: [PATCH 11/13] Pr feedback and improved exceptions --- .../ArrowFlightJdbcDateVectorAccessor.java | 2 +- .../ArrowFlightJdbcTimeStampVectorAccessor.java | 16 ++++++++++------ .../ArrowFlightJdbcTimeVectorAccessor.java | 2 +- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java index f696e5c0a..cdafeffc3 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java @@ -95,7 +95,7 @@ public T getObject(final Class type) throws SQLException { } else if (type == Date.class) { value = getObject(); } else { - throw new SQLException("invalid class"); + throw new SQLException("Object type not supported for Date Vector"); } return !type.isPrimitive() && wasNull ? null : type.cast(value); } diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java index bf4428d65..d43698a23 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java @@ -32,6 +32,7 @@ import java.time.temporal.ChronoUnit; import java.util.Calendar; import java.util.Objects; +import java.util.Set; import java.util.TimeZone; import java.util.concurrent.TimeUnit; import java.util.function.IntSupplier; @@ -81,7 +82,9 @@ public Class getObjectClass() { @Override public T getObject(final Class type) throws SQLException { final Object value; - if (type == OffsetDateTime.class) { + if (!this.isZoned & Set.of(OffsetDateTime.class, ZonedDateTime.class, Instant.class).contains(type)) { + throw new SQLException("Vectors without timezones can't be converted to objects with offset/tz info."); + } else if (type == OffsetDateTime.class) { value = getOffsetDateTime(); } else if (type == LocalDateTime.class) { value = getLocalDateTime(null); @@ -92,8 +95,9 @@ public T getObject(final Class type) throws SQLException { } else if (type == Timestamp.class) { value = getObject(); } else { - throw new SQLException("invalid class"); + throw new SQLException("Object type not supported for TimeStamp Vector"); } + return !type.isPrimitive() && wasNull ? null : type.cast(value); } @@ -160,7 +164,7 @@ public Date getDate(Calendar calendar) { return null; } - return new Date(getTimstampWithOffset(calendar, localDateTime).getTime()); + return new Date(getTimestampWithOffset(calendar, localDateTime).getTime()); } @Override @@ -170,7 +174,7 @@ public Time getTime(Calendar calendar) { return null; } - return new Time(getTimstampWithOffset(calendar, localDateTime).getTime()); + return new Time(getTimestampWithOffset(calendar, localDateTime).getTime()); } @Override @@ -180,7 +184,7 @@ public Timestamp getTimestamp(Calendar calendar) { return null; } - return getTimstampWithOffset(calendar, localDateTime); + return getTimestampWithOffset(calendar, localDateTime); } /** @@ -190,7 +194,7 @@ public Timestamp getTimestamp(Calendar calendar) { * vector includes TZ info. In order to maintain backward compatibility, we apply the offset if * needed for getDate, getTime, and getTimestamp. */ - private Timestamp getTimstampWithOffset(Calendar calendar, LocalDateTime localDateTime) { + private Timestamp getTimestampWithOffset(Calendar calendar, LocalDateTime localDateTime) { if (calendar != null && !isZoned) { TimeZone timeZone = calendar.getTimeZone(); long millis = Timestamp.valueOf(localDateTime).getTime(); diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorAccessor.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorAccessor.java index 315d36b2f..d525c2fdd 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorAccessor.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorAccessor.java @@ -131,7 +131,7 @@ public T getObject(final Class type) throws SQLException { } else if (type == Time.class) { value = getObject(); } else { - throw new SQLException("invalid class"); + throw new SQLException("Object type not supported for Time Vector"); } return !type.isPrimitive() && wasNull ? null : type.cast(value); } From f0e28a6630fe1bc208f949a6ada6d27c1e7ec68c Mon Sep 17 00:00:00 2001 From: Diego Fernandez Date: Thu, 24 Apr 2025 08:20:27 -0600 Subject: [PATCH 12/13] Spotless fix --- .../calendar/ArrowFlightJdbcTimeStampVectorAccessor.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java index d43698a23..813fbc7cf 100644 --- a/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java +++ b/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java @@ -82,8 +82,10 @@ public Class getObjectClass() { @Override public T getObject(final Class type) throws SQLException { final Object value; - if (!this.isZoned & Set.of(OffsetDateTime.class, ZonedDateTime.class, Instant.class).contains(type)) { - throw new SQLException("Vectors without timezones can't be converted to objects with offset/tz info."); + if (!this.isZoned + & Set.of(OffsetDateTime.class, ZonedDateTime.class, Instant.class).contains(type)) { + throw new SQLException( + "Vectors without timezones can't be converted to objects with offset/tz info."); } else if (type == OffsetDateTime.class) { value = getOffsetDateTime(); } else if (type == LocalDateTime.class) { From 21e72d5d6f64c3c94066b9cef5850595e32b1d7d Mon Sep 17 00:00:00 2001 From: Diego Fernandez Date: Sun, 27 Apr 2025 09:52:59 -0600 Subject: [PATCH 13/13] Fix tests --- ...FlightJdbcTimeStampVectorAccessorTest.java | 51 ++++++++++++------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java index 6e29d28e8..e4863bd80 100644 --- a/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java +++ b/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java @@ -20,8 +20,10 @@ import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; import java.sql.Date; +import java.sql.SQLException; import java.sql.Time; import java.sql.Timestamp; import java.time.Instant; @@ -230,15 +232,20 @@ public void testShouldGetObjectReturnValidInstant( throws Exception { setup(vectorSupplier); final String expectedTimeZone = Objects.requireNonNullElse(timeZone, "UTC"); - + final boolean vectorHasTz = timeZone != null; accessorIterator.iterate( vector, (accessor, currentRow) -> { - final Instant value = accessor.getObject(Instant.class); - final Instant expectedValue = getZonedDateTime(currentRow, expectedTimeZone).toInstant(); + if (vectorHasTz) { + final Instant value = accessor.getObject(Instant.class); + final Instant expectedValue = + getZonedDateTime(currentRow, expectedTimeZone).toInstant(); - assertThat(value, equalTo(expectedValue)); - assertThat(accessor.wasNull(), is(false)); + assertThat(value, equalTo(expectedValue)); + assertThat(accessor.wasNull(), is(false)); + } else { + assertThrows(SQLException.class, () -> accessor.getObject(Instant.class)); + } }); } @@ -249,17 +256,21 @@ public void testShouldGetObjectReturnValidOffsetDateTime( throws Exception { setup(vectorSupplier); final String expectedTimeZone = Objects.requireNonNullElse(timeZone, "UTC"); - + final boolean vectorHasTz = timeZone != null; accessorIterator.iterate( vector, (accessor, currentRow) -> { - final OffsetDateTime value = accessor.getObject(OffsetDateTime.class); - final OffsetDateTime expectedValue = - getZonedDateTime(currentRow, expectedTimeZone).toOffsetDateTime(); + if (vectorHasTz) { + final OffsetDateTime value = accessor.getObject(OffsetDateTime.class); + final OffsetDateTime expectedValue = + getZonedDateTime(currentRow, expectedTimeZone).toOffsetDateTime(); - assertThat(value, equalTo(expectedValue)); - assertThat(value.getOffset(), equalTo(expectedValue.getOffset())); - assertThat(accessor.wasNull(), is(false)); + assertThat(value, equalTo(expectedValue)); + assertThat(value.getOffset(), equalTo(expectedValue.getOffset())); + assertThat(accessor.wasNull(), is(false)); + } else { + assertThrows(SQLException.class, () -> accessor.getObject(OffsetDateTime.class)); + } }); } @@ -270,16 +281,20 @@ public void testShouldGetObjectReturnValidZonedDateTime( throws Exception { setup(vectorSupplier); final String expectedTimeZone = Objects.requireNonNullElse(timeZone, "UTC"); - + final boolean vectorHasTz = timeZone != null; accessorIterator.iterate( vector, (accessor, currentRow) -> { - final ZonedDateTime value = accessor.getObject(ZonedDateTime.class); - final ZonedDateTime expectedValue = getZonedDateTime(currentRow, expectedTimeZone); + if (vectorHasTz) { + final ZonedDateTime value = accessor.getObject(ZonedDateTime.class); + final ZonedDateTime expectedValue = getZonedDateTime(currentRow, expectedTimeZone); - assertThat(value, equalTo(expectedValue)); - assertThat(value.getZone(), equalTo(ZoneId.of(expectedTimeZone))); - assertThat(accessor.wasNull(), is(false)); + assertThat(value, equalTo(expectedValue)); + assertThat(value.getZone(), equalTo(ZoneId.of(expectedTimeZone))); + assertThat(accessor.wasNull(), is(false)); + } else { + assertThrows(SQLException.class, () -> accessor.getObject(ZonedDateTime.class)); + } }); }