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 'constraints' markers to primitive and fix incompatibility with deploy #561

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

mamcx
Copy link
Contributor

@mamcx mamcx commented Nov 14, 2023

Description of Changes

Refactor the definition of constraints into the primitives crate, removing duplications of logic and fix bug when

#[spacetimedb(table)]
struct Foo {
    #[primarykey]
    id: u32,
    #[unique]
    #[autoinc]
    other: u32
}

gives error Multiple primary columns defined for table, which shows up in the deployment of spacetimedb.com

API and ABI breaking changes

This changes the bits stored in the st_constraints system table.

Expected complexity level and risk

1

@mamcx mamcx added the abi-break A PR that makes an ABI breaking change label Nov 14, 2023
@mamcx mamcx self-assigned this Nov 14, 2023
//!
//! A column that is made up of values generated by the database using a sequence.
//!
//! Differs from `PRIMARY_KEY` in that its values are managed by the database and usually cannot be modified.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: Currently IDENTITY implies UNIQUE but that is wrong: https://en.wikipedia.org/wiki/Identity_column.

However, to change this need we review how others are using this.

@@ -52,6 +52,15 @@ pub struct Point {
y: i64,
}

// Test we can compile multiple constraints
#[spacetimedb(table)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is added to check it compiles.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good. You might have to change an integration smoke test which reads specific log lines because another table will add a log line and through the numbers off by one.

//! Additionally, is possible to add markers to columns as:
//!
//! - AUTO_INC: Auto-generate a sequence
//! - INDEXED: Auto-generate a non-unique index
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: AUTO_INC right now is semantically ambiguous. it clashes with the purpose of IDENTITY (and is possible to have many of it) which is the recommended one when asked for auto-generated numbers.

I think we should use instead SEQUENCE and deprecate the use of this tag.

Copy link
Contributor

@kulakowski kulakowski left a comment

Choose a reason for hiding this comment

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

The code change itself looks good to me. I want to make sure we discuss in chat how we are going agree on the semantics you bring up here, before approving. Thanks for the pointers to documentation about it, Mario.

@@ -52,6 +52,15 @@ pub struct Point {
y: i64,
}

// Test we can compile multiple constraints
#[spacetimedb(table)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good. You might have to change an integration smoke test which reads specific log lines because another table will add a log line and through the numbers off by one.

@mamcx mamcx requested a review from kim November 14, 2023 21:24
Copy link
Contributor

@kim kim left a comment

Choose a reason for hiding this comment

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

Thanks! <3 the docs


/// The assigned constraint for a `Table`
#[allow(non_camel_case_types)]
#[derive(Debug, Clone, Copy, Eq, PartialEq, PartialOrd, Ord)]
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO these are all confused. We should just have attributes which are separate from constraints:

IMO these shouldn't even be on the same axis.

We have the following column attributes:

  • auto_inc

We should have the following table attributes:

  • primary_key e.g. (col_a, col_b)
  • indexed e.g. (col_a, col_b)

We should have the following table constraints:

  • unique e.g. (col_a, col_b)

These are all mutually exclusive and should not be combined into a single "kind"

Also constraints != attributes!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be addressed in a separate PR, this one is to solve the immediate issue with deploy.

@mamcx mamcx force-pushed the mamcx/constraint_flags branch from fd57740 to d65c99d Compare November 15, 2023 15:54
@mamcx mamcx merged commit f3a5dc2 into master Nov 15, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abi-break A PR that makes an ABI breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants