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

Move schemas to schema crate, rename Def to RawDefV8 #1498

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

kazimuth
Copy link
Contributor

@kazimuth kazimuth commented Jul 11, 2024

Description of Changes

This is in preparation for further schema rewrites, extracted from #1448, based on #1479. There are only name changes (well, and one bindings change due to the name changes).

I've chosen aggressive, ugly names for the existing Def types. This is because soon, these types will be considered unvalidated user input, which should not be used anywhere except at the module boundary, where they are immediately validated and converted to trusted types. I want these types to stick out in code to encourage migration away from them.

API and ABI breaking changes

This is technically an API break, but the APIs involved are only used by macros, so 🤷

Expected complexity level and risk

0

@kazimuth kazimuth requested a review from cloutiertyler as a code owner July 11, 2024 22:05
@kazimuth kazimuth requested review from Centril and removed request for cloutiertyler July 11, 2024 22:05
@kazimuth kazimuth force-pushed the jgilles/schemacrate branch 2 times, most recently from b0093d4 to 084805e Compare July 15, 2024 16:06
@kazimuth kazimuth force-pushed the jgilles/schemacrate branch from 084805e to 55688bd Compare July 15, 2024 16:42
@bfops bfops added the release-any To be landed in any release window label Jul 15, 2024
Copy link
Contributor

@mamcx mamcx left a comment

Choose a reason for hiding this comment

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

LGTM.

NOTE: The removal of names for sequence and such reordering break the system tables, so I think this should be marked.

@kazimuth
Copy link
Contributor Author

Yeah, which is why I might not remove them after all, together with SQL compatibility. But this PR doesn't actually implement that, it just renames things to make future PRs easier to read.

@kazimuth
Copy link
Contributor Author

WAIT A MINUTE. We're on ABI version 8. Should this all be renamed RawModuleDefV8? Or possibly V9, since we are about to introduce an ABI break to make this versioned?

@cloutiertyler
Copy link
Contributor

cloutiertyler commented Jul 15, 2024

Two suggestions:

  1. Use v8 instead of v0
  2. remove the V0 from the type and make it v8::RawTableDef instead using a rust module

I implemented these changes here: https://github.com/clockworklabs/SpacetimeDB/pull/1519/files

Pros:

  • easier to rename later if necessary
  • arguably cleaner in that it allows us to eventually put all handling of a particular version under a single Rust module in the future

Cons:

  • harder to tell which structure is in use at a glance

Thoughts?

@mamcx
Copy link
Contributor

mamcx commented Jul 17, 2024

Good question. I think if we need constant warnings then having the version in the structs is better, else if wanna clean refactorings in the module.

@kazimuth kazimuth force-pushed the jgilles/schemacrate branch from 05c123d to 784d6a6 Compare July 17, 2024 17:07
Base automatically changed from jgilles/movedb to master July 17, 2024 23:50
@kazimuth kazimuth force-pushed the jgilles/schemacrate branch from 784d6a6 to 0621b3e Compare July 18, 2024 19:48
@kazimuth kazimuth changed the title Move schemas to schema crate, rename Def to RawDefV0 Move schemas to schema crate, rename Def to RawDefV8 Jul 18, 2024
@kazimuth
Copy link
Contributor Author

kazimuth commented Jul 19, 2024

Yeah, we want warnings because these are now considered unvalidated user input types, and we want people to move to the new immutable + validated ModuleDef in my upcoming PR.

@kazimuth kazimuth force-pushed the jgilles/schemacrate branch from beb530c to f8d7689 Compare July 19, 2024 17:28
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

This looks good to me. I think we're going to have to coordinate merging with with @gefjon's P0

@kazimuth
Copy link
Contributor Author

Rebased over that, this should be good to go

@kazimuth kazimuth added this pull request to the merge queue Jul 24, 2024
Merged via the queue into master with commit 45b2cee Jul 24, 2024
7 checks passed
@kazimuth kazimuth deleted the jgilles/schemacrate branch July 24, 2024 18:00
@bfops bfops added the api-break A PR that makes an API breaking change label Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break A PR that makes an API breaking change release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants