Encode millisecond precision #1024
Conversation
This would fix the broken test, but IMO it's wrong.
Now we need to assert millisecond precision has only 3 sub-sec digits. I dropped one of the timestamps for brevity.
native/explorer/src/datatypes.rs
Outdated
| // fn microsecond_tuple(&self) -> (u32, u32) { | ||
| // let us = self.subsec_micros_limited(); | ||
| // if us == 0 { | ||
| // (0, 0) | ||
| // } else { | ||
| // (self.subsec_micros_limited(), 6) | ||
| // } | ||
| // } |
There was a problem hiding this comment.
There's currently a test that fails:
iex> s = Explorer.Series.from_list([~T[01:55:00], ~T[15:35:00], ~T[23:00:00]])
iex> Explorer.Series.quantile(s, 0.5)
~T[15:35:00]with:
code: Explorer.Series.quantile(s, 0.5) === ~T[15:35:00]
left: ~T[15:35:00.000000]
right: ~T[15:35:00]
The commented code would fix this test. However, I think we should change the test instead.
The Rust-side can't tell the difference between a chrono::NaiveTime that was originally encoded from an Elixir %Time{} with :second-precision and one with higher precision that happens to have 0 microseconds. I think the best we can do is return what we know for sure.
Thoughts?
There was a problem hiding this comment.
We can change the test. All of the times given in the test have precision of 0, so ideally we would return precision 0, but if we have to give it higher precision, any of the precisions are fine.
There was a problem hiding this comment.
ideally we would return precision 0
Right. Unfortunately there's no way to annotate this now because of the lack of :second precision in Polars: #885. Also we only support :time and not {:time, :millisecond | :microsecond | :nanosecond} (though we could if we wanted).
Follow-up to: #1020.
Before we were always encoding microseconds like:
microsecond: {us, 6}This PR changes that for when we have millisecond precision to:
microsecond: {us / 1_000 * 1_000, 3}It's still 6 digits, but rounded down to the nearest 1,000.