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

Re-implement Serialize and Deserialize for Catalog as is done for its nested types #25574

Open
hiltontj opened this issue Nov 21, 2024 · 0 comments · May be fixed by #25730
Open

Re-implement Serialize and Deserialize for Catalog as is done for its nested types #25574

hiltontj opened this issue Nov 21, 2024 · 0 comments · May be fixed by #25730
Assignees
Labels

Comments

@hiltontj
Copy link
Contributor

hiltontj commented Nov 21, 2024

Problem

The Catalog type does not implement the Deserialize trait, which means that if we try to nest it in another struct that needs to implement Deserialize, then we run into headaches.

Proposed solution

There exist several _Snapshot types in the serialize.rs module that provide the minimum needed info for de/serialization of their related type in the catalog, e.g., DatabaseSchema/DatabaseSnapshot, TableDefinition/TableSnapshot, etc.

I propose that we re-use this pattern at the top level, and define a CatalogSnapshot in the same serialize.rs module, along with a impl Deserialize/impl Serialize for the Catalog type that uses it.

Alternatives

  • Implement Deserialize for Catalog as is. This should be as simple as
// current code:
#[derive(Debug)]
pub struct Catalog {
    inner: RwLock<InnerCatalog>,
}

// change to:
#[derive(Debug, Deserialize, Serialize)]
pub struct Catalog {
    #[serde(flatten)] // <- this is important
    inner: RwLock<InnerCatalog>,
}

Additional context

The reason for the use of these _Snapshot types is to avoid serializing/deserializing the bi-directional maps of ID <-> name at the various levels of the catalog. Since the info in those maps is already intrinsic to the map of ID -> object (which includes the name), there is no need to also serialize/deserialize them. The top level is still serializing/deserializing the bi-directional map for DbId <-> name, so following through with this would avoid that.

@hiltontj hiltontj added the v3 label Nov 21, 2024
@mgattozzi mgattozzi self-assigned this Dec 31, 2024
mgattozzi added a commit that referenced this issue Jan 2, 2025
This was fairly easy all things considered. We needed to turn on the
serde feature for parking_lot, remove the Serialize impl, derive both it
and Deserialize, and add the flatten attribute and we're all set. The
output for the tests changed, but only the order of fields, not the
actual content so it's fine that we updated them. This change will allow
us to deserialize the Catalog in the future if need be without running
into issues around the fact that it can't implement it.

Closes #25574
@mgattozzi mgattozzi linked a pull request Jan 2, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants