Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Timestamptz autoconversion #978

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

Open
dantownsend wants to merge 34 commits into master
base: master
Choose a base branch
Loading
from timezonetz-autoconversion
Open

Conversation

@dantownsend
Copy link
Member

@dantownsend dantownsend commented Apr 5, 2024
edited
Loading

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

codecov-commenter commented Apr 5, 2024
edited
Loading

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

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

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

Files with missing lines 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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dantownsend dantownsend changed the title (削除) Timezonetz autoconversion (削除ここまで) (追記) Timestamptz autoconversion (追記ここまで) Apr 5, 2024
@dantownsend dantownsend changed the title (削除) Timestamptz autoconversion (削除ここまで) (追記) Timestamptz autoconversion (追記ここまで) Apr 5, 2024
Copy link
Member Author

dantownsend commented Apr 5, 2024
edited
Loading

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.

Copy link
Member Author

dantownsend commented Apr 5, 2024
edited
Loading

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
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 and jrycw reacted with thumbs up emoji

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'})

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.

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
edited
Loading

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

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.

pavdwest reacted with thumbs up emoji
Copy link
Member

@sinisaos sinisaos Apr 7, 2024

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
edited
Loading

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.

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.

sinisaos reacted with thumbs up emoji

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

Reviewers

@sinisaos sinisaos sinisaos left review comments

+1 more reviewer

@jrycw jrycw jrycw left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

Status: In progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /