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

WIP: updating Schema and system tables #1662

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 26 additions & 19 deletions crates/core/src/db/datastore/system_tables.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,26 +173,30 @@ macro_rules! st_fields_enum {
// WARNING: For a stable schema, don't change the field names and discriminants.
st_fields_enum!(enum StTableFields {
"table_id", TableId = 0,
"table_name", TableName = 1,
"name", TableName = 1,
"table_type", TableType = 2,
"table_access", TablesAccess = 3,
});
Copy link
Contributor

@cloutiertyler cloutiertyler Sep 6, 2024

Choose a reason for hiding this comment

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

If you're going to rename table_name to name, we should probably do table_type and table_access. Incidentally, I don't know what table_type even is.

Better yet, I would just leave these names as is to reducer code churn and errors and then we can do a final pass at the end. So leave it as:

st_fields_enum!(enum StTableFields {
    "table_id", TableId = 0,
    "table_name", TableName = 1,
    "table_name", TableName = 1,
    "table_type", TableType = 2,
    "table_access", TablesAccess = 3,
});

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree to keep the prefix for the columns, that helps with debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah and joins.

// WARNING: For a stable schema, don't change the field names and discriminants.
st_fields_enum!(enum StColumnFields {
"table_id", TableId = 0,
"col_pos", ColPos = 1,
"col_name", ColName = 2,
"col_type", ColType = 3,
"name", ColName = 2,
"type", ColType = 3,
});
// WARNING: For a stable schema, don't change the field names and discriminants.
st_fields_enum!(enum StIndexFields {
"index_id", IndexId = 0,
"table_id", TableId = 1,
"index_name", IndexName = 2,
"columns", Columns = 3,
"is_unique", IsUnique = 4,
"index_type", IndexType = 5,
});
st_fields_enum!(
enum StIndexFields {
"index_id", IndexId = 0,
"table_id", TableId = 1,
"index_name", IndexName = 2,

// TODO(jgilles): can a system table store an enum?
// What if we need to add new variants to the enum? Will that break
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, system tables can store any SATN type.

// the schema? Does this need to store a byte array instead?
"index_algorithm", IndexAlgorithm = 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

This unfortunately doesn't tell me what the type of this field is.

Could you put a schema for all the tables in a comment like this:

| table_id | table_name  | table_type | foobar          |
|----------|-------------|------------|-----------------|
| u32      | String      | u32        | IndexAlgorithm  |

enum IndexAlgorithm {
    Foobar(String)
}

Or something similar? It's just hard to comment on the changes without the type info.

}
);
// WARNING: For a stable schema, don't change the field names and discriminants.
st_fields_enum!(
/// The fields that define the internal table [crate::db::relational_db::ST_SEQUENCES_NAME].
Expand All @@ -208,13 +212,16 @@ st_fields_enum!(
"allocated", Allocated = 8,
});
// WARNING: For a stable schema, don't change the field names and discriminants.
st_fields_enum!(enum StConstraintFields {
"constraint_id", ConstraintId = 0,
"constraint_name", ConstraintName = 1,
"constraints", Constraints = 2,
"table_id", TableId = 3,
"columns", Columns = 4,
});
st_fields_enum!(
enum StConstraintFields {
"constraint_id", ConstraintId = 0,
"name", ConstraintName = 1,
// TODO(jgilles): can a system table store an enum?
// What if we need to add new variants to the enum? Will that break
// the schema? Does this need to store a byte array instead?
"data", ConstraintData = 3,
}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect this to mirror how constraints are represented in ModuleDef. If we're going to have an enum here, that's what we should do in ModuleDef as well.

Notably postgres appears to put them all in a single table, but I would like you to confirm that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah JK you seem to have done that below.

// WARNING: For a stable schema, don't change the field names and discriminants.
st_fields_enum!(enum StModuleFields {
"database_address", DatabaseAddress = 0,
Expand All @@ -233,10 +240,10 @@ st_fields_enum!(enum StVarFields {
"name", Name = 0,
"value", Value = 1,
});

st_fields_enum!(enum StScheduledFields {
"table_id", TableId = 0,
"reducer_name", ReducerName = 1,
"name", Name = 2,
});

/// System Table [ST_TABLE_NAME]
Expand Down
17 changes: 12 additions & 5 deletions crates/lib/src/db/raw_def/v9.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,8 @@ pub struct RawTableDefV9 {
/// The indices of the table.
pub indexes: Vec<RawIndexDefV9>,

/// Any unique constraints on the table.
pub unique_constraints: Vec<RawUniqueConstraintDefV9>,
/// Any constraints on the table.
pub constraints: Vec<RawConstraintDefV9>,

/// The sequences for the table.
pub sequences: Vec<RawSequenceDefV9>,
Expand Down Expand Up @@ -272,6 +272,13 @@ pub struct RawUniqueConstraintDefV9 {
pub columns: ColList,
}

#[derive(Debug, Clone, ser::Serialize, de::Deserialize)]
#[cfg_attr(feature = "test", derive(PartialEq, Eq, PartialOrd, Ord))]
#[non_exhaustive]
pub enum RawConstraintDefV9 {
Unique(RawUniqueConstraintDefV9),
}

/// Marks a table as a timer table for a scheduled reducer.
///
/// The table must have columns:
Expand Down Expand Up @@ -397,7 +404,7 @@ impl RawModuleDefV9Builder {
name,
product_type_ref,
indexes: vec![],
unique_constraints: vec![],
constraints: vec![],
sequences: vec![],
schedule: None,
primary_key: None,
Expand Down Expand Up @@ -570,8 +577,8 @@ impl<'a> RawTableDefBuilder<'a> {
pub fn with_unique_constraint(mut self, columns: ColList, name: Option<RawIdentifier>) -> Self {
let name = name.unwrap_or_else(|| self.generate_unique_constraint_name(&columns));
self.table
.unique_constraints
.push(RawUniqueConstraintDefV9 { name, columns });
.constraints
.push(RawConstraintDefV9::Unique(RawUniqueConstraintDefV9 { name, columns }));
self
}

Expand Down
23 changes: 17 additions & 6 deletions crates/schema/src/def.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,8 +329,8 @@ pub struct TableDef {
/// The indices on the table, indexed by name.
pub indexes: IdentifierMap<IndexDef>,

/// The unique constraints on the table, indexed by name.
pub unique_constraints: IdentifierMap<UniqueConstraintDef>,
/// The constraints on the table, indexed by name.
pub constraints: IdentifierMap<ConstraintDef>,

/// The sequences for the table, indexed by name.
pub sequences: IdentifierMap<SequenceDef>,
Expand Down Expand Up @@ -376,7 +376,7 @@ impl From<TableDef> for RawTableDefV9 {
product_type_ref,
primary_key,
indexes: to_raw(indexes, |index: &RawIndexDefV9| &index.name),
unique_constraints: to_raw(unique_constraints, |constraint: &RawUniqueConstraintDefV9| {
constraints: to_raw(unique_constraints, |constraint: &RawUniqueConstraintDefV9| {
&constraint.name
}),
sequences: to_raw(sequences, |sequence: &RawSequenceDefV9| &sequence.name),
Expand Down Expand Up @@ -518,15 +518,26 @@ pub struct ColumnDef {
pub table_name: Identifier,
}

/// A constraint definition.
pub struct ConstraintDef {
/// The name of the constraint.
pub name: Identifier,
/// The data for the constraint.
pub data: ConstraintData,
}

/// Data for a constraint.
#[non_exhaustive]
pub enum ConstraintData {
Unique(UniqueConstraintDef),
}

/// Requires that the projection of the table onto these columns is an bijection.
///
/// That is, there must be a one-to-one relationship between a row and the `columns` of that row.
#[derive(Debug, Clone, Eq, PartialEq)]
#[non_exhaustive]
pub struct UniqueConstraintDef {
/// The name of the unique constraint. Must be unique within the containing `RawDatabaseDef`.
pub name: Identifier,

/// The columns on the containing `TableDef`
pub columns: ColList,
}
Expand Down
2 changes: 1 addition & 1 deletion crates/schema/src/def/validate/v8.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ fn upgrade_table(
product_type_ref,
primary_key,
indexes,
unique_constraints,
constraints: unique_constraints,
sequences,
schedule,
table_type,
Expand Down
2 changes: 1 addition & 1 deletion crates/schema/src/def/validate/v9.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ impl ModuleValidator<'_> {
product_type_ref,
primary_key,
indexes,
unique_constraints,
constraints: unique_constraints,
sequences,
schedule,
table_type,
Expand Down
67 changes: 39 additions & 28 deletions crates/schema/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ pub struct TableSchema {
/// The unique identifier of the table within the database.
pub table_id: TableId,
/// The name of the table.
pub table_name: Box<str>,
pub name: Box<str>,
/// The columns of the table.
/// Inaccessible to prevent mutation.
/// The ordering of the columns is significant. Columns are frequently identified by `ColId`, that is, position in this list.
Expand All @@ -61,8 +61,10 @@ pub struct TableSchema {
pub table_type: StTableType,
/// The visibility of the table.
pub table_access: StAccess,
pub scheduled: Option<Box<str>>,
pub scheduled: Option<ScheduleSchema>,
/// Cache for `row_type_for_table` in the data store.
/// Not stored in resolved form, may contain references to the module's
/// `Typespace`.
row_type: ProductType,
}

Expand Down Expand Up @@ -714,6 +716,7 @@ pub struct ColumnSchema {
/// The name of the column. Unique within the table.
pub col_name: Box<str>,
/// The type of the column.
/// May refer to the `Typespace` of the module.
pub col_type: AlgebraicType,
}

Expand Down Expand Up @@ -804,8 +807,7 @@ pub struct SequenceSchema {
/// The unique identifier for the sequence within a database.
pub sequence_id: SequenceId,
/// The name of the sequence.
/// Deprecated. In the future, sequences will be identified by col_pos.
pub sequence_name: Box<str>,
pub name: Box<str>,
/// The ID of the table associated with the sequence.
pub table_id: TableId,
/// The position of the column associated with this sequence.
Expand Down Expand Up @@ -893,23 +895,20 @@ impl From<SequenceSchema> for RawSequenceDefV8 {

/// A struct representing the schema of a database index.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct IndexSchema {
/// The unique ID of the index within the schema.
pub enum IndexSchema {
BTree(BTreeIndexSchema),
}

/// A BTree index.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct BTreeIndexSchema {
/// The ID of the index.
pub index_id: IndexId,
/// The ID of the table associated with the index.
/// The ID of the table to which the index is attached.
pub table_id: TableId,
/// The type of the index.
pub index_type: IndexType,
/// The name of the index. This should not be assumed to follow any particular format.
/// Unique within the table.
// TODO(jgilles): check and/or specify if this is currently unique within the database.
pub index_name: Box<str>,
/// If the index is unique.
pub is_unique: bool,
/// The list of columns associated with the index.
/// This is truly a list: the order of the columns is significant.
/// The columns are projected and serialized to bitstrings in this order,
/// which affects the order of elements within a BTreeIndex.
/// The name of the index.
pub name: Box<str>,
/// The columns of the index.
pub columns: ColList,
}

Expand Down Expand Up @@ -960,21 +959,33 @@ impl From<IndexSchema> for RawIndexDefV8 {
}

/// A struct representing the schema of a database constraint.
///
/// This struct holds information about a database constraint, including its unique identifier,
/// name, the table it belongs to, and the columns it is associated with.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ConstraintSchema {
/// The unique ID of the constraint within the database.
/// The unique identifier for the constraint within a database.
pub constraint_id: ConstraintId,
/// The name of the constraint.
/// Deprecated, in the future constraints will be identified by the columns they refer to and their constraint type.
pub constraint_name: Box<str>,
/// The constraints applied to the columns
pub constraints: Constraints,
/// The ID of the table the constraint applies to.
/// The data attached to the constraint.
pub constraint_data: ConstraintData,
}

/// A struct representing the schema of a database constraint.
///
/// This struct holds information about a database constraint, including its unique identifier,
/// name, the table it belongs to, and the columns it is associated with.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum ConstraintData {
Unique(UniqueConstraintSchema),
}

/// A unique constraint.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct UniqueConstraintSchema {
/// A unique constraint applies to columns of this table.
pub table_id: TableId,
/// The columns the constraint applies to.
/// Projecting the table onto these columns must result in a table with the same cardinality
/// (set-theoretically).
/// I.e. if you project onto these columns and deduplicate, you shouldn't need to remove any rows.
pub columns: ColList,
}

Expand Down
Loading