Skip to content
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

QueryBuilder: add support for datetime.date objects in filters #3796

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Feb 24, 2020

Fixes #3794

Each query is hashed for efficiency reasons, such that repeated queries
can be taken from a cache. This requires the builder instance and all
its attributes to be hashable. The filters can reference datetime.date
objects to express date and time conditions, but these were not
supported by the hashing implementation, unlike datetime.datetime
objects. Here we update the aiida.common.hashing.make_hash single
dispatch to also support datetime.date objects.

@sphuber sphuber requested a review from greschd February 24, 2020 11:02
@sphuber
Copy link
Contributor Author

sphuber commented Feb 24, 2020

Not a hundred percent sure about the hash implementation for datetime.date. It should not matter for how it is used for the QueryBuilder but if it is used for other purposes then it may be a problem if implemented incorrectly.


# The method to get the timestamp from the date object depends on whether it is in UTC or a local timezeon
# See for an extended discussion https://stackoverflow.com/a/8778548
if getattr(settings, 'USE_TZ', False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if TZ handling makes sense here. Without it, we could just use _single_digest('date', val.isoformat().encode('utf-8')).

In addition to the QueryBuilder, _make_hash is also used in the caching implementation. Since the datetime.date itself is a naive, timezone-unaware datastructure, I think the hashing should do the same. Both should be more or less trouble-free in most cases. But, if I were to import a Node containing a datetime.date somewhere (if that's even possible?) into a system using a different timezone, caching for that node would be broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the datetime.date itself is a naive, timezone-unaware datastructure, I think the hashing should do the same. Both should be more or less trouble-free in most cases.

Fair enough

But, if I were to import a Node containing a datetime.date somewhere (if that's even possible?) into a system using a different timezone, caching for that node would be broken.

When talking about attributes as to what a Node can contain, then it has to be serializable according to aiida.orm.utils.node.clean_value and neither datetime.datetime nor datetime.date objects are supported. The former used to be, but we removed that. Users are now required to serialize these themselves into a serializable type, such as a timestamp. The deserialization also has to be done manually by the caller. So I guess it is true, even for the caching mechanism this shouldn't matter because it should never get a datetime.date object but some string or float timestamp.

@giovannipizzi do you agree that date objects we can always treat as timezone naive and just take the isoformat string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in person with @giovannipizzi and he agrees that timezone for date object is meaningless and so just the ISO formatted string is fine

Each query is hashed for efficiency reasons, such that repeated queries
can be taken from a cache. This requires the builder instance and all
its attributes to be hashable. The filters can reference `datetime.date`
objects to express date and time conditions, but these were not
supported by the hashing implementation, unlike `datetime.datetime`
objects. Here we update the `aiida.common.hashing.make_hash` single
dispatch to also support `datetime.date` objects. Note that, unlike for
`datetime.datetime` objects, timezone does not have to be considered as
dates are timezone unaware data structures.
@sphuber sphuber force-pushed the fix_3794_querybuilder_date_support branch from 4109a51 to 3020441 Compare February 24, 2020 13:14
@sphuber sphuber requested a review from greschd February 24, 2020 13:15
@sphuber sphuber merged commit 061f6ae into aiidateam:develop Feb 24, 2020
@sphuber sphuber deleted the fix_3794_querybuilder_date_support branch February 24, 2020 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QueryBuilder does not support datetime.date types
2 participants