Avoid using floating points during timestamp-datetime conversions by hakanakyurek · Pull Request #591 · msgpack/msgpack-python

hakanakyurek · 2024-04-18T12:49:22Z

Right now, when we convert timestamp to datetime or datetime to timestamp, the library uses floating points to do the conversion. This introduces losses during the conversion as the datetime and timestamp datatypes do not support the same precision(nanoseconds).

For instance if we have a timestamp such as:
2024-04-16 08:43:09.420317+00:00
with the current implementation the nanoseconds in the resulting Timestamp object has 420316934 while it should be 420317000 instead.

or in this timestamp:
2024-04-16 08:43:09.400000+00:00
it has 400000095 nanoseconds instead of 400000000.

ThomasWaldmann

LGTM, just a few nitpicks.

I assume that these tests fail without your changes?

ThomasWaldmann · 2024-04-18T19:54:50Z

msgpack/ext.py

        :rtype: Timestamp
        """
-        return Timestamp.from_unix(dt.timestamp())
+        return Timestamp(seconds=int(dt.timestamp() // 1), nanoseconds=dt.microsecond * 10**3)


That x // 1 feels strange.

Considering you use int() after that anyway, why do you need that?

Also: just use 1000 instead of 10**3?

That x // 1 feels strange.

Considering you use int() after that anyway, why do you need that?

Yes, i overlooked that part completely, we dont need the floor division at all. I'll remove it.

Also: just use 1000 instead of 10**3?

we can of course use 1000 instead, its just that in the code that i checked, i either saw "10**x" or "1ex" and wanted follow that convention.

ThomasWaldmann · 2024-04-18T19:55:08Z

msgpack/ext.py

        utc = datetime.timezone.utc
-        return datetime.datetime.fromtimestamp(0, utc) + datetime.timedelta(seconds=self.to_unix())
+        return datetime.datetime.fromtimestamp(0, utc) + datetime.timedelta(
+            seconds=self.seconds, microseconds=round(self.nanoseconds / 1e3)


Just use 1000 instead of 1e3?

I prefer // over round.

Consider "12:34:56.78". It is "12:34" or "12:34:56", not "12:35" nor "12:34:57".

Just use 1000 instead of 1e3?

same as the previous comment, I just though "10**x" or "1ex" was the convention that was being used.

I prefer // over round.

Consider "12:34:56.78". It is "12:34" or "12:34:56", not "12:35" nor "12:34:57".

I'm not actually knowledgeable about whether there should be a round or not. The idea I had with the round is when working with a timestamp with nanosecond precision, lets say, "100,000,900" nanoseconds, I thought it should round to "100,001" microseconds, whereas "100,000,100" nanoseconds should be "100,000" microseconds.

With the case "12:34:56.78", I'm not sure whether it should "12:34:56" or "12:34:57", if rounding isn't the norm in these cases, we should definitely remove it and the tests related to it.

The idea I had with the round is when working with a timestamp.

The year of 2024-10-01 is 2025?
The day of 2024-10-01 15:00:00 is 2024-10-02?
The hour of 15:34 is 16?

Despite what you explain it in natural language, "round down" is consistent in programming.

https://github.com/golang/go/blob/a63907808d14679c723e566cb83acc76fc8cafc2/src/time/time.go#L1214-L1244

https://doc.rust-lang.org/src/core/time.rs.html#360

https://doc.rust-lang.org/src/core/time.rs.html#384

https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/time/Instant.html#toEpochMilli()

If this instant has greater than millisecond precision, then the conversion will drop any excess precision information as though the amount in nanoseconds was subject to integer division by one million.

hmm i see, thanks a lot it makes more sense. Then i'll remove the rounding and tests related to it.

2023-12-31 23:59:59.999999500 -- year 2023, date 2023-12-31.

After round half by micros:

2024-01-01 00:00:00.000000 -- year 2024, date 2024-01-01.

This is why most languages uses consistent "round down" for subseconds.
Consistency is important to avoid bugs. Every programmer should love consistency.

yup of course, I didn't look at it like that when I was proposing the change. Again it makes more sense to round down instead as it would be more consistant as you said. Thanks for the explanation!

ThomasWaldmann · 2024-04-18T20:01:19Z

test/test_timestamp.py

+    t2 = Timestamp(1713256989, 420318123)
+    t3 = Timestamp(1713256989, 420318499)
+    t4 = Timestamp(1713256989, 420318501)
+    assert t2.to_datetime() == t3.to_datetime() != t4.to_datetime()


Maybe just using 2 asserts would be simpler.

definitely, we dont necessarily need to compare with t2 now i think of it. But these were to test the rounding specifically, if rounding is wrong way to go, these tests arent necessarily needed imo.

hakanakyurek · 2024-04-19T08:52:30Z

Exactly, the tests i added except line 100 should fail without the changes.

methane · 2024-04-19T12:49:01Z

msgpack/ext.py

        utc = datetime.timezone.utc
-        return datetime.datetime.fromtimestamp(0, utc) + datetime.timedelta(seconds=self.to_unix())
+        return datetime.datetime.fromtimestamp(0, utc) + datetime.timedelta(
+            seconds=self.seconds, microseconds=self.nanoseconds // 1e3


Suggested change

seconds=self.seconds, microseconds=self.nanoseconds // 1e3

seconds=self.seconds, microseconds=self.nanoseconds // 1000

methane · 2024-04-19T12:49:12Z

msgpack/ext.py

        :rtype: Timestamp
        """
-        return Timestamp.from_unix(dt.timestamp())
+        return Timestamp(seconds=int(dt.timestamp()), nanoseconds=dt.microsecond * 10**3)


Suggested change

return Timestamp(seconds=int(dt.timestamp()), nanoseconds=dt.microsecond * 10**3)

return Timestamp(seconds=int(dt.timestamp()), nanoseconds=dt.microsecond * 1000)

methane · 2024-04-19T12:49:33Z

test/test_timestamp.py

+    ts = datetime.datetime(2024, 4, 16, 8, 43, 9, 420317, tzinfo=utc)
+    ts2 = datetime.datetime(2024, 4, 16, 8, 43, 9, 420318, tzinfo=utc)
+
+    assert Timestamp.from_datetime(ts2).nanoseconds - Timestamp.from_datetime(ts).nanoseconds == 1e3


Suggested change

assert Timestamp.from_datetime(ts2).nanoseconds - Timestamp.from_datetime(ts).nanoseconds == 1e3

assert Timestamp.from_datetime(ts2).nanoseconds - Timestamp.from_datetime(ts).nanoseconds == 1000

methane · 2024-04-19T12:49:42Z

test/test_timestamp.py

+    ts3 = datetime.datetime(2024, 4, 16, 8, 43, 9, 4256)
+    ts4 = datetime.datetime(2024, 4, 16, 8, 43, 9, 4257)
+    assert (
+        Timestamp.from_datetime(ts4).nanoseconds - Timestamp.from_datetime(ts3).nanoseconds == 1e3


Suggested change

Timestamp.from_datetime(ts4).nanoseconds - Timestamp.from_datetime(ts3).nanoseconds == 1e3

Timestamp.from_datetime(ts4).nanoseconds - Timestamp.from_datetime(ts3).nanoseconds == 1000

methane · 2024-04-19T22:46:39Z

thanks

avoid using float to convert timestamp from and to datetime

e92f68e

hakanakyurek changed the title ~~Avoid using floaring points during timestamp-datetime conversions~~ Avoid using floating points during timestamp-datetime conversions Apr 18, 2024

ThomasWaldmann reviewed Apr 18, 2024

View reviewed changes

removed floor division in from_datetime

12d9cc0

removed rounding for sub-second precision in to_datetime

64c0e67

methane requested changes Apr 19, 2024

View reviewed changes

1e3/10**3 --> 1000

34ab273

methane merged commit e776722 into msgpack:main Apr 19, 2024

uyha mentioned this pull request Apr 24, 2024

New release #595

Closed

	seconds=self.seconds, microseconds=self.nanoseconds // 1e3
	seconds=self.seconds, microseconds=self.nanoseconds // 1000

	return Timestamp(seconds=int(dt.timestamp()), nanoseconds=dt.microsecond * 10**3)
	return Timestamp(seconds=int(dt.timestamp()), nanoseconds=dt.microsecond * 1000)

	assert Timestamp.from_datetime(ts2).nanoseconds - Timestamp.from_datetime(ts).nanoseconds == 1e3
	assert Timestamp.from_datetime(ts2).nanoseconds - Timestamp.from_datetime(ts).nanoseconds == 1000

	Timestamp.from_datetime(ts4).nanoseconds - Timestamp.from_datetime(ts3).nanoseconds == 1e3
	Timestamp.from_datetime(ts4).nanoseconds - Timestamp.from_datetime(ts3).nanoseconds == 1000

Conversation

hakanakyurek commented Apr 18, 2024

Uh oh!

ThomasWaldmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hakanakyurek commented Apr 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

methane commented Apr 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants