-
-
Notifications
You must be signed in to change notification settings - Fork 443
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
Add Jiff support #1164
Add Jiff support #1164
Conversation
postgres-types/src/jiff_01.rs
Outdated
fn from_sql(type_: &Type, raw: &[u8]) -> Result<JiffTimestamp, Box<dyn Error + Sync + Send>> { | ||
Ok(DateTime::from_sql(type_, raw)? | ||
.to_zoned(TimeZone::UTC)? | ||
.timestamp()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not intricately familiar with how PostgreSQL represents time, but it seems like you can avoid going through a DateTime
here and just compute a JiffTimestamp
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in f00d208
postgres-types/src/jiff_01.rs
Outdated
|
||
impl<'a> FromSql<'a> for Zoned { | ||
fn from_sql(type_: &Type, raw: &[u8]) -> Result<Zoned, Box<dyn Error + Sync + Send>> { | ||
Ok(JiffTimestamp::from_sql(type_, raw)?.to_zoned(TimeZone::UTC)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct to assume UTC here? There is no time zone information in PostgreSQL?
I wonder if it makes sense to leave out the impl for Zoned
and instead require that callers just use a jiff::Timestamp
and do their own conversions to Zoned
as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UTC is correct. From the Postgres docs:
For
timestamp
with time zone, the internally stored value is always in UTC... An input value that has an explicit time zone specified is converted to UTC using the appropriate offset for that time zone. If no time zone is stated in the input string, then it is assumed to be in the time zone indicated by the system's TimeZone parameter, and is converted to UTC using the offset for the timezone zone.
Consumers will usually reach for Timestamp
, but Zoned
being ToSql
and FromSql
can occasionally be convenient. I am in favour of including them. It would also match chrono::DateTime<FixedOffset>
and time::OffsetDateTime
having them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm skeptical that a Zoned
implementation makes sense here personally. Think of a Zoned
as "an assertion that a particular datetime is in a specific geographic region." Just because you can pick some kind of default here doesn't mean you should. And the civil datetime types are also somewhat questionable given that you're freely going to and from instants in time and back to inexact time. That also seems wrong.
I understand you're starting from a position of "this is how the Chrono integration works," but Chrono's DateTime<FixedOffset>
and time's OffsetDateTime
are much closer to Jiff's Timestamp
than Jiff's Zoned
. The closest analog Chrono has to a Jiff Zoned
is DateTime<chrono_tz::Tz>
.
The only trait implementation I feel 100% confident about here is for jiff::Timestamp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have always felt weird about how Postgres handles time zones and how the timestamp with time zone
type does not in fact contain the time zone information. Keeping the actual geographic region requires an additional column. What you wrote about Zoned
resonates with me when it comes to correctness. I agree that jiff::Timestamp
matches up with timestamptz
and Zoned
does not match with any current Postgres datetime type. I was concerned about conforming to Postgres and this crate's implementation details but I agree with you now on omitting impl for Zoned
.
I am not sure I agree with your civil type concerns though. I feel like the Postgres types match up quite nicely for these inexact expressions of time
(Postgres also has a timetz
. Can we express that in Jiff?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RE Zoned
: okay, great. Thank you.
I am not sure I agree with your civil type concerns though. I feel like the Postgres types match up quite nicely for these inexact expressions of time
I'm going based on the code in this PR. What I see is that a PostgreSQL timestamp (which is exact time) is being convert to and from a Jiff civil::DateTime
, which is inexact time. Going from inexact time to exact time should require some assertion of time zone. Indeed, if you add a time zone to a civil::DateTime
, then you get a Zoned
. The same reason that a Zoned
impl is inappropriate applies here.
AIUI, PostgreSQL does have Date
and Time
types, and those are inexact times, just like Jiff's civil::Date
and civil::Time
types.
Now that I look closer at this patch, I see that a DateTime
is being constructed from a timestamp, while Date
and Time
are being constructed from PostgreSQL date
and time
types. The fact that these are different is 100% certainly wrong. I don't think PostgreSQL has a datetime
type (something that combines date
and time
), so probably the jiff::civil::DateTime
implementation should be removed.
Basically, any time you move between exact and inexact time, you should have some kind of time zone assertion involved. And usually, "just use UTC" is not the right thing to do.
(Postgres also has a timetz. Can we express that in Jiff?)
Noooooooooo. See: https://wiki.postgresql.org/wiki/Don%27t_Do_This#Don.27t_use_timetz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timestamp
in Postgres is inexact, being a combination of date
and time
without time zone. It can be used to record the general idea of the 2025 New Year's Countdown, taking place on Dec 31, 2024 at 11:59pm, without regard for the geographic location.
timestamptz
is exact., asserting the time zone (albeit in UTC only).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timestamp in Postgres is inexact, being a combination of date and time without time zone.
Whoa. OK. Let me backup and read their docs more carefully... Because if a timestamp
is just inexact time and timestamptz
is just inexact time with a time zone, then that does not make a timestamptz
exact. It could be ambiguous. But according to this, yes, PostgreSQL is saying that it is exact. So maybe they are doing disambiguation on insert or something. And also, this seems to indeed agree that just a timestamp
is actually inexact time:
timestamp (also known as timestamp without time zone) doesn't do any of that, it just stores a date and time you give it. You can think of it being a picture of a calendar and a clock rather than a point in time. Without additional information - the timezone - you don't know what time it records.
Wild.
I had assumed that timestamp
was, well, just a Unix timestamp. Which is always in UTC. And timestamptz
stored some extra bit of information like an offset. OK, so I think this is the mapping then:
date
->jiff::civil::Date
time
->jiff::civil::Time
timestamp
->jiff::civil::DateTime
timestamptz
->jiff::Timestamp
I think where I got confused is that in this patch, the timestamp -> jiff::civil::DateTime
is implemented by using what appears to be a representation for exact time (number of seconds since an epoch). But I guess that's just a representational thing.
And yes, it looks like the above mapping lines up with what you have in this PR. OK, sorry for the noise!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had assumed that
timestamp
was, well, just a Unix timestamp. Which is always in UTC. Andtimestamptz
stored some extra bit of information like an offset. OK, so I think this is the mapping then:
date
->jiff::civil::Date
time
->jiff::civil::Time
timestamp
->jiff::civil::DateTime
timestamptz
->jiff::Timestamp
I think where I got confused is that in this patch, the
timestamp -> jiff::civil::DateTime
is implemented by using what appears to be a representation for exact time (number of seconds since an epoch). But I guess that's just a representational thing.
It's certainly confusing.
timestamp
and timestamptz
both internally store microseconds since Y2K midnight UTC. The difference is that timestamptz
converts the input to UTC before storing the number of microseconds. It bakes in the offset so we get the correct exact time, but the actual source time zone/offset is discarded.
For the record, DateTime
and Timestamp
are much better names than timestamp
and timestamptz
to describe what they really are.
And yes, it looks like the above mapping lines up with what you have in this PR. OK, sorry for the noise!
Comments were much appreciated :) At least we're only dealing with Terran time.
The impl now directly computes `Timestamp` rather than going through `DateTime` and `Zoned`.
`Timestamp` already has impl and is semantically accurate for mapping to `timestamptz`, unlike `Zoned`. End users can do their own conversions from `Timestamp` to `Zoned` if desired.
Can you also add some tests? Here are chrono's for reference: https://github.com/sfackler/rust-postgres/blob/master/tokio-postgres/tests/test/types/chrono_04.rs |
This adds tests in the same fashion as the existing ones for `chrono` and `time`. Overflow is now handled using fallible operations. For example, `Span:microseconds` is replaced with `Span::try_microseconds`. Postgres infinity values are workiing as expected. All tests are passing.
The implementation was previously removed as per jiff author comment.
This PR is complete and ready for review. All tests are passing. One minor note: In the tests, I use jiff's let s = "'1970-01-01 00:00:00.010000000Z'";
let ts: Timestamp = s.trim_matches('\'').parse().unwrap(); // I did this
let ts: Timestamp = Timestamp::strptime("'%Y-%m-%d %H:%M:%S.%f %#z'", s); // I could do this if you prefer Special thanks to @cmarkh who opened allan2/rust-postgres#1, which I unfortunately missed. |
The FromStr impl should be used if possible IMO. It's more flexible (it permits a |
Thanks! |
This PR adds support for version 0.1 of Jiff.
Jiff is inspired by Temporal, a TC39 proposal to improve datetime handling in JavaScript.
The implementation in jiff_01.rs is based on chrono_04.rs. When using
FromSql
andToSql
,Zoned
sets the timezone to UTC.Jiff does not support leap seconds, which is nice because Postgres doesn't either.
Closes #1163