Feature/datetime-metadata-filters #486
Conversation
sdafni
force-pushed
the
feature/datetime-metadata-advanced
branch
from
93d98bc
to
4915bd3
Compare
There was a problem hiding this comment.
Additional comments:
- Did you check saving/restoring a dataset?
- Pls add a couple tests for the new operators to https://github.com/DagsHub/client/blob/master/tests/data_engine/test_querying.py
- Probably need a brainstorm with Guy w.r.t. to what he wants to do with the timezones in the querying, because this all seems very unwieldy right now for me
| def _get_local_timezone(): | ||
| now_utc = datetime.datetime.utcnow() | ||
| local_timezone = tz.tzlocal() | ||
| return now_utc.astimezone(local_timezone).strftime('%z')[:-2] + ':' + now_utc.astimezone( | ||
| local_timezone).strftime('%z')[-2:] |
There was a problem hiding this comment.
https://stackoverflow.com/a/61049837
shouldn't be using utcnow(), because it returns tz-unaware datetimes.
Might also break on windows, where the clock is set to localtime and not UTC.
Can you explain a bit more what this function does in general, because I'm not sure I understand it right now and it feels like there will be some tz errors inevitably
There was a problem hiding this comment.
return a timezone offset in the form of "+03:00" or "-03:00"
dagshub/data_engine/model/query.py
Outdated
| "filter": { | ||
| "key": key, | ||
| "value": value if query_op is FieldFilterOperand.TIMEOFDAY else 0, | ||
| "valueRange": 0 if query_op is FieldFilterOperand.TIMEOFDAY else value, |
There was a problem hiding this comment.
Is there a point to doing the valueRange thing if it can be instead represented as multiple ORs?
I don't know how the frontend would go about handling all of this though, but adding a new field in the filter just for this seems strange to me
There was a problem hiding this comment.
we could, but since in the UI there is an option to mark multiple days, its more convenient to have a single filter built at once
sdafni
force-pushed
the
feature/datetime-metadata-advanced
branch
from
f6aa7b1
to
21c4091
Compare
|
|
||
| """ | ||
| self._test_not_comparing_other_ds(item) | ||
| self._query.timezone = _get_local_timezone() |
There was a problem hiding this comment.
if not self._query.timezone:
self._query.timezone = _get_local_timezone()
There was a problem hiding this comment.
Oops 🙃
Make a PR to fix it I guess 🤷
backend: https://github.com/DagsHub/gogs/pull/1826