TINKERPOP-2264 Fixed g:Date serialization for python.#1165
Conversation
Got timezone of the local system out of the derser mix and standardized around what's always been expected in Java's Date.getTime().
| -1, -1, -1)) + obj.microsecond / 1e6 | ||
| # Hack for legacy Python - timestamp() in Python 3.3 | ||
| pts = (time.mktime(obj.timetuple()) + obj.microsecond / 1e6) - \ | ||
| (time.mktime(cls.epoch.timetuple())) |
There was a problem hiding this comment.
If I'm understanding this right, it's taking the epoch offset and removing it from our current time.
So for example, obj.timetuple() will return current UTC-5 for me, and cls.epoch.timetuple() should return epoch-5, so we if we subtract them, we get the UTC time as unix time.
But according to the end of https://stackoverflow.com/a/5499906/37205, the epoch offset can be different for DST timezones. I don't really understand how that can be, but when I'm not in daylight time, I'm UTC-6. I think this answer is saying that if my timezone was affected, it would return epoch as epoch-6 when current time would be UTC-5.
| # Java timestamp expects milliseconds. | ||
| if six.PY3: | ||
| pts = obj.timestamp() | ||
| pts = (obj - cls.epoch) / datetime.timedelta(seconds=1) |
There was a problem hiding this comment.
To me this is a bit of a confusing way to get to a timestamp. I might suggest having a look at datetime handling in the Cassandra driver: https://github.com/datastax/python-driver/blob/master/cassandra/cqltypes.py#L577-L579
This technique has a few advantages:
- No special cases per python runtime
- Handles datetime inputs with timezones attached, normalizing back to UTC
- More readable (imo)
| -1, -1, -1)) + obj.microsecond / 1e6 | ||
| # Hack for legacy Python - timestamp() in Python 3.3 | ||
| pts = (time.mktime(obj.timetuple()) + obj.microsecond / 1e6) - \ | ||
| (time.mktime(cls.epoch.timetuple())) |
There was a problem hiding this comment.
I know this was not introduced in this change, but a "hack" should not be needed if an alternative conversion technique is used (see above).
| # Java timestamp expects milliseconds. | ||
| if six.PY3: | ||
| pts = obj.timestamp() | ||
| pts = (obj - cls.epoch) / datetime.timedelta(seconds=1) |
There was a problem hiding this comment.
Same comments as V2d0 file.
This approach shouldn't hose up with daylight savings time.
spmallette
force-pushed
the
TINKERPOP-2264
branch
from
b1be2a4 to
a015b2f
Compare
|
thanks @aholmberg and @TheRealFalcon - i got rid of hacks (and the "old" way in general) in exchange for |
aholmberg
left a comment
aholmberg
left a comment
There was a problem hiding this comment.
Agree that you only needed the datetime leg of that code.
|
VOTE +1 |
|
Not an expert in Python, but the code appears to be good. VOTE +1 |
5 participants
https://issues.apache.org/jira/browse/TINKERPOP-2264
Got timezone of the local system out of the derser mix and standardized around what's always been expected in Java's
Date.getTime().Builds with
mvn clean install :pl gremlin-pythonVOTE +1