Datetime Autoconversion for Timestamptz Columns

[aabmets]

I made this pull request to rectify the issue where Piccolo does not provide a way to tell Timestamptz columns in which timezones they should identify themselves as.

Outtake from PG docs:

Internally, PostgreSQL stores the timestamptz in UTC value.

When you insert a value into a timestamptz column, PostgreSQL converts the timestamptz value into a UTC value and stores the UTC value in the table.
When you retrieve data from a timestamptz column, PostgreSQL converts the UTC value back to the time value of the timezone set by the database server, the user, or the current database connection.

The solutions provided by Postgres as by which it determines the timezone to convert the value back to was not working properly with Piccolo. Hence I made this pull request. It works, please merge. Kinda need it in my enterprise project. @dantownsend

[dantownsend]

Thanks for this. I think it makes sense.

zoneinfo seems to be Python 3.9 and above, so we'll need to add this to requirements.txt for Python 3.8:

https://pypi.org/project/backports.zoneinfo/

[aabmets]

@dantownsend The initial proposal contained some shortcomings which should be fully fixed by the subsequent commits.

[dantownsend]

It's looking good, thanks. It seems like there's some minor linter and test issues.

[sinisaos]

@aabmets Sorry to intrude into this conversation, but first you need to run linters and tests locally with this set of commands, (then you'll see what you need to fix)

1. Run formaters: ./scripts/format.sh
2. Run linters: ./scripts/lint.sh
3. Run the test suite with Sqlite: ./scripts/test-sqlite.sh
4. Run the test suite with Postgres: ./scripts/test-postgres.sh
5. Run the test suite with Cockroach: ./scripts/test-cockroach.sh

Code contribution guide is here and code style is here. Hope this help to pass linters check and tests

[aabmets]

@sinisaos Thanks for the help, I'll try doing that option.

[aabmets]

@sinisaos Okay, i think i got it fixed now. Can we try again the workflow?

[aabmets]

@sinisaos I think i need help with fixing migration errors.
EDIT: I was able to find the location in code where to fix it.

[aabmets]

@dantownsend Okay I think I've done it, the linting and unittests should be fixed now.

[sinisaos]

@aabmets Sorry for the late reply. When I start formatting, a lot of files are not well formatted and to satisfy mypy in Python3.8 I have to # type ignore all from zoneinfo import ZoneInfo lines (another way can be to add zoneinfo to pyproject.toml tool.mypy.overrides). That is not big deal because ./scripts/format.sh and ./scripts/lint.sh will solve that.
Also tests/columns/test_timestamptz.py::TestTimestamptzDefault::test_timestamptz_default does not pass both Sqlite and Postgres. You must wait for @dantownsend to solve these linters and tests problems.

[aabmets]

@sinisaos hey, thanks for also taking a look at possible solutions. I think my latest commits fixed both linting and test issues, now waiting for approval to rerun workflow.

[aabmets]

For crying out loud 😂 why is the god damn linter test suite failing. I ran the linting and style commands on the commited files.

[aabmets]

@sinisaos Would you be so kind as to re-run the CI workflow, thanks.

[sinisaos]

@aabmets I'm sorry, but I'm not a maintainer and I don't have permission to do that. You have to wait for @dantownsend for that. Daniel, can you please fix this issue with linters as ./scripts/format.sh and ./scripts/lint.sh will fix that. Thanks.

[dantownsend]

@sinisaos yes, will try and do it later tonight. I've given you extra permissions now, so in the future you should be able to approve running CI. GitHub permissions are a bit confusing.

[aabmets]

@dantownsend @sinisaos Hey, at least we're seeing some improvements! I see only one failing test now.
EDIT: I wonder why MyPy is failing on Python 3.8, but successful on other python versions.

[sinisaos]

@aabmets To satisfy mypy in Python3.8 try to # type ignore all from zoneinfo import ZoneInfo lines (another way can be to add zoneinfo to pyproject.toml tool.mypy.overrides) That should work.

I've given you extra permissions now, so in the future you should be able to approve running CI. GitHub permissions are a bit confusing.

@dantownsend Thanks. I also don't know much about Github permissions.

[aabmets]

@sinisaos Done. I couldn't find a solution how to surgically disable MyPy from pyproject.toml so I went with the line-by-line solution. I don't think blanket-ignore would be the correct solution, as that could cause misses on legitimate type errors.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.82%. Comparing base (33cce04) to head (db53939).
Report is 9 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #959      +/-   ##
==========================================
+ Coverage   92.02%   92.82%   +0.80%     
==========================================
  Files         108      108              
  Lines        8246     8207      -39     
==========================================
+ Hits         7588     7618      +30     
+ Misses        658      589      -69     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[aabmets]

@dantownsend @sinisaos
Is it necessary to fix the codecov complaints about this PR?
The fixes for all impacted lines entail adding a #pragma: no cover
to the except clause in ZoneInfo import statements:

try:
    from zoneinfo import ZoneInfo  # type: ignore
except ImportError:  # pragma: no cover  <-- necessary to add to silence codecov
    from backports.zoneinfo import ZoneInfo  # type: ignore  # noqa: F401

[dantownsend]

@aabmets Yes, if you don't mind - we use # pragma: no cover in a few other places in the codebase. I think it makes sense here.

[sinisaos]

@aabmets I think you should remove this imports as it is not used anywhere in the file

[aabmets]

@sinisaos Done

[aabmets]

@dantownsend Not to be a bother, but would it be perchance possible to merge this PR into master and release a minor version in the near future?

[dantownsend]

@aabmets Yes, will do my best to test and release it this week.

@sinisaos Do you mind helping me test this if you get a chance? With timezone stuff it's always best to have an extra pair of eyes on it.

[sinisaos]

@dantownsend Sure, I'll try tomorrow.

[sinisaos]

@dantownsend I tested this PR a bit. objects queries work fine, but select queries do not (results are the same as in the master branch code). Here is an example of the results from Piccolo shell.

# objects query
In [1]: [i.to_dict() for i in await TallinnConcerts.objects()]
Out[1]: 
[{'id': 1,
  'event_start': datetime.datetime(2050, 1, 1, 21, 0, tzinfo=zoneinfo.ZoneInfo(key='Europe/Tallinn'))},
 {'id': 2,
  'event_start': datetime.datetime(2050, 1, 1, 21, 0, tzinfo=zoneinfo.ZoneInfo(key='Europe/Tallinn'))}]
# select queries
In [2]: await TallinnConcerts.select()
Out[2]: 
[{'id': 1,
  'event_start': datetime.datetime(2050, 1, 1, 19, 0, tzinfo=datetime.timezone.utc)},
 {'id': 2,
  'event_start': datetime.datetime(2050, 1, 1, 19, 0, tzinfo=datetime.timezone.utc)}]

I don't know if this is expected behavior (or I'm doing something wrong), but it doesn't match this lines from the docstring.

piccolo/piccolo/columns/column_types.py

Lines 983 to 991 in db53939

	# Query
	>>> await TallinnConcerts.select(TallinnConcerts.event_start)
	{
	'event_start': datetime.datetime(
	2050, 1, 1, 20, 0, tzinfo=zoneinfo.ZoneInfo(
	key='Europe/Tallinn'
	)
	)
	}

[dantownsend]

@sinisaos Thanks for testing it - that's very helpful.

[sinisaos]

@dantownsend I found another problem. When we try to bulk update a column in Piccolo Admin, the fields don't show up in dropdown with the changes from this PR.

bulk_update.webm

[dantownsend]

I've had a chance to test this now. As @sinisaos says, the issue at the moment is with select queries. objects queries work fine. It looks like we're doing the timezone conversion inside Table.__init__, which is a smart idea, but doesn't work for select.

I think instead we might be able to modify the select query SQL to use AT TIME ZONE, and let the database do the heavy lifting for us. I doubt SQLite supports this, so will need a work around. Will play around with it a bit.

[dantownsend]

Using AT TIME ZONE works OK, but the main problem is it's returned as a naive timestamp. Maybe that's OK ... but my feeling is it's better to somehow add the timezone information back.

Edit: I managed to work around this. Now I have select queries working, but not objects 😆 - I think it's getting close though.

Edit 2: I have objects queries working now.

[dantownsend]

Besides writing some docs, and a few more tests, I think this is done now:

#978

It just adds a few things to this original PR. You can override the timezone in a select query:

await Concert.select(Concert.starts.at_time_zone('America/New_York'))

[aabmets]

@dantownsend @sinisaos Thank you for taking the time to test and improve this PR.
As I am not intimately familiar with Piccolo internals, some oversights were bound to remain in my original implementation. I'm glad you were able to find solutions to the remaining problems.

I do have a question about the proposed solution to the select queries. If a column is designated as Timestamptz with tz information set with ZoneInfo, that column identifies itself as only existing in that timezone, meaning that whenever a value of that column is being accessed, the user can be confident that the value is in the timezone that they defined the column as. When the user is able to override the timezone information with at_time_zone, that defeats the whole purpose of assigning timezone information about the column at table definition, because it provides a supported workaround how to alter the values stored in the column in a non-congruent way to the column timezone definition. Maybe it should work like this, maybe it's a good idea. I do not know however, that's why I'm asking.

EDIT:
I would propose the solution, that select queries should return the datetime from the database in the timezone that the column was defined to exist in with Timestamptz(tz=ZoneInfo()). If the user wants to convert the datetime into another timezone, they should explicitly perform the conversion using Pythons builtin method of datetime.datetime.astimezone(tz) for timezone conversions. The rationale behind this is that Piccolo maintains datetime congruency across its queries and column definitions and the user has a standardized way of performing timezone conversions.