Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add 'millisecond' option to ser_json_timedelta config parameter #1427
feat: add 'millisecond' option to ser_json_timedelta config parameter #1427
Changes from 1 commit
0107915
21be1a2
537a484
f7a4008
f5df4d4
367da21
4af4625
c959789
5d80b34
c38fce0
abe32e6
9241bd7
61971a0
9aea492
6d54bd8
bff8e3b
00e68b7
b4c86de
574a011
a3900f6
fb80b83
d313c90
3e23511
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
There might be a better way to do this which is alluding me, maybe we could do the multiplication in python? 🤷🏻♂️
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.
Seems reasonable enough - multiplication here should be faster.
Can we pull out some of the shared logic into a function (both for Float and for Millisecond) and repeat that across the various branches?
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.
Hm, maybe, a couple of them use logic like:
However then the serializer needs to wrap these in
map_err
calls, so can't be reused there..Maybe a rust wizard could help with some clever refactoring im not seeing 🧙🏻
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.
To do the multiplication in Python would be something like
... this requires creating a Python integer
1000
, which for best performance we might want to consider caching.But there is another inefficiency here (and in the other cases) which is that the call to
try_into_py
creates a Pythontimedelta
object which then gets thrown away immediately to convert to float. It will probably be better in all cases to use.to_duration()
, which will avoid the temporary Python object in the case of aDuration
Rust value.The best option would be to go further and to add a
.total_seconds()
method toEitherTimedelta
which extracts the fractional seconds from whatever state theEitherTimedelta
is currently in (doing the most efficient thing for each case) and then doing the multiplication in Rust.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, yeah neat suggestion on the
EitherTimedelta
I guess we'd want something like this:
And then we have two cases:
Duration
andPyDelta
to deal with. Looks like we have some methods around that would involve getting thepy_timedelta
into aDuration
object, so then its justDuration
we need to deal with. However (maybe im reading the docs wrong!) i can't seem to see a method on there that returns the total_seconds as a float?Probably missed something here!
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.
Something like this? Not 100% sure what to do with the
Raw
case, but doing what we do currently and extracting from the python object by calling "total_seconds" on it feels like the best way in this case.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 suggest looking at the
to_duration
method to see how that gets days / seconds / microseconds out of the Python value, as that should have the most efficient implementation already set up to get the data out (and then you can do a bit of arithmetic). In general calling a Python method will be slow-ish, so there might be a faster step forPyExact
case.For
Raw
, you need to work with thespeedate::Duration
object, can probably get a timestamp value and microseconds and combine those.