impl To/FromSql<Text, Sqlite> for chrono::DateTime<FixedOffset>

[boustrophedon]

Would you accept PRs for
impl FromSql<Text, Sqlite> for DateTime<FixedOffset> and the corresponding ToSql where the DateTime is stored as either rfc2822 or (probably better) rfc3339 ?

[weiznich]

I would be open to such an addition. RFC3339 seems to be the better variant, as it seems to be supported by sqlite. (Documentation)

[boustrophedon]

I'm sorry, I don't understand the context of the sqlite datetime functions wrt what I am proposing.

I am proposing allowing/using a TEXT column to store chrono::DateTime serialized/deserialized in rfc3339 format. (I do agree with you that 3339 is better, in particular it's sortable) This is desirable because the sqlite DATETIME datatype is a chrono::NaiveDateTime with no timezone data.

For testing, I was planning on just copying the tests here. Maybe also a few tests that create multiple FixedOffset dts from the same Utc time, and checking that they deserialize back to distinct Tzs but are equivalent to the original Utc time.

Does this make sense? If so I'll work on it this weekend.

[weiznich]

That means I should probably explain a bit more in detail what I've meant above. Sqlite fundamentally supports only 5 data types (Null, Integer, Real, Text and Blob). The date time functions I've linked above all work on Text fields, only the encoding of required to be one of that variants listed there. That's the main reason why I've linked that page. It basically says RFC3339 style datetime values are supported, while RFC2822 style values are not supported, which makes using the RFC3339 format the logical choice here.

So for the implementation strategy: Adding tests at the linked location and providing impls above is the right solution.

[boustrophedon]

Oh, you mentioned it just for compatibility purposes, not that I would actually use it in the implementation.

The implementation is essentially the one for Date but with NaiveDate::parse_from_str replaced with DateTime::parse_from_rfc3339.

[boustrophedon]

Well, I'll just write the code and then you can review it :)

[boustrophedon]

I was able to implement To/FromSql easily and will push that for your review, but I'm not sure how to proceed to implement the SelectDsl etc. traits.

The two features I think would be most useful would be select(datetime(dsl::timestamp_col).lt(chrono_datetime)) and select(datetime(dsl::timestamp_col)).order(dsl::timestamp_col.desc()) i.e. selecting rows more/less recent than a given DateTime, and ordering by timestamp. It's also possible to order by timestamp without using the sqlite datetime builtin if all the timezones (and timestamp precision) are the same.

I'm not really sure how the code works but I think the derives here: https://github.com/diesel-rs/diesel/blob/master/diesel/src/type_impls/date_and_time.rs are how these get implemented for the other backends/NaiveDateTime for sqlite?

The problem is which sql type to use, since sqlite doesn't have an official TimestampTz type. However, since sqlite is so weakly typed, we could actually just pretend it does have a TimestampTz type and give it the rfc3339 strings to store as text, and all the builtin datetime functions will work with it. We would need to indicate somewhere in the documentation that in your schema you need to use TIMESTAMPTZ.

I'm not sure if this would be too "non-standard". For my use case, only having store/load functionality is fine and sorting the values outside the database is acceptable.

(sorry for taking so long to get back to this - I was laid off last week 🙁)

[weiznich]

First of all there is no reason to apologize to anyone for taking some time for other things. It is complete voluntarily to work on diesel, so feel free to take whatever time you need.

The problem is which sql type to use, since sqlite doesn't have an official TimestampTz type. However, since sqlite is so weakly typed, we could actually just pretend it does have a TimestampTz type and give it the rfc3339 strings to store as text, and all the builtin datetime functions will work with it. We would need to indicate somewhere in the documentation that in your schema you need to use TIMESTAMPTZ.

I should probably been a bit clearer above. We should use a distinct sql type for this. One way would be to reuse the postgres Timestamptz type here, as it has a similar semantic. That is already done with the normal Timestamp type. By doing so we are basically adding some type system on top of sqlite. Another thing that we should probably clarify here is how types are inferred by the cli. According to this sqlite documentation using a datatype like Timestamp or DateTime will result in a numeric column affinity. Diesel cli will then infer the corresponding diesel sql types here. We should probably adjust that to also infer Timestamptz then.

The two features I think would be most useful would be select(datetime(dsl::timestamp_col).lt(chrono_datetime)) and select(datetime(dsl::timestamp_col)).order(dsl::timestamp_col.desc()) i.e. selecting rows more/less recent than a given DateTime, and ordering by timestamp. It's also possible to order by timestamp without using the sqlite datetime builtin if all the timezones (and timestamp precision) are the same.

That all should just work if we are using Timestamptz as sql type.

I'm not really sure how the code works but I think the derives here: https://github.com/diesel-rs/diesel/blob/master/diesel/src/type_impls/date_and_time.rs are how these get implemented for the other backends/NaiveDateTime for sqlite?

Yes that's the right place. You will need to add an option similar to this one there.

[boustrophedon]

Hi,

I'm sorry for not getting back sooner - was busy with interviewing and then took a vacation.

Unfortunately, my new job has a somewhat strict open source policy, so I don't think I'll be able to follow up on this in the near future.

Would you like to keep this issue open or should I close it?

[weiznich]

For the time being it's fine to keep it open. Maybe someone shows up to finish this work.

[chusteven]

i think i should be able to take a stab at picking up the work within the next week or two; should i open up a new PR or pull down the brach from the currently in-flight draft one? 😅

additionally i think i mostly understand what needs be done -- i'm inclined to agree with you @weiznich that we should just use a new type rather than reusing postgres's Timestamptz type (mostly seems to align though there is some funny business with system timezones [see part just a little under selected text]) that i don't think sqlite supports (just plain old +/- hrs or Z for the default UTC timezone); i'm less sure about the CLI stuff but if i understand correctly, we need to override the sqlite affinity for datetime as numeric values to coerce it to use text ones in this case?

[weiznich]

i think i should be able to take a stab at picking up the work within the next week or two; should i open up a new PR or pull down the brach from the currently in-flight draft one? 😅

Creating a new branch should be fine here. Also easier given how old the linked PR is.

additionally i think i mostly understand what needs be done -- i'm inclined to agree with you @weiznich that we should just use a new type rather than reusing postgres's Timestamptz type (mostly seems to align though there is some funny business with system timezones [see part just a little under selected text]) that i don't think sqlite supports (just plain old +/- hrs or Z for the default UTC timezone);

Yes using a different sql type there should be the correct solution. Feel free to reach out if you hit a problem while implementing this.

i'm less sure about the CLI stuff but if i understand correctly, we need to override the sqlite affinity for datetime as numeric values to coerce it to use text ones in this case?

The mapping is done here:

diesel/diesel_cli/src/infer_schema_internals/sqlite.rs

Line 189 in ad79c72

pub fn determine_column_type(

Basically we would need to just create some rule what exactly should be treated as timestamp with timezone. We might just use Timestamptz or something similar there.