Fill the missing date/time related decoding in ReadExecutor #287
Conversation
|
|
|
@robmwalsh could you take a look at this PR? It touches the same area as #263 |
There was a problem hiding this comment.
Thanks for your work on this! Just a couple of minor things to fix up. Apparently ZonedDateTime, Instant and OffsetTime aren't supported by jdbc. We will have to decide how/if we want to support this (this is a jdbc problem, not just postgres), but at the moment I think getting an offset time in local time as a convenience is probably reasonable. I'll create an issue for this Issue #322. I resolved a merge conflict so please pull your branch before making changes. We also need you to please sign the CLA - there are instructions on that above.
| case TOffsetTime => | ||
| tryDecode[OffsetTime]( | ||
| column | ||
| .fold(resultSet.getTimestamp(_), resultSet.getTimestamp(_)) |
There was a problem hiding this comment.
I believe we need resultSet.getTime for this - I'm not sure what calling getTimestamp on a time column would do.
| column | ||
| .fold(resultSet.getTimestamp(_), resultSet.getTimestamp(_)) | ||
| .toLocalDateTime() | ||
| .atOffset(ZoneOffset.of(ZoneId.systemDefault().getId)) |
There was a problem hiding this comment.
According to https://jdbc.postgresql.org/documentation/head/java8-date-time.html, "OffsetDateTime instances will have be in UTC (have offset 0). This is because the backend stores them as UTC" - can you please change this to be UTC instead of local time?
There was a problem hiding this comment.
Yes for sure. Thank you for the feedback. See my latest commit.
| @@ -2,7 +2,8 @@ package zio.sql | |||
|
|
|||
| import java.sql._ | |||
| import java.io.IOException | |||
| import java.time.{ ZoneId, ZoneOffset } | |||
| import java.time.{ OffsetDateTime, OffsetTime, ZoneId, ZoneOffset, ZonedDateTime } | |||
There was a problem hiding this comment.
please remove unused import
[error] /home/circleci/project/jdbc/src/main/scala/zio/sql/jdbc.scala:5:68: Unused import
[error] import java.time.{ OffsetDateTime, OffsetTime, ZoneId, ZoneOffset, ZonedDateTime }
[error] ^
|
Thanks, looks looks like just that unused import (this might seem picky but our CI fails for this to keep the imports tidy - 1 isn't too bad but they'd just grow over time if it wasn't enforced) & CLA and this will be good to merge |
|
Sorry about that, I'm working on a new machine and didn't have the setting turned on that flags unused imports so I wasn't seeing it. I appreciate your patience! I work with zio at work but this is my first time contributing to open source. I'm not sure what's wrong with the CLA, I have signed it already under my github account laurachapman but it's showing "LAURA CHAPMAN seems not to be a github user" in the contributers section. Not sure how to resolve this. Worst case I can make a new branch/PR and be sure to sign it there. Let me know and I'll go ahead and do that. |
|
No worries, thanks for helping out! Your new machine is probably the culprit for the CLA issue as well - you need to make sure that the email address that's used by git on your local machine is listed in your github account. Take a look at this article. |
|
looks like I just had to rerun CI to recheck the CLA status. Thanks and congrats on your first contribution! |
…g-issue-127 Fill the missing date/time related decoding in ReadExecutor
Fixes #147