catalog: big cleanup of durable catalog types #34583
Draft
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
This commit cleans up the durable catalog types, by removing useless `Option`s and by removing useless submodules.
teskje
force-pushed
the
big-catalog-types-cleanup
branch
from
82671a2 to
3b62874
Compare
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.
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.
In the durable catalog, some of the `StateUpdateKind`s were named differently in the `mz-catalog-proto` types than in the higher-level types. This commit fixes these inconsistencies by adopting the naming of the higher-level types everywhere.
This type existed because protobuf treats absent strings as empty strings, rather than `None`. Not needed anymore since the removal of protobuf.
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.
teskje
force-pushed
the
big-catalog-types-cleanup
branch
from
3b62874 to
8b54e6f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.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 only +100 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.