apache/arrow-java

[JDBC] Timezone improvements for the JDBC driver

Closed this issue · 5 comments

aiguofer commented

Describe the enhancement requested

Currently, our Timezone handling is pretty iffy as identified in a few different threads. I've been doing some digging and would like to propose a few changes. I will make a draft PR with how I think we should implement these changes so we can open up discussions on each point.

Support java.time objects

The JDBC 4.2 spec specifies users can fetch java.time objects using resultSet.getObject(..., <java.time>.class). We should support OffsetDateTime, ZonedDateTime, Instant, and LocalDateTime for timestamps. We should also support LocalDate for dates vectors, and LocalTime for times.

Support TIMESTAMP_WITH_TIMEZONE JDBC type

Timestamp vectors that include a timezone should be treated as TIMESTAMP_WITH_TIMEZONE

  • When retrieved as OffsetDateTime or ZonedDateTime, use the TZ included in the vector
  • When retrieved as Timestamp with a Calendar, adjust the value to display time in the TZ specified by the Calendar, assuming the value in the vector is in the vector specified TZ
  • When retrieved as LocalDateTime, do same as Timestamp but assume the Calendar specifies UTC.
  • Still not sure how Instant plays into all of this

Timestamp vectors without a timezone should be treated as TIMESTAMP

  • When retrieved as OffsetDateTime or ZonedDateTime, throw an error?
  • When retrieved as Timestamp or LocalDateTime, take the value as-is and don't do any TZ conversion. The assumption should be that if the vector didn't include TZ, we're just dealing with "wall-clock" time so it should never be adjusted
  • Still not sure how Instant plays into all of this

If we get a new vector type that includes TZ/Offset per record (apache/arrow#44248), it should also be treated as TIMESTAMP_WITH_TIMEZONE

  • This is still a hypothetical, but I think if this comes to fruition we should treat it similar to Timestamp vectors with TZ, but use the TZ per record instead of for the whole vector.

Don't do any TZ adjustments for regular TIMESTAMP w/o TZ

  • We currently assume UTC for vectors w/o a TZ and still do an offset conversion when requested with a given Calendar. We should not do any TZ conversion for these since they should be treated as "wall-clock" time.

Parameter binding

We probably also need to deal with relevant changes when binding parameters, but I'll leave that as a separate change since we also need to deal with #153.

Did I miss anything? Let me know!

aiguofer commented

@jduo would certainly love your feedback since I know you have lots of experience in this space

lidavidm commented

Broadly this sounds right to me

lidavidm commented

I suppose if the user requests an Instant, we can treat it as a special case of requesting a zoned datetime

lidavidm commented

I'm going to file a new issue since this isn't really fixed