Skip to content
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

adapter: fix merge skew in vars declaration #18004

Merged
merged 3 commits into from
Mar 8, 2023

Conversation

aalexandrov
Copy link
Contributor

@aalexandrov aalexandrov commented Mar 8, 2023

Fixes a merge skew regression in some declaration of a fixed sized array.

Motivation

  • This PR fixes a recognized bug.

There was a merge skew with #17526 and #17966.

Tips for reviewer

N/A

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.
  • This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration, 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:
    • There are no user-facing behavior changes.

@aalexandrov aalexandrov requested a review from a team as a code owner March 8, 2023 17:52
@@ -1193,7 +1193,7 @@ impl SystemVars {
/// Returns an iterator over the configuration parameters and their current
/// values on disk.
pub fn iter(&self) -> impl Iterator<Item = &dyn Var> {
let vars: [&dyn Var; 22] = [
let vars: [&dyn Var; 23] = [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a way to not declare the type or use some type inference magic please feel free to force-push!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parker-timmerman any thoughts?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we could do let vars: [&dyn Var; _] = ... and it would infer the length of the array, unfortunately that syntax is gated behind an unstable feature generic_arg_infer.

What we could do though is put &dyn Var on the first element i.e.

let vars = [
    &self.config_has_synced_once as &dyn Var,
    &self.max_aws_privatelink_connections,
    ...
];

which tells the compiler that we want to treat these as &dyn Var, and allows it to infer the length.

It doesn't look great, but it's the only way I know of to get the type inference here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh I'm surprised the compiler didn't complain about the array being the wrong length 🤔
jk, just realized this was the result of a merge skew

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like that suggestion. This is not the first time this exact merge skew issue has happened, and it probably won't be the last.

@jkosh44 jkosh44 enabled auto-merge (squash) March 8, 2023 18:20
@jkosh44 jkosh44 merged commit d60b95f into MaterializeInc:main Mar 8, 2023
@aalexandrov aalexandrov deleted the fix_vars_merge_skew branch March 9, 2023 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants