-
Notifications
You must be signed in to change notification settings - Fork 457
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
interchange: disallow Protobuf messages with unsigned integers #7623
Conversation
doc/user/content/release-notes.md
Outdated
@@ -46,6 +46,17 @@ Use relative links (/path/to/doc), not absolute links | |||
Wrap your release notes at the 80 character mark. | |||
{{< /comment >}} | |||
|
|||
{{% version-header v0.8.5 %}} |
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.
this is still 0.8.4 unless the plan is to merge it after the branch on monday.
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, thanks.
We are increasingly considering adding unsigned integer types to Materialize. If we do so, we will certainly want to change the ingestion of "uint32" and "uint64" types in Protobuf messages to their corresponding unsigned types in Materialize. We currently ingest these types as "numeric", which is capable of representing all 32- and 64-bit unsigned integers, but loses information about the "unsigned" constraint. This wart would be particularly noticeable in a world with Protobuf sinks, where the following setup message Message { uint32 field f = 1; } CREATE SOURCE src ... FORMAT PROTOBUF MESSAGE '.Message' ... CREATE SINK ... FROM src FORMAT PROTOBUF MESSAGE '.Message' ... would yield a sink that wrote Protobuf messages containing a decimal type instead of uint32. To leave our options open, this commit proposes disabling use of Materialize with unsigned types entirely. While this might break some users, we're almost certain we'll need to make a breaking change here eventually, so better to do it sooner rather than later. If we do get complaints, we can prioritize the unsigned integer work.
2eaad27
to
5749744
Compare
Ok, @umanwizard, this is ready to go pending confirmation that the current customer interested in protobufs does not need unsigned integers. But if we do need unsigned integers, @sploiselle and I put together a spec for their design (#7629) that I think is tractable in a week or two. I added this to the sources and sinks team's "todo" column so the follow up with the customer doesn't get lost. |
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.
LGTM!
rustfmt went a little crazy here but the actual diff is quite small, sorry!
We are increasingly considering adding unsigned integer types to
Materialize. If we do so, we will certainly want to change the ingestion
of "uint32" and "uint64" types in Protobuf messages to their
corresponding unsigned types in Materialize.
We currently ingest these types as "numeric", which is capable of
representing all 32- and 64-bit unsigned integers, but loses information
about the "unsigned" constraint. This wart would be particularly
noticeable in a world with Protobuf sinks, where the following setup
would yield a sink that wrote Protobuf messages containing a decimal
type instead of uint32.
To leave our options open, this commit proposes disabling use of
Materialize with unsigned types entirely. While this might break some
users, we're almost certain we'll need to make a breaking change here
eventually, so better to do it sooner rather than later. If we do get
complaints, we can prioritize the unsigned integer work.