catalog: big cleanup of durable catalog types #34583
Merged
+4,524
−4,857
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
teskje
force-pushed
the
big-catalog-types-cleanup
branch
2 times, most recently
from
3b62874 to
8b54e6f
Compare
teskje
force-pushed
the
big-catalog-types-cleanup
branch
3 times, most recently
from
7829c11 to
1922343
Compare
antiguru
approved these changes
Jan 6, 2026
Member
antiguru
left a comment
antiguru
left a comment
There was a problem hiding this comment.
LGTM. I didn't check that the enum values match, but since this is a mechanical translation I trust that you checked the mapping!
This commit cleans up the durable catalog types, by removing useless `Option`s and by removing useless submodules.
In the protobuf-derived format the `Empty` type was necessary because
protobuf cannot represent union enum variants, so we'd end up with,
e.g., `GlobalId::Explain(Empty {})`. This can now just be
`GlobalId::Explain` and the `Empty` type isn't needed anymore.
Note that we can't fully delete it because the JSON representation of
the audit log objects cannot change, so we need to keep `Empty` around
for those.
This commit removes the `Timestamp` durable catalog type, since it isn't used anywhere.
This commit changes the durable catalog objects to reference C-style enums directly, rather than only container `i32` fields and leaving the mapping to enum variants to higher-level code.
This commit adds a big migration from the previous protobuf-structured format to the new denoised format. Some new special handling needs to be introduced in the `objects!` macro in `upgrade.rs`, due to the new format having less indirection than the old format. We'll be able to remove that again eventually, when we can delete the old object definitions.
To make the `JsonCompatible` tests work correctly for those types where we removed useless `Option`s in the transition from v78 to v79, we have to ensure that the `Arbitrary` implementations for the v78 types don't produce `None` values for these `Option`s, as they couldn't be decoded by the v79 types. Same for types that now contain enum values directly, where we need to ensure `Arbitrary` doesn't generate invalid variant IDs.
teskje
force-pushed
the
big-catalog-types-cleanup
branch
from
1922343 to
b5bf0cc
Compare
Contributor
Author
|
TFTR! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, we removed the catalog's dependency on protobuf, but left the durable catalog types in a protobuf-style format, with a bunch of unnecessary indirection in the form of
Options and submodules. This PR cleans all that up by removing the unnecessary indirection. It also does a bunch of other cleanups to make the type definitions more idiomatic Rust, like removing theEmptytype and referencing C-style enums directly, rather than theiri32representations. Finally, we perform a big migration to move the catalog from the previous format into the new cleaned up one.Unfortunately, we need to keep some of the protobuf-isms around under the
AuditLoghierarchy. That's because the audit log types don't go through migration, so their JSON representation cannot change. Luckily,serde_jsonserializes bothOption<T>andTthe same way (the former having a possiblenullvalue), so we can still delete unnecessaryOptions. But we do need to keep aroundEmpty,StringWrapper, and the likes.Motivation
Part of https://github.com/MaterializeInc/database-issues/issues/9792
Tips for reviewer
Sorry this got so large! I could have submitted the different cleanup steps as different PRs, but then I'd have to write a migration for each of those. Migrations should be less of a hassle to write now that all the
Options are gone, but they are still a hassle. On the bright side, the total diff is net negative lines, even though this PR adds a new objects version and a large migration :DThe individual commits are meaningful and have meaningful descriptions. I recommend reviewing them separately.
I also recommend reviewing with whitespace disabled, at least the first commit. It removes a bunch of submodules, which changes a lot of indentation in
objects.rs.Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.