piccolo icon indicating copy to clipboard operation
piccolo copied to clipboard

`Timestamptz` autoconversion

Open dantownsend opened this issue 1 year ago • 7 comments

Based off https://github.com/piccolo-orm/piccolo/pull/959

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

Remaining tasks:

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

dantownsend avatar Apr 05 '24 12:04 dantownsend

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 :warning:
piccolo/query/methods/objects.py 96.15% 1 Missing :warning:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Apr 05 '24 13:04 codecov-commenter

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 avatar Apr 05 '24 17:04 dantownsend

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 avatar Apr 05 '24 18:04 dantownsend

@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 avatar Apr 06 '24 12:04 sinisaos

@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 avatar Apr 06 '24 21:04 dantownsend

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

sinisaos avatar Apr 07 '24 05:04 sinisaos

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

jrycw avatar Apr 07 '24 08:04 jrycw