Fix hour transform #1146
Fix hour transform #1146
Conversation
Fokko
force-pushed
the
fd-fix-hour-truncation
branch
from
e01d6e1 to
c3b3ba9
Compare
Seems like we're rounding towards zero by default by Rust. While we want to `floor` the value.
Fokko
force-pushed
the
fd-fix-hour-truncation
branch
from
c3b3ba9 to
f606fd4
Compare
Fokko
force-pushed
the
fd-fix-hour-truncation
branch
from
c978057 to
8d6e797
Compare
Fokko
force-pushed
the
fd-fix-hour-truncation
branch
from
4616f1c to
51e5407
Compare
There was a problem hiding this comment.
Thanks @Fokko for this fix! Rust std has provided functionality for us so that we don't need to do the hack by ourself.
| @@ -337,12 +337,12 @@ pub struct Hour; | |||
| impl Hour { | |||
| #[inline] | |||
| fn hour_timestamp_micro(v: i64) -> i32 { | |||
| (v / MICROSECONDS_PER_HOUR) as i32 | |||
| (v as f64 / MICROSECONDS_PER_HOUR).floor() as i32 | |||
There was a problem hiding this comment.
| (v as f64 / MICROSECONDS_PER_HOUR).floor() as i32 | |
| (v.div_euclid(MICROSECONDS_PER_HOUR)) as i32 |
There is a div_euclid which could help use to do this. The div_floor method seems better, but it's unstable. Give MICROSECONDS_PER_HOUR is always positive, div_euclid is good enough here.
There was a problem hiding this comment.
Love it, thanks for the suggestion! 🙌
There was a problem hiding this comment.
Out of curiosity, what's the risk of using the unstable API? We have limited test cases in place to verify the behavior. Shouldn't you use unstable API's at all?
There was a problem hiding this comment.
Using unstable api means we need to use unstable rust sdk, what's worse it requires all of our downstream users to use unstable rust sdk, which is anti pattern for a rust library.
Unstable rust sdk contains experimental apis, but there is no guarantee that those apis will be still there when a stable version is released.
See doc about rust channels
| test_timestamp_and_tz_transform("1970-01-01T22:01:01.000000", &hour, Datum::int(22)); | ||
| test_timestamp_and_tz_transform("1969-12-31T23:00:00.000000", &hour, Datum::int(-1)); | ||
| test_timestamp_and_tz_transform("1969-12-31T22:01:01.000000", &hour, Datum::int(-2)); | ||
| test_timestamp_and_tz_transform("0022-05-01T22:01:01.000000", &hour, Datum::int(-17072906)); |
There was a problem hiding this comment.
-17_072_905.98 but rounded up 👍
Which issue does this PR close?
Seems like we're rounding towards zero by default by Rust. While we want to
floorthe value.Checked against the reference implementation:
What changes are included in this PR?
Are these changes tested?