Skip to content
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

Allow handling timezone-aware datetime values when inserting or updating #557

Merged
merged 3 commits into from
Jul 4, 2023

Conversation

amotl
Copy link
Member

@amotl amotl commented Jun 26, 2023

Problem

As reported at #626, when inserting timezone-aware datetime values, the CrateJsonEncoder fails with

TypeError: can't subtract offset-naive and offset-aware datetimes

This patch intends to fix it.

Reference

@amotl amotl requested review from mfussenegger and matriv June 26, 2023 08:36
@amotl amotl marked this pull request as ready for review June 26, 2023 08:36
@amotl amotl force-pushed the amo/aware-datetimes branch from 947b470 to 48fe6a8 Compare June 26, 2023 08:38
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Left a comment for the changelog

CHANGES.txt Outdated Show resolved Hide resolved
@amotl amotl force-pushed the amo/aware-datetimes branch from 48fe6a8 to 55192cf Compare June 26, 2023 11:21
@amotl amotl force-pushed the amo/aware-datetimes branch 2 times, most recently from 911765c to e7ad5b5 Compare June 26, 2023 11:52
@amotl amotl requested review from mfussenegger and matriv June 26, 2023 11:54
Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be worth also extending the data type section in the docs and explain the behavior of datetime with tagged timezone.

One could assume that it persists the timezone information as well, but it's instead converting it to a timestamp with 1970-01-01 as epoch. That's an important distinction

docs/by-example/client.rst Outdated Show resolved Hide resolved
@amotl amotl force-pushed the amo/aware-datetimes branch from e7ad5b5 to 6c14e44 Compare June 26, 2023 12:13
Comment on lines 100 to 108
.. NOTE::

Values of ``TIMESTAMP`` columns will always be stored using a ``LONG`` type,
representing the `Unix time`_ (epoch) timestamp, i.e. number of seconds which
have passed since 00:00:00 UTC on Thursday, 1 January 1970.

This means, when inserting or updating records using timezone-aware Python
``datetime`` objects, timezone information will not be preserved. If you
need to store it, you will need to use a separate column.
Copy link
Member Author

@amotl amotl Jun 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mfussenegger: This paragraph intends to reflect your suggestion. Thank you again, and let me know about any improvements, if you think it could be conveyed better.

-- https://crate-python--557.org.readthedocs.build/en/557/data-types.html

@amotl amotl requested a review from mfussenegger June 26, 2023 12:20
@amotl amotl changed the title Allow handling "aware" datetime values when inserting or updating Allow handling timezone-aware datetime values when inserting or updating Jun 26, 2023
@amotl amotl force-pushed the amo/aware-datetimes branch from 6c14e44 to f88d10c Compare June 26, 2023 14:15
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx again, but please wait for @mfussenegger to also check.

CHANGES.txt Outdated Show resolved Hide resolved
Comment on lines 145 to 146
Both when inserting or updating data, values for ``TIMESTAMP`` columns can be obtained
in different formats. Both literal strings and datetime objects are supported.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Both when inserting or updating data, values for ``TIMESTAMP`` columns can be obtained
in different formats. Both literal strings and datetime objects are supported.
Values for ``TIMESTAMP`` columns can be set as either string literal or datetime object::

And then maybe:

"If the datetime object contains timezone information, it is converted to UTC and the timezone information is lost:"

(e.g. do a update returning)

Copy link
Member Author

@amotl amotl Jun 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it [all three variants] contains timezone information, it is converted to UTC, and the timezone information is discarded.

Text has been changed with 677bef3, thanks.

(e.g. do a update returning)

We may elaborate this on another occasion? I will be all ears to adjust the documentation, in order to improve guidance, but I would need a bit of education first.

Comment on lines +148 to +159
>>> import datetime as dt
>>> timestamp_full = "2023-06-26T09:24:00.123+02:00"
>>> timestamp_date = "2023-06-26"
>>> datetime_aware = dt.datetime.fromisoformat("2023-06-26T09:24:00.123+02:00")
>>> datetime_naive = dt.datetime.fromisoformat("2023-06-26T09:24:00.123")
>>> datetime_date = dt.date.fromisoformat("2023-06-26")
>>> cursor.execute("UPDATE locations SET date=? WHERE name='Cloverleaf'", (timestamp_full, ))
>>> cursor.execute("UPDATE locations SET date=? WHERE name='Cloverleaf'", (timestamp_date, ))
>>> cursor.execute("UPDATE locations SET date=? WHERE name='Cloverleaf'", (datetime_aware, ))
>>> cursor.execute("UPDATE locations SET date=? WHERE name='Cloverleaf'", (datetime_naive, ))
>>> cursor.execute("UPDATE locations SET date=? WHERE name='Cloverleaf'", (datetime_date, ))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this includes date as well I guess the sentence above should mention that too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, added with 677bef3.

docs/data-types.rst Outdated Show resolved Hide resolved
@amotl amotl force-pushed the amo/aware-datetimes branch 2 times, most recently from 3ebf25d to 677bef3 Compare June 28, 2023 13:50
@amotl
Copy link
Member Author

amotl commented Jun 28, 2023

@mfussenegger: Thanks for your valuable suggestions. 677bef3 bundles all of them into a corresponding fixup commit.

@amotl amotl force-pushed the amo/aware-datetimes branch from 677bef3 to a494bce Compare June 28, 2023 14:04
@amotl amotl merged commit 4e20913 into master Jul 4, 2023
@amotl amotl deleted the amo/aware-datetimes branch July 4, 2023 08:22
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.

3 participants