Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 148
GH-463: Improve TZ support for JDBC driver#464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
be59f2f
Improve TZ support for JDBC driver
aiguofer d3321d1
Add notes and minor cleanup
aiguofer 26e64c6
Reformat code
aiguofer 2439197
Fix tests
aiguofer 3ed198a
Undo breaking change for ArrowFlightJdbcTimeStampVectorAccessor.getTi…
aiguofer ee121e5
Also fix getTime and getDate
aiguofer a2a7930
Add unit tests for java.time objects for TimeStampVectorAccessor
aiguofer 2ba36cc
Minor test tweaks
aiguofer 6435f99
Use Instant to generate ZonedDateTime
aiguofer f910063
Add docstring to test method
aiguofer ea11d7b
Pr feedback and improved exceptions
aiguofer f0e28a6
Spotless fix
aiguofer 21e72d5
Fix tests
aiguofer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
19 changes: 19 additions & 0 deletions
19 ...rg/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorAccessor.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
101 changes: 93 additions & 8 deletions
101 ...ache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -21,11 +21,18 @@ | ||
| 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.Objects; | ||
| import java.util.Set; | ||
| import java.util.TimeZone; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.function.IntSupplier; | ||
| @@ -43,6 +50,7 @@ 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. */ | ||
| interface LongToLocalDateTime { | ||
| @@ -58,6 +66,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); | ||
| @@ -68,11 +79,62 @@ public Class<?> getObjectClass() { | ||
| return Timestamp.class; | ||
| } | ||
| @Override | ||
| public <T> T getObject(final Class<T> 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."); | ||
| } else 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("Object type not supported for TimeStamp Vector"); | ||
| } | ||
| return !type.isPrimitive() && wasNull ? null : type.cast(value); | ||
| } | ||
| @Override | ||
| public Object getObject() { | ||
| return this.getTimestamp(null); | ||
| } | ||
| private ZonedDateTime getZonedDateTime() { | ||
aiguofer marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| 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 +147,9 @@ 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 = | ||
| @@ -102,7 +166,7 @@ public Date getDate(Calendar calendar) { | ||
| return null; | ||
| } | ||
| return new Date(Timestamp.valueOf(localDateTime).getTime()); | ||
| return new Date(getTimestampWithOffset(calendar, localDateTime).getTime()); | ||
| } | ||
| @Override | ||
| @@ -112,7 +176,7 @@ public Time getTime(Calendar calendar) { | ||
| return null; | ||
| } | ||
| return new Time(Timestamp.valueOf(localDateTime).getTime()); | ||
| return new Time(getTimestampWithOffset(calendar, localDateTime).getTime()); | ||
| } | ||
| @Override | ||
| @@ -122,6 +186,24 @@ public Timestamp getTimestamp(Calendar calendar) { | ||
| return null; | ||
| } | ||
| return getTimestampWithOffset(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 getTimestampWithOffset(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); | ||
| } | ||
| @@ -170,11 +252,14 @@ 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); | ||
| } | ||
| protected static boolean getVectorIsZoned(TimeStampVector vector) { | ||
| ArrowType.Timestamp arrowType = | ||
| (ArrowType.Timestamp) vector.getField().getFieldType().getType(); | ||
| return arrowType.getTimezone() != null; | ||
| } | ||
| } | ||
19 changes: 19 additions & 0 deletions
19 ...rg/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorAccessor.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 changes: 7 additions & 1 deletion
8 flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/SqlTypes.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 changes: 3 additions & 2 deletions
5 ...t-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ArrowDatabaseMetadataTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.