Skip to content

Update :utc_datetime handling#49

Merged
warmwaffles merged 2 commits into
elixir-sqlite:mainfrom
LostKobrakai:pr/align-utc-datetime-handling
Aug 25, 2021
Merged

Update :utc_datetime handling#49
warmwaffles merged 2 commits into
elixir-sqlite:mainfrom
LostKobrakai:pr/align-utc-datetime-handling

Conversation

@LostKobrakai
Copy link
Copy Markdown
Contributor

@LostKobrakai LostKobrakai commented Aug 25, 2021

This changes the handling of :utc_datetime to not write offsets to the db and ignore them when fetching.

I'm wondering if this one should be changed as well though.
https://github.com/elixir-sqlite/exqlite/blob/a173dc87e6094a7beb535773b989d596461a3a75/lib/exqlite/sqlite3.ex#L170

It seems neither postgrex nor myxql support %DateTime{}s not being supplied in UTC:
Postgrex:
https://github.com/elixir-ecto/postgrex/blob/60abc91d7d64155e9d5fe587178e063b1083f668/lib/postgrex/extensions/timestamptz.ex#L52-L54
MyXQL:
https://github.com/elixir-ecto/myxql/blob/fdb147dba07d7699c4af23448996ae7f4663bce6/lib/myxql/protocol/values.ex#L341-L343

The mssql/tds driver seems to have a type for datetimes with offsets.
https://github.com/livehelpnow/tds/blob/36f228275c2bfde2baa45fe70d29d1eb1d44379b/lib/tds/types.ex#L1775-L1788
Usable with custom ecto types it seems: elixir-ecto/tds#111 (comment)

This PR will currently ignore any offsets, given there's no clear path on how to handle the offset if there is one.

Closes #46

@warmwaffles
Copy link
Copy Markdown
Member

I'm wondering if this one should be changed as well though.
https://github.com/elixir-sqlite/exqlite/blob/a173dc87e6094a7beb535773b989d596461a3a75/lib/exqlite/sqlite3.ex#L170

Yes we should probably alter that there as well.

This PR will currently ignore any offsets, given there's no clear path on how to handle the offset if there is one.

In theory SQLite3 should shift the offsets upon writing the record to UTC. Not 100% certain on that, but I swear I remember reading it in the sqlite documentation.

@LostKobrakai
Copy link
Copy Markdown
Contributor Author

Maybe here:

Formats 2 through 10 may be optionally followed by a timezone indicator of the form "[+-]HH:MM" or just "Z". The date and time functions use UTC or "zulu" time internally, and so the "Z" suffix is a no-op. Any non-zero "HH:MM" suffix is subtracted from the indicated date and time in order to compute zulu time. For example, all of the following time values are equivalent:

https://www.sqlite.org/lang_datefunc.html

@LostKobrakai LostKobrakai force-pushed the pr/align-utc-datetime-handling branch from 2ee6287 to 5dd8eac Compare August 25, 2021 12:57
@warmwaffles
Copy link
Copy Markdown
Member

Yea that's where I saw it. 👍🏻

@warmwaffles warmwaffles merged commit 51b4645 into elixir-sqlite:main Aug 25, 2021
@warmwaffles
Copy link
Copy Markdown
Member

@LostKobrakai if you can, will you go update the exqlite handling of datetimes?

@LostKobrakai
Copy link
Copy Markdown
Contributor Author

I can do that, I'm just not sure if I should go with an Exception for non UTC datetimes like the other adapters, or an error tuple like everything else seems to be.

@warmwaffles
Copy link
Copy Markdown
Member

I modeled both this adapter and the client library to be close to how MyXQL handles things. If either postgrex or myxql raise an exception for that, let's do that to keep parity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

:utc_datetime handling

2 participants