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

Timestamptz autoconversion #978

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open

Conversation

dantownsend
Copy link
Member

@dantownsend dantownsend commented Apr 5, 2024

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

aabmets and others added 23 commits March 18, 2024 01:57
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2024

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 dantownsend changed the title Timezonetz autoconversion Timestamptz autoconversion Apr 5, 2024
@dantownsend dantownsend changed the title Timestamptz autoconversion Timestamptz autoconversion Apr 5, 2024
@dantownsend
Copy link
Member Author

dantownsend commented Apr 5, 2024

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
Copy link
Member Author

dantownsend commented Apr 5, 2024

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.

@dantownsend dantownsend added the enhancement New feature or request label Apr 5, 2024
@dantownsend dantownsend marked this pull request as ready for review April 5, 2024 20:56
@sinisaos
Copy link
Member

sinisaos commented Apr 6, 2024

@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
Copy link
Member Author

@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
Copy link
Member

sinisaos commented Apr 7, 2024

@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.

) -> 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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jrycw jrycw Apr 7, 2024

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines -971 to -973
>>> await Concert(
... starts=datetime.datetime(
... year=2050, month=1, day=1, tzinfo=datetime.timezone.tz
Copy link
Contributor

Choose a reason for hiding this comment

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

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

) -> str:
select_string = self._meta.get_full_name(with_alias=False)

if self._at_time_zone != ZoneInfo("UTC"):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Comment on lines +1022 to +1032
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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"):
Copy link
Contributor

@jrycw jrycw Apr 7, 2024

Choose a reason for hiding this comment

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

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
Copy link
Contributor

jrycw commented Apr 7, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

5 participants