-
Notifications
You must be signed in to change notification settings - Fork 95
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
Conversation
...ordering with isort
...s, migration tests are not fixed
Codecov ReportAttention: Patch coverage is
❗ 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. 🚀 New features to boost your workflow:
|
Timestamptz autoconversion (追記ここまで)
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.
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 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.
@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'})
@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
objectsqueries. 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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 oftzto be consistent with theat_time_zonemethod name, and to make it clearer what it does.I was thinking of adding a method to the
objectsquery 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
objectsqueries. 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.
Uh oh!
There was an error while loading. Please reload this page.
Based off #959
Created this branch for testing. Experimenting with the
AT TIME ZONEclause.Remaining tasks:
selectandobjectsqueries workingat_time_zonemethodselectqueriesat_time_zonemethodObjects._get_select_querya bitat_time_zonemethod toobjects