feature property types ✨ #16
Conversation
- implemented datetime property type
|
@vaind would be nice if you could take a look |
| PropertyType.string: flatbuffers.number_types.UOffsetTFlags, | ||
| # PropertyType.date: flatbuffers.number_types.Int64Flags, | ||
| PropertyType.date: flatbuffers.number_types.Int64Flags, | ||
| PropertyType.dateNano: flatbuffers.number_types.Float64Flags, |
There was a problem hiding this comment.
Date nano also use 64 bit integers; there should not be any floats involved?
There was a problem hiding this comment.
Sorry thought for the extra resolution of nano seconds this would be necessary
There was a problem hiding this comment.
Internally (e.g. FlatBuffers wise) it's ints and the resolution is "perfect" for a limited time (a couple of 200+ years left iirc) - I do not know about Python though and how to treat nano precision there...
There was a problem hiding this comment.
Hmm I would see it as a little hack.
When I have a timestamp in nanosecond resolution e.g. 1667401463110012130 I'd have to divide it by 1.000.000.000 before I can use it in .fromtimestamp(). But the division is not different to using a float value in the first place. It might be even better to use floats to preserve precision cause:
>>> 1667401463110012130 / 1000000000
1667401463.110012
results in a precision loss. What do you think @greenrobot ?
There was a problem hiding this comment.
The flatbuffers representation should not store floating point for date nano to match the other platforms. (binary compatibility, also because of sync)
| val = prop._py_type(table.Bytes[start:start+size]) | ||
| elif prop._ob_type == OBXPropertyType_Date or prop._ob_type == OBXPropertyType_DateNano: | ||
| table_val = table.Get(prop._fb_type, o + table.Pos) | ||
| val = datetime.fromtimestamp(table_val) if table_val != 0 else 0 # default timestamp |
There was a problem hiding this comment.
I think you would need to differentiate Date vs. DateNano here? E.g. a factor of 1000000?
There was a problem hiding this comment.
If floats are used this happens automatically
objectbox/model/entity.py
Outdated
| val = getattr(object, prop._name) | ||
| if val == prop: | ||
| return prop._py_type() # default (empty) value for the given type | ||
| if isinstance(val, datetime): # handle datetimes first |
There was a problem hiding this comment.
I'm not very familiar with specifics here; maybe we can avoid isinstance here and go via property type? I am assuming that isinstance takes more CPU cycles as it is common for reflective calls for the languages I know, but I am not familiar with Python internals.
There was a problem hiding this comment.
changed to use hasttr; property type is ambiguous unfortunately if val is the property itself
| # TODO property type DATE | ||
| date_created = Property(int, id=3, uid=1003) | ||
| date_finished = Property(int, id=4, uid=1004) | ||
| date_created = Property(datetime, type=PropertyType.dateNano, id=3, uid=1003) |
There was a problem hiding this comment.
Cool, one TODO less! 👍
Btw, using it in the example is great, and a test would be even better (optionally only if you want...)
example/__main__.py
Outdated
| def now_ms() -> int: | ||
| seconds: float = datetime.datetime.utcnow().timestamp() | ||
| return round(seconds * 1000) | ||
| return datetime.utcnow() |
There was a problem hiding this comment.
This is much more readable. Does that also have similar (millisecond) precision?
Btw, The docs say that datetime.now(timezone.utc) is preferred for some reason, but I did not dig deeper.
There was a problem hiding this comment.
Docs say this because of:
Because naive datetime objects are treated by many datetime methods as local times, it is preferred to use aware datetimes to represent times in UTC. As such, the recommended way to create an object representing the current time in UTC is by calling datetime.now(timezone.utc).
Gonna change it 👍
|
@greenrobot any news on this? :) |
|
Hey @liquidiert, we have just released Date types: https://github.com/objectbox/objectbox-python/releases/tag/v0.5.0. Thanks for your contribution, it was helpful and is highly appreciated! Unfortunately, we weren't able to include your commits in the release, as significant changes were needed to allow using both |
3 participants
references #5