Skip to content

A bug in conversion of milliseconds #215

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

Merged
merged 2 commits into from
Aug 21, 2015
Merged

Conversation

or-else
Copy link
Contributor

@or-else or-else commented Aug 20, 2015

Rethink stores time as a floating point number with precision to milliseconds. Go's native time has precision to nanoseconds. The gorethink's conversion of time from float64 milliseconds to time.Time creates artifacts in microseconds and nanoseconds. With the current code every time.Time fetched from RethinkDB must be rounded to milliseconds if consistency of stored vs retrieved value is important. It makes sense to do correct rounding in the driver. The fix ensures that the time is consistent.

Rethink stores time as a floating point number with precision to milliseconds. Go's native time has precision to nanoseconds. The gorethink's conversion of time from float64 milliseconds to time.Time creates artifacts in microseconds and nanoseconds. With the current code every time.Time fetched from RethinkDB it must be rounded to milliseconds if consistency of stored vs retrieved value is important. It makes sense to do correct rounding in the driver. The fix ensures that the time is consistent.
@or-else
Copy link
Contributor Author

or-else commented Aug 20, 2015

The failing test makes no sense to me. It asserts that
[1986-11-03 12:30:15.67900002] is equal to [1986-11-03 12:30:15.679] which is only possible if the fact of storing [1986-11-03 12:30:15.679] in the database converts it to [1986-11-03 12:30:15.67900002]
It asserts incorrect behavior.

@dancannon
Copy link
Collaborator

The failing test is due to the hacky way the test was originally implemented https://github.com/dancannon/gorethink/blob/master/query_time_test.go#L26. With your fix this is no longer needed so the test could probably be changed to:

c.Assert(response.Equal(time.Date(1986, 11, 3, 12, 30, 15, 679*1000*1000, time.UTC)), test.Equals, true)

Would it be possible for you to include that in your PR?

Fixing failing test
@or-else
Copy link
Contributor Author

or-else commented Aug 20, 2015

I made a change to the call to Time to assert that precision beyond milliseconds is correctly stripped.

@ianberinger
Copy link

I stumbled over this too. For what it's worth, I rounded the time value with time.Round(time.Millisecond) in my code. Your changes look good though.

@dancannon
Copy link
Collaborator

time.Round seems like a simpler solution (I didn't realise that function existed haha).

What do you think @or-else?

@or-else
Copy link
Contributor Author

or-else commented Aug 20, 2015

Actually that was my first attempt:
t := time.Unix(int64(sec), int64(ms*1000*1000*1000)).Round(time.Millisecond)
but then it seemed suboptimal - first construct incorrect value, then fix it.

@dancannon
Copy link
Collaborator

Ok makes sense, one last question (and this is just me being slow) why are you adding 0.5 before rounding?

@or-else
Copy link
Contributor Author

or-else commented Aug 20, 2015

It ensures correct rounding, math.Floor rounds down. See here:
https://play.golang.org/p/AijkNZjEzY

@or-else
Copy link
Contributor Author

or-else commented Aug 20, 2015

Also see discussion here: golang/go#4594

dancannon added a commit that referenced this pull request Aug 21, 2015
A bug in conversion of milliseconds
@dancannon dancannon merged commit 08eafb4 into rethinkdb:master Aug 21, 2015
@dancannon
Copy link
Collaborator

Merged, thanks again for both the PR and for dealing with all my questions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants