Update rust version to 1.77, chrono to 0.4.35 by rjobanp · Pull Request #26273 · MaterializeInc/materialize

rjobanp · 2024-03-26T16:09:59Z

Motivation

A recent update to https://github.com/MaterializeInc/rust-postgres/ uses new chrono features and upgrades the version of rust to 1.77. That update of rust-postgres is needed in #26186 so I decided to go ahead and update the version for MZ.

The majority of the changes are due to the deprecations of many timestamp methods on chrono::NaiveDateTime -- see https://github.com/chronotope/chrono/releases/tag/v0.4.35 for details.

Tips for reviewer

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:

jkosh44

Adapter changes LGTM

src/adapter/src/coord/appends.rs

bin/ci-builder

teskje

Compute changes look good, but I don't think SinkToken should be marked as dead code.

src/compute/src/compute_state.rs

rjobanp · 2024-03-28T20:38:43Z

I just fixed what appears to be the last test failure (just kicked off another full re-run), if anyone wants to be a hero and do a final review/approval

ParkMyCar

Thanks for making this change!

Would love to get this merged soon because AFAICT if you add a new crate dependency Cargo.lock will get updated to the lastest commit of our fork and we'll fail to build MZ

ParkMyCar · 2024-03-29T14:01:42Z

misc/python/materialize/mzbuild.py

+                # parse the package name from a package_id that looks like one of:
+                # git+https://github.com/MaterializeInc/rust-server-sdk#launchdarkly-server-sdk@1.0.0
+                # path+file:///Users/roshan/materialize/src/catalog#mz-catalog@0.0.0
+                # registry+https://github.com/rust-lang/crates.io-index#num-rational@0.4.0
+                # file:///path/to/my-package#0.1.0
+                package_id = message["package_id"]
+                if "@" in package_id:
+                    package = package_id.split("@")[0].split("#")[-1]
+                else:
+                    package = message["package_id"].split("#")[0].split("/")[-1]


Why was this change required? Maybe the upgrade to Rust 1.77 changed output of cargo build?

Yes the spec of package-id was updated in the json output: rust-lang/cargo#13311

took a while to figure this out.

ParkMyCar · 2024-03-29T14:11:39Z

src/compute/src/compute_state.rs


 /// A token that keeps a sink alive.
-pub struct SinkToken(Box<dyn Any>);
+pub struct SinkToken(#[allow(dead_code)] Box<dyn Any>);


Suggested change

pub struct SinkToken(#[allow(dead_code)] Box<dyn Any>);

pub struct SinkToken {

_guard: Box<dyn Any>,

}

This and the other sites triggering "dead code" warning are because the inner fields aren't technically read, we just rely on their Drop impls getting called. If you make this a named field it should remove the need for #[allow(dead_code)]

Yeah that'll be cleaner! Discussed offline with @ParkMyCar -- will address these in a follow up PR to get this update in asap since it'll take some time to update all instantiations of the relevant structs

ParkMyCar · 2024-03-29T14:17:57Z

src/interchange/src/avro/decode.rs

 // Get around orphan rule
 #[derive(Debug)]
-pub(super) struct RowWrapper(pub Row);
+pub(super) struct RowWrapper(#[allow(dead_code)] pub Row);


Row doesn't seem to be a guard here, but I'm not sure if we ever actually read from the created Row? Could you leave a comment here explaining that this might not actually be dead?

ParkMyCar · 2024-03-29T14:20:44Z

src/repr/src/adt/date.rs

+    /// 5874897-12-31, chrono is limited to December 31, 262142, which we mirror
    /// here so we can use chrono's formatting methods and have guaranteed safe
    /// conversions.
-    pub const HIGH_DAYS: i32 = 95_015_644;
+    pub const HIGH_DAYS: i32 = 95_015_279;


This is a little scary because a date could exist in MZ today that is after the new limit, do you know what would happen in this case? probably a panic?

Yes unfortunately a panic will likely occur if a date of this value already exists. I mentioned to @benesch yesterday, I'm hopeful that since the max limit was defined by chrono and not by postgres, it's unlikely that any of our customers used it as some sort of notional timestamp maximum

Makes sense!

rjobanp requested a review from a team as a code owner March 26, 2024 16:10

rjobanp requested a review from a team March 26, 2024 16:10

rjobanp requested a review from a team as a code owner March 26, 2024 16:10

rjobanp requested a review from a team March 26, 2024 16:10

rjobanp requested review from a team, aalexandrov and benesch as code owners March 26, 2024 16:10

rjobanp requested a review from jkosh44 March 26, 2024 16:10

jkosh44 approved these changes Mar 26, 2024

View reviewed changes

src/adapter/src/coord/appends.rs Outdated Show resolved Hide resolved

def- reviewed Mar 26, 2024

View reviewed changes

bin/ci-builder Outdated Show resolved Hide resolved

rjobanp force-pushed the rust-77 branch from 0afa354 to d3b04f3 Compare March 26, 2024 17:38

rjobanp mentioned this pull request Mar 26, 2024

storage: Deny localhost/private-ips in storage connections #26186

Merged

5 tasks

rjobanp force-pushed the rust-77 branch from d3b04f3 to de8ed6d Compare March 26, 2024 19:12

teskje requested changes Mar 27, 2024

View reviewed changes

src/compute/src/compute_state.rs Outdated Show resolved Hide resolved

rjobanp force-pushed the rust-77 branch 13 times, most recently from 1739b49 to 7cd47f0 Compare March 28, 2024 19:59

Update rust version to 1.77, chrono to 0.4.35

d7c1be4

rjobanp force-pushed the rust-77 branch from 7cd47f0 to d7c1be4 Compare March 28, 2024 20:37

teskje approved these changes Mar 29, 2024

View reviewed changes

ParkMyCar approved these changes Mar 29, 2024

View reviewed changes

rjobanp merged commit bb22508 into MaterializeInc:main Mar 29, 2024

-pub struct SinkToken(#[allow(dead_code)] Box<dyn Any>);
+pub struct SinkToken {
+  _guard: Box<dyn Any>,
+}

Comments

Conversation

rjobanp commented Mar 26, 2024

Motivation

Tips for reviewer

Checklist

Uh oh!

jkosh44 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

teskje left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rjobanp commented Mar 28, 2024

Uh oh!

ParkMyCar 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants