Skip to content

Plain enums as index keys with specialized indices #2506

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

Merged
merged 4 commits into from
Apr 9, 2025

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Mar 26, 2025

Description of Changes

Commits:

  1. Allow plain enums as index keys in Rust and C# bindings. For Rust, FilterableValue is impld when deriving SpacetimeType.
  2. Specialize indices on plain enums by using u8 as the native type. For direct indices on plain enums, the entire index allocated at index creation to the number of tags. This should be quite efficient as the number of tags is expected to be low in the common case.

API and ABI breaking changes

None

Expected complexity level and risk

2?

Testing

SDK tests are added exercising both the host, the modules, and the sdk. New tests are also added for the new fixed capacity direct index type.

@Centril
Copy link
Contributor Author

Centril commented Mar 26, 2025

Requested reviews from:

Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

I reviewed only the parts you asked for, namely the SDK tests (both module and client parts) and the stuff in the table crate.

Minor style nits. Otherwise looks good!

I wonder if we should even bother to use unique BTree indices for simple enums, or if we should just impl a unique index on a simple enum column as a direct index. I wonder the same thing about u8 and i8 columns. This is idle speculation, though.

@bfops bfops linked an issue Mar 31, 2025 that may be closed by this pull request
@bfops bfops added the release-any To be landed in any release window label Mar 31, 2025
@Centril Centril force-pushed the centril/plain-enum-as-index branch from ea92071 to 3abc61d Compare March 31, 2025 19:04
@Centril
Copy link
Contributor Author

Centril commented Mar 31, 2025

I wonder if we should even bother to use unique BTree indices for simple enums, or if we should just impl a unique index on a simple enum column as a direct index. I wonder the same thing about u8 and i8 columns. This is idle speculation, though.

I sorta tried this direction but decided that the user might want to express "this index likely doesn't use all elements" so I skipped that change. It's worth exploring in the future though.

@Centril Centril force-pushed the centril/plain-enum-as-index branch 3 times, most recently from 4fa1f58 to c533311 Compare April 2, 2025 09:52
Copy link
Collaborator

@coolreader18 coolreader18 left a comment

Choose a reason for hiding this comment

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

LGTM

gefjon added a commit to clockworklabs/spacetime-docs that referenced this pull request Apr 3, 2025
Includes references to changes made by
clockworklabs/SpacetimeDB#2506 ,
which as of writing has not merged.
We should not push this commit live until after that PR is released.
@Centril Centril force-pushed the centril/plain-enum-as-index branch 2 times, most recently from 2b508d0 to 8cdac9a Compare April 7, 2025 22:21
@Centril Centril force-pushed the centril/plain-enum-as-index branch from 8cdac9a to 9b3758b Compare April 7, 2025 22:38
@Centril Centril requested a review from gefjon April 7, 2025 22:53
Copy link
Contributor

@kazimuth kazimuth left a comment

Choose a reason for hiding this comment

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

The C# side of this generally makes sense. I would prefer to implement IEquatable for plain enums marked with [SpacetimeDB.Type] and keep the bounds on various interfaces -- I think I missed implementing IEquatable for plain enums in my IEquatable PR.

@Centril Centril enabled auto-merge April 9, 2025 18:27
@Centril Centril added this pull request to the merge queue Apr 9, 2025
Merged via the queue into master with commit c39f7fa Apr 9, 2025
13 of 14 checks passed
gefjon added a commit to clockworklabs/spacetime-docs that referenced this pull request Apr 16, 2025
Includes references to changes made by
clockworklabs/SpacetimeDB#2506 ,
which as of writing has not merged.
We should not push this commit live until after that PR is released.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bitcraft request: allow using enums as index keys
5 participants