search

zio / zio-quill

Compile-time Language Integrated Queries for Scala
https://zio.dev/zio-quill
Apache License 2.0
2.15k stars 348 forks source link

Wrong handling of `Instant` timestamps #3067

Open michalkoza opened 3 months ago

michalkoza commented 3 months ago

Version: 4.5.0-4.8.5 Module: quill-jdbc Database: postgres 16.3

Expected behavior

Inserting timestamp of type Instant, to column of type TIMEZONE should store exact Instant value, without introducing timezone offsets

Actual behavior

being e.g. in CET/CEST timezone and storing Instant.ofEpochSecond(1L) stores in DB: 1970-01-01 01:00:01.000000 instead of 1970-01-01 00:00:01.000000 1 hr difference while in CET

storing Instant.parse("2018-04-08T19:52:29Z") stores in DB: 2018-04-08 21:52:29.000000 instead of 2018-04-08 19:52:29.000000 2 hr difference while in CEST

Issue introduction

The issue has been introduced here: https://github.com/zio/zio-quill/commit/8117a0ea257c4cf57f131cf64effd47f323b1b0a#diff-5161e16f0f0a107bd28613fe49aa063e2e7390ec87a61c5a8fd540ca191f67beR83

Workaround

Storing Instants in TIMESTAMP with time zone columns seems to work. But it is very confusing.

Reasoning

Instant type is supposed to be a time-zone agnostic moment in time. The same goes for the TIMESTAMP type in Postgres. Binding time-zone agnostic type with explicitly time-zone aware TIMESTAMP with time zone type seems to be a mistake.

@getquill/maintainers

vladimir-lu commented 3 months ago

Is your JVM running in the CEST tz? JDBC drivers typically pick the current machine timezone, which unfortunately leads to such interactions. In the commit you linked, Instant is converted to a java.sql.Timestamp. There's still an open discussion in pgjdbc about supporting Instant natively, e.g. https://github.com/pgjdbc/pgjdbc/issues/1325#issuecomment-438847217

Also see that in https://github.com/zio/zio-jdbc/pull/182 you probably need to use TIMESTAMP WITH TIME ZONE if you want the correct behavior currently.

michalkoza commented 3 months ago

Thank you @vladimir-lu ! But is it really converted to java.sql.Timestamp? It seems more that it is actually converted to OffsetDateTime in this line https://github.com/zio/zio-quill/commit/8117a0ea257c4cf57f131cf64effd47f323b1b0a#diff-5161e16f0f0a107bd28613fe49aa063e2e7390ec87a61c5a8fd540ca191f67beR101

I also reached the conclusion that I need to have TIMESTAMP WITH TIME ZONE (see Workaround), but it is very disappointing and counter intuitive (see Reasoning)