Correctly decode negative timestamps with Avro #16609
Conversation
|
I'm force-merging this because CI already passed on 7ce3ba2, and the only thing I've changed since then is comment text. CI now seems broken for unrelated reasons. |
benesch
left a comment
benesch
left a comment
There was a problem hiding this comment.
Oops, forgot to press "submit" on this last night. Should we also upgrade chrono to the latest main, to get the bugfix in chrono itself?
src/avro/src/decode.rs
Outdated
| // I think it's better if we just inherit Chrono's behavior for now here. | ||
| // I opened an issue with them to discuss: | ||
| // https://github.com/chronotope/chrono/issues/903 |
There was a problem hiding this comment.
Seems like it's a bug in Chrono, so maybe we should do the good thing? Maybe we should also get on the latest main for Chrono?
There was a problem hiding this comment.
We are doing the good thing. I copied the new algorithm from the unreleased main of Chrono.
There was a problem hiding this comment.
I don't think we're using the old buggy from_timestamp_millis anywhere important, so we don't really need to upgrdae to the unreleased branch. I will check that, though.
There was a problem hiding this comment.
I think Petros is adding a use in #16622 (review).
There was a problem hiding this comment.
ok, let's look into upgrading Chrono in that case.
4 participants
Our algorithm was just totally wrong for negative numbers; copy the algorithm Chrono uses instead.
Motivation
Reported on Slack here. Note that I'm not sure whether there are other places in Materialize where negative timestamps break things, but this at least fixes the issue for the Avro decoder.
Tips for reviewer
See the
// TODOcomment in the codebase. I've filed an issue with Chrono here: chronotope/chrono#903Checklist
This PR has adequate test coverage / QA involvement has been duly considered.
This PR evolves an existing
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way) and therefore is tagged with aT-protolabel.This PR includes the following user-facing behavior changes: