`Timestamptz` autoconversion #978

dantownsend · 2024-04-05T12:58:26Z

Based off #959

Created this branch for testing. Experimenting with the AT TIME ZONE clause.

Remaining tasks:

Get select and objects queries working
Get SQLite working
Fix existing tests
Add docs for at_time_zone method
Add tests for select queries
Add tests for the at_time_zone method
Optimise Objects._get_select_query a bit
Add an at_time_zone method to objects

…om_datetime

…ordering with isort

…s, migration tests are not fixed

codecov-commenter · 2024-04-05T13:37:39Z

Codecov Report

Attention: Patch coverage is 94.89796% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 92.77%. Comparing base (363d683) to head (2c75e70).
Report is 1 commits behind head on master.

Files	Patch %	Lines
piccolo/columns/column_types.py	81.81%	4 Missing ⚠️
piccolo/query/methods/objects.py	96.15%	1 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #978      +/-   ##
==========================================
- Coverage   92.78%   92.77%   -0.02%     
==========================================
  Files         108      109       +1     
  Lines        8182     8216      +34     
==========================================
+ Hits         7592     7622      +30     
- Misses        590      594       +4

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

dantownsend · 2024-04-05T17:29:57Z

I think this is almost done now. select queries work fine. But surprisingly object queries override response_handler from select, so aren't currently working. It's just a matter of shuffling some logic around.

Update: objects queries now work too.

dantownsend · 2024-04-05T18:22:51Z

Now we need to work out what to do with SQLite - I might just raise a ValueError for now.

Update: I was able to get SQLite working.

sinisaos · 2024-04-06T12:13:50Z

@dantownsend I tried your solution and everything worked great. Here is a Piccolo shell results

# if we declare a time zone in the table definition
# class TallinnConcerts(Table):
#   event_start = Timestamptz(at_time_zone=ZoneInfo("Europe/Tallinn"))
In [1]: await TallinnConcerts.select()
Out[1]: 
[{'id': 1,
  'event_start': datetime.datetime(2024, 4, 5, 10, 44, 46, tzinfo=zoneinfo.ZoneInfo(key='Europe/Tallinn'))},
 {'id': 2,
  'event_start': datetime.datetime(2024, 4, 5, 10, 44, 46, tzinfo=zoneinfo.ZoneInfo(key='Europe/Tallinn'))}]

In [2]: [i.to_dict() for i in await TallinnConcerts.objects()]
Out[2]: 
[{'id': 1,
  'event_start': datetime.datetime(2024, 4, 5, 10, 44, 46, tzinfo=zoneinfo.ZoneInfo(key='Europe/Tallinn'))},
 {'id': 2,
  'event_start': datetime.datetime(2024, 4, 5, 10, 44, 46, tzinfo=zoneinfo.ZoneInfo(key='Europe/Tallinn'))}]

# if we do not declare a timezone in the table definition, we can define `at_time_zone` in the  select query
# class TallinnConcerts(Table):
#   event_start = Timestamptz()
In [3]: await TallinnConcerts.select(TallinnConcerts.event_start.at_time_zone("Europe/Tallinn"))
Out[3]: 
[{'event_start': datetime.datetime(2024, 4, 6, 16, 37, 33, tzinfo=zoneinfo.ZoneInfo(key='Europe/Tallinn'))},
 {'event_start': datetime.datetime(2024, 4, 6, 16, 37, 33, tzinfo=zoneinfo.ZoneInfo(key='Europe/Tallinn'))}]

Piccolo Admin also work.

dantownsend · 2024-04-06T21:26:25Z

@sinisaos Thanks a lot for testing it.

What do you think about the design of it?

I changed it to be:

Timestamptz(at_time_zone=ZoneInfo("America/New_York"))

So the argument is at_time_zone, instead of tz to be consistent with the at_time_zone method name, and to make it clearer what it does.

I was thinking of adding a method to the objects query too. For select we can do this:

await Concert.select(Concert.starts_at.at_time_zone('America/Los_Angeles'))

But there's currently no way of overriding it for objects queries. Maybe something like:

await Concert.objects().at_time_zone({Concert.starts_at: 'America/Los_Angeles'})

sinisaos · 2024-04-07T05:11:09Z

@dantownsend Everything looks and works great to me, as I wrote in the previous comment.

But there's currently no way of overriding it for objects queries. Maybe something like:
await Concert.objects().at_time_zone({Concert.starts_at: 'America/Los_Angeles'})

It would be great to have that objects method to manipulate the results.

jrycw · 2024-04-07T08:09:10Z

piccolo/columns/column_types.py

    ) -> None:
        self._validate_default(
            default, TimestamptzArg.__args__  # type: ignore
        )

        if isinstance(default, datetime):
-            default = TimestamptzCustom.from_datetime(default)
+            default = TimestamptzCustom.from_datetime(default, at_time_zone)

        if default == datetime.now:


I'm a bit confused about this if statement. It seems that datetime.now is a callable, which doesn't seem to align with the TimestamptzArg from the signature. By the way, it seems that the test doesn't cover this scenario involving a callable.

Oops, it should be the line if default == datetime.now:.

jrycw · 2024-04-07T08:14:30Z

requirements/requirements.txt

@@ -5,3 +5,5 @@ targ>=0.3.7
 inflection>=0.5.1
 typing-extensions>=4.3.0
 pydantic[email]==2.*
+tzdata>=2024.1


Based on my experience with zoneinfo on Windows machines, it can exhibit some unexpected behavior even after installing tzdata. If piccolo aims to support cross-platform conditions, I suggest running continuous integration tests for timestamp-related functionality in the Windows container, at least for a period, once this PR is merged.

@dantownsend @sinisaos This might be a little off-topic, but have you guys heard about uv? I've used it for a couple of personal projects and it looks promising to me.

@jrycw Sorry, I've never used uv. I always use virtualenv and I'm used to it.

jrycw · 2024-04-07T08:21:14Z

piccolo/columns/column_types.py

-        >>> await Concert(
-        ...    starts=datetime.datetime(
-        ...        year=2050, month=1, day=1, tzinfo=datetime.timezone.tz


It seems that tzinfo=datetime.timezone.tz should be replaced with tzinfo=datetime.timezone.utc in the old documentation.

jrycw · 2024-04-07T08:31:46Z

piccolo/columns/column_types.py

+    ) -> str:
+        select_string = self._meta.get_full_name(with_alias=False)
+
+        if self._at_time_zone != ZoneInfo("UTC"):


There might be other instances where timezone comparisons are performed similarly, and I'll use this one as an example. It seems this type of comparison could lead to unexpected outcomes. The following example might illustrate this unpredictability.

>>> import datetime >>> from zoneinfo import ZoneInfo >>> tz1 = datetime.timezone.utc >>> tz2 = ZoneInfo("UTC") >>> d1 = datetime.datetime(year=2024, month=4, day=7, tzinfo=tz1) >>> d2 = datetime.datetime(year=2024, month=4, day=7, tzinfo=tz2) >>> d1 == d2 True >>> str(d1) == str(d2) True >>> d1.tzinfo, d2.tzinfo (datetime.timezone.utc, zoneinfo.ZoneInfo(key='UTC')) >>> d1.tzinfo == d2.tzinfo False >>> d1.tzinfo is d2.tzinfo False

jrycw · 2024-04-07T08:35:44Z

piccolo/columns/column_types.py

+    def at_time_zone(self, time_zone: t.Union[ZoneInfo, str]) -> Timestamptz:
+        """
+        By default, the database returns the value in UTC. This lets us get
+        the value converted to the specified timezone.
+        """
+        time_zone = (
+            ZoneInfo(time_zone) if isinstance(time_zone, str) else time_zone
+        )
+        instance = self.copy()
+        instance._at_time_zone = time_zone
+        return instance


Regarding #959, it appears that we are creating a new object by copying self and then modifying the _at_time_zone attribute of that object to match the time_zone variable, without altering self. Is my understanding correct?

jrycw · 2024-04-07T08:39:46Z

piccolo/columns/column_types.py

+        if self._at_time_zone != ZoneInfo("UTC"):
+            # SQLite doesn't support `AT TIME ZONE`, so we have to do it in
+            # Python instead (see ``Select.response_handler``).
+            if self._meta.engine_type in ("postgres", "cockroach"):


Occurrences of in or == checks like if self._meta.engine_type in ("postgres", "cockroach"): seem to be becoming increasingly common. I would recommend using data structures like Enum to mitigate typos and rely more on auto-completion while coding.

jrycw · 2024-04-07T08:52:14Z

@sinisaos Thanks a lot for testing it.

What do you think about the design of it?

I changed it to be:
Timestamptz(at_time_zone=ZoneInfo("America/New_York"))
So the argument is at_time_zone, instead of tz to be consistent with the at_time_zone method name, and to make it clearer what it does.

I was thinking of adding a method to the objects query too. For select we can do this:
await Concert.select(Concert.starts_at.at_time_zone('America/Los_Angeles'))
But there's currently no way of overriding it for objects queries. Maybe something like:
await Concert.objects().at_time_zone({Concert.starts_at: 'America/Los_Angeles'})

I quite like this polars-like select expression (or should I say polars is piccolo-like 😊?), which feels natural to use. Additionally, I agree with @sinisaos that the syntax of the objects method looks good to me.

aabmets and others added 23 commits March 18, 2024 01:57

Added column tz autoconversion to Table __init__ method

8cfa0c7

Added tz ability to Timestamptz

95d8e4d

Added tz ability to TimestamptzNow

7dd76d8

Added timezone awareness to TimestamptzOffset and TimestamptzCustom

06e0625

Added zoneinfo fallback dependency for missing zone data

c589d44

Added backports.zoneinfo

fc708fc

Un-privatized tz attribute from default timestamptz classes

705629a

Added python version constraint to backports.zoneinfo

a25e814

Merge branch 'piccolo-orm:master' into timestamptz-autoconversion

86004b0

Updated Timestamptz docstring and fixed a bug in TimestamptzCustom.fr…

6611a97

…om_datetime

Fixed bug in TimestamptzCustom.from_datetime

aa9f077

Fixed linter and test issues (hopefully)

6fcb93f

Added backport.zoneinfo import with try-except clause, fixed imports …

59a531e

…ordering with isort

Fixed linting errors across codebase and fixed timestamptz test error…

7a3c584

…s, migration tests are not fixed

Fixed zoneinfo module import for autogenerated migrations files

41a6d0c

I swear to god if this commit doesn't fix the linting and test errors

ad0d1d1

Added more ZoneInfo import ignore rules for MyPy

c816d8e

Added pragma no cover to ZoneInfo import except clauses

cbf6d41

Removed ZoneInfo import from table.py

db53939

Merge branch 'master' into timezonetz-autoconversion

de33a9b

move tz after default for better backwards compatibility

36b0da6

drop tz_type - I don't think we need it for now

9fda29d

add missing continues

3733218

dantownsend added 6 commits April 5, 2024 15:14

add at_timezone clause

7c6ce55

remove unused imports

755b059

check != ZoneInfo("UTC")

026b182

centralised imports for ZoneInfo

e9b02be

added method for getting column alias

4327178

make sure all Timestamptz values have a tz set

e9fae63

fix typos

4416adc

dantownsend changed the title ~~Timezonetz autoconversion~~ Timestamptz autoconversion Apr 5, 2024

dantownsend changed the title ~~Timestamptz autoconversion~~ Timestamptz autoconversion Apr 5, 2024

fix tests

0ab2d24

fix sqlite

c64efa9

dantownsend added the enhancement New feature or request label Apr 5, 2024

dantownsend added this to In progress in Enhancements via automation Apr 5, 2024

dantownsend marked this pull request as ready for review April 5, 2024 20:56

dantownsend mentioned this pull request Apr 5, 2024

Datetime Autoconversion for Timestamptz Columns #959

Open

dantownsend added 2 commits April 6, 2024 11:35

refactor objects queries to proxy to select

1d98c36

rename tz to at_time_zone

2c75e70

dantownsend mentioned this pull request Apr 6, 2024

Refactor ModelBuilder and RandomBuilder #971

Open

jrycw reviewed Apr 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Timestamptz` autoconversion #978

`Timestamptz` autoconversion #978

dantownsend commented Apr 5, 2024 •

edited

codecov-commenter commented Apr 5, 2024 •

edited

dantownsend commented Apr 5, 2024 •

edited

dantownsend commented Apr 5, 2024 •

edited

sinisaos commented Apr 6, 2024

dantownsend commented Apr 6, 2024

sinisaos commented Apr 7, 2024

jrycw Apr 7, 2024

jrycw Apr 7, 2024 •

edited

jrycw Apr 7, 2024

jrycw Apr 7, 2024

sinisaos Apr 7, 2024

jrycw Apr 7, 2024

jrycw Apr 7, 2024

jrycw Apr 7, 2024

jrycw Apr 7, 2024 •

edited

jrycw commented Apr 7, 2024

Timestamptz autoconversion #978

Are you sure you want to change the base?

Timestamptz autoconversion #978

Conversation

dantownsend commented Apr 5, 2024 • edited

codecov-commenter commented Apr 5, 2024 • edited

Codecov Report

dantownsend commented Apr 5, 2024 • edited

dantownsend commented Apr 5, 2024 • edited

sinisaos commented Apr 6, 2024

dantownsend commented Apr 6, 2024

sinisaos commented Apr 7, 2024

jrycw Apr 7, 2024

Choose a reason for hiding this comment

jrycw Apr 7, 2024 • edited

Choose a reason for hiding this comment

jrycw Apr 7, 2024

Choose a reason for hiding this comment

jrycw Apr 7, 2024

Choose a reason for hiding this comment

sinisaos Apr 7, 2024

Choose a reason for hiding this comment

jrycw Apr 7, 2024

Choose a reason for hiding this comment

jrycw Apr 7, 2024

Choose a reason for hiding this comment

jrycw Apr 7, 2024

Choose a reason for hiding this comment

jrycw Apr 7, 2024 • edited

Choose a reason for hiding this comment

jrycw commented Apr 7, 2024

`Timestamptz` autoconversion #978

`Timestamptz` autoconversion #978

dantownsend commented Apr 5, 2024 •

edited

codecov-commenter commented Apr 5, 2024 •

edited

dantownsend commented Apr 5, 2024 •

edited

dantownsend commented Apr 5, 2024 •

edited

jrycw Apr 7, 2024 •

edited

jrycw Apr 7, 2024 •

edited