-
Notifications
You must be signed in to change notification settings - Fork 18
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
feature property types ✨ #16
Conversation
- implemented datetime property type
@vaind would be nice if you could take a look |
@@ -44,7 +46,8 @@ class PropertyType(IntEnum): | |||
PropertyType.float: flatbuffers.number_types.Float32Flags, | |||
PropertyType.double: flatbuffers.number_types.Float64Flags, | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Date nano also use 64 bit integers; there should not be any floats involved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry thought for the extra resolution of nano seconds this would be necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flatbuffers representation should not store floating point for date nano to match the other platforms. (binary compatibility, also because of sync)
@@ -143,6 +146,9 @@ def unmarshal(self, data: bytes): | |||
|
|||
# slice the vector as a requested type | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you would need to differentiate Date vs. DateNano here? E.g. a factor of 1000000?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If floats are used this happens automatically
objectbox/model/entity.py
Outdated
@@ -78,8 +79,10 @@ def fill_properties(self): | |||
def get_value(self, object, prop: Property): | |||
# in case value is not overwritten on the object, it's the Property object itself (= as defined in the Class) | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
from example.model import * | ||
|
||
|
||
# objectbox expects date timestamp in milliseconds since UNIX epoch | ||
def now_ms() -> int: | ||
seconds: float = datetime.datetime.utcnow().timestamp() | ||
return round(seconds * 1000) | ||
return datetime.utcnow() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
references #5