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
Correctly decode negative timestamps with Avro #16609
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are doing the good thing. I copied the new algorithm from the unreleased main of Chrono.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Petros is adding a use in #16622 (review).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let's look into upgrading Chrono in that case.
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
// TODO
comment 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$T
mapping (possibly in a backwards-incompatible way) and therefore is tagged with aT-proto
label.This PR includes the following user-facing behavior changes: