redis-om-python icon indicating copy to clipboard operation
redis-om-python copied to clipboard

Support time points as numeric indexes

Open moznuy opened this issue 3 years ago • 2 comments

This is a rough draft

What's is implemented:

  • datetime.datetime works as numeric index.
  • datetime.date works as numeric index.
  • datetime.time works as numeric index. ~~(very sketchy, see below)~~
  • Add test for all above for:
    • Saving to Redis, checking if is valid.
    • Finding by less operator(any comparison should work) in sorted fashion.
    • Saving no-UTC and checking that restored version(UTC) is equal to pre-saving no-UTC value.

What can be added / improved:

  • ~~datetime.time custom parsing implementation~~
  • Our type for datetime.time because user will have to write parsing(validation) themselves otherwise(see post_model_time in tests).
  • Support for datetime.timedelta (I skipped it for now).
  • Support for Model.find(Model.field < "Thu Sep 25 10:36:28 2003") or ISO format, or etc...
  • Documentation.
  • More tests.
  • MORE TESTS(Seriously, I don't think I understand time zones 100%).

Questions:

  • How to store timezone information as numeric field? Right now I just convert everything to UTC before saving. (This needs to be checked)
  • What to do if user did not provide timezone?
  • Do we want to support Model.find(Model.field < "Thu Sep 25 10:36:28 2003") or "2003-09-25T10:49:41.5-03:00" or any other string in expressions. (Because pydantic supports only ISO format(I think)). We could use dateutil. Or make it optional dependency.
  • Precision. To what precision should we save time-points. I implemented milliseconds because:
    • It should be enough
    • pydantic parsing support parsing datetime from integers ms out of the box.
    • Later I checked redis-om-node, and there they support precision to seconds(I'm not sure): https://github.com/redis/redis-om-node#searching-on-dates
  • Cross type expression(Model.datetime < date/time). Should we support this? Or catch this and raise exception?
  • Will Redis at some point add "Timepoint" index field type(like NUMERIC)? (with timezone support). Because that would simplify things.
  • I probably forgot thing or 10, will add later

Problems:

  • datetime.time: pydantic support parsing only as:
    time, existing time object
    str, following formats work:
       HH:MM[:SS[.ffffff]][Z or [±]HH[:]MM]]]
    
    so no int parsing ~~, very custom logic(probably wrong)~~

Sketchy things:

  • ~~datetime.time custom parsing implementation~~
  • I added this to resolve_value. Maybe there is better approach.
      elif field_type is RediSearchFieldTypes.NUMERIC:
    +     value = jsonable_encoder(value)
    
    https://github.com/redis/redis-om-python/pull/240/files#diff-ed42e1a9e452289cee8007ec1dc71beb5e883a54f7f58db63320901b9e4baaf3R546

Fixes #199

moznuy avatar May 07 '22 09:05 moznuy

Codecov Report

Merging #240 (97c617c) into main (c2339b3) will increase coverage by 0.36%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #240      +/-   ##
==========================================
+ Coverage   77.25%   77.61%   +0.36%     
==========================================
  Files          12       12              
  Lines        1165     1184      +19     
==========================================
+ Hits          900      919      +19     
  Misses        265      265              
Flag Coverage Δ
unit 77.61% <100.00%> (+0.36%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aredis_om/model/encoders.py 59.77% <100.00%> (+8.38%) :arrow_up:
aredis_om/model/model.py 86.33% <100.00%> (+0.06%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c2339b3...97c617c. Read the comment docs.

codecov-commenter avatar May 07 '22 09:05 codecov-commenter

Any input will be appreciated

moznuy avatar May 12 '22 19:05 moznuy