Skip to content
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

[JDBC] Timezone improvements for the JDBC driver #463

Open
aiguofer opened this issue Dec 24, 2024 · 1 comment
Open

[JDBC] Timezone improvements for the JDBC driver #463

aiguofer opened this issue Dec 24, 2024 · 1 comment
Labels
Type: enhancement New feature or request

Comments

@aiguofer
Copy link
Contributor

aiguofer commented Dec 24, 2024

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant