Refactoring IDs for system objects #439

mamcx · 2023-10-17T22:13:01Z

Description of Changes

Note: This is a multi-step set of PRs to bring constraints + multi-column indexes

Step:

Add system tables
Add multi-column indexes
Rework schema definition
Refactoring IDs for system objects <-- THIS PR

This PR is to simplify the review of #267. It only have the movement of schema objects and the use of newtypes for system objects.

API and ABI

This is a breaking change to the module ABI
This is a breaking change to the module API
This is a breaking change to the ClientAPI
This is a breaking change to the SDK API

If the API is breaking, please state below what will break

Centril

Some more ideas on how to further split this PR.

crates/core/src/db/datastore/system_tables.rs

crates/sats/src/relation.rs

crates/sats/src/db/mod.rs

crates/core/src/vm.rs

crates/bindings-macro/src/lib.rs

crates/vm/src/expr.rs

crates/core/src/db/datastore/locking_tx_datastore/btree_index.rs

crates/core/src/db/datastore/locking_tx_datastore/mod.rs

crates/core/src/sql/compiler.rs

crates/core/src/vm.rs

crates/sats/src/product_value.rs

kulakowski

I'm worried about the number of seemingly stray kind-constraint find and replace changes.

Cargo.lock

kulakowski · 2023-11-13T20:03:45Z

crates/bench/src/sqlite.rs

@@ -12,7 +12,7 @@ use std::{
    hint::black_box,
    sync::{Arc, RwLock},
 };
-use tempfile::TempDir;
+use tempdir::TempDir;


Don't change it at this point, but why did this change in this PR?

kulakowski · 2023-11-13T20:06:49Z

crates/core/src/db/commit_log.rs

@@ -248,7 +245,7 @@ impl CommitLogView {
    /// the transactions in a [`Commit`].
    ///
    /// The iterator attempts to read each large object in turn, yielding an
-    /// [`io::Error`] with kind [`io::ErrorKind::NotFound`] if the object was
+    /// [`io::Error`] with constraints [`io::ErrorKind::NotFound`] if the object was


Was this a stray find-and-replace change? It reads oddly now.

You are right. I make a wrong refactor.

kulakowski · 2023-11-13T20:19:24Z

crates/core/src/db/datastore/traits.rs

@@ -223,7 +223,7 @@ pub trait MutTxDatastore: TxDatastore + MutTx {

 /// Describes a programmable [`TxDatastore`].
 ///
-/// A programmable datastore is one which has a program of some kind associated
+/// A programmable datastore is one which has a program of some constraints associated


Ditto on the find-and-replace in a comment.

kulakowski · 2023-11-13T20:20:07Z

crates/core/src/db/relational_db.rs

@@ -477,7 +467,7 @@ impl RelationalDB {
    ///
    /// NOTE: It loads the data from the table into it before returning
    #[tracing::instrument(skip(self, tx, index), fields(index=index.name))]
-    pub fn create_index(&self, tx: &mut MutTxId, index: IndexDef) -> Result<IndexId, DBError> {
+    pub fn create_index(&self, tx: &mut MutTxId, table_id: TableId, index: IndexDef) -> Result<IndexId, DBError> {


Why is table_id plumbed through but not used? Does it get used in #267 ?

Correct. I will remove this change because if is not used is pointless here.

kulakowski · 2023-11-13T20:21:10Z

crates/core/src/db/datastore/system_tables.rs

    "table_id", TableId = 3,
    "columns", Columns = 4,
 });
 // WARNING: For a stable schema, don't change the field names and discriminants.
 st_fields_enum!(enum StModuleFields {
    "program_hash", ProgramHash = 0,
-    "kind", Kind = 1,
+    "constraints", Kind = 1,


It's a bit odd that this one did not change both, but the equivalent in StConstraintsFields did. But maybe that's an artifact of the split from #267?

kulakowski · 2023-11-13T20:21:40Z

crates/core/src/db/datastore/system_tables.rs

@@ -945,7 +941,7 @@ pub struct ModuleKind(u8);

 /// The [`ModuleKind`] of WASM-based modules.
 ///
-/// This is currently the only known kind.
+/// This is currently the only known constraints.


Another constraint-kind doc comment change?

kulakowski · 2023-11-13T20:21:55Z

crates/core/src/db/datastore/system_tables.rs

            x.table_id,
            ArrayValue::from(cols)
        ]
    }
 }

-/// Indicates the kind of module the `program_hash` of a [`StModuleRow`]
+/// Indicates the constraints of module the `program_hash` of a [`StModuleRow`]


Ditto. I'm a bit concerned about these.

kulakowski · 2023-11-13T20:22:56Z

crates/core/src/db/datastore/locking_tx_datastore/mod.rs

@@ -503,7 +505,7 @@ impl Inner {
                    is_unique: false,
                    index_type: IndexType::BTree,
                },
-                x => panic!("Adding constraint of kind `{x:?}` is not supported yet."),
+                x => panic!("Adding constraint of constraints `{x:?}` is not supported yet."),


Yet another.

kulakowski · 2023-11-14T17:59:26Z

crates/bindings/src/lib.rs

@@ -13,22 +13,22 @@ mod timestamp;
 use crate::sats::db::def::IndexType;
 use spacetimedb_lib::buffer::{BufReader, BufWriter, Cursor, DecodeError};
 use spacetimedb_lib::sats::{impl_deserialize, impl_serialize, impl_st};
+pub use spacetimedb_lib::ser::Serialize;


This looks out of place. It should at least be with the other pub use entries.

kulakowski · 2023-11-14T18:07:23Z

crates/sats/src/db/def.rs

@@ -343,24 +332,118 @@ impl From<&ColumnSchema> for ProductTypeElement {
 #[derive(Clone)]
 pub struct ColumnDefMeta {
    pub column: ProductTypeElement,
-    pub attr: ColumnIndexAttribute,
+    pub attr: Constraints,


Nit: should this field be named constraints now?

kulakowski · 2023-11-14T18:07:49Z

crates/sats/src/db/def.rs

+#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Ord)]
+pub struct ProductTypeMeta {
+    pub columns: ProductType,
+    pub attr: Vec<Constraints>,


Nit: should this field be named constraints now?

This will be gone it the last PR, and for now attr is more correct because is for the column, not the table.

kulakowski · 2023-11-14T18:19:58Z

Also, my approval is assuming the chat discussion about autoinc sematics etc is agreed to be good

mamcx mentioned this pull request Oct 17, 2023

Refactoring bootstrapping and adding full support for constraints & multi-column indexes #267

Closed

9 tasks

Centril self-requested a review October 17, 2023 22:47

Centril reviewed Oct 17, 2023

View reviewed changes

crates/sats/src/relation.rs Outdated Show resolved Hide resolved

Centril reviewed Oct 17, 2023

View reviewed changes

crates/sats/src/db/mod.rs Outdated Show resolved Hide resolved

mamcx force-pushed the mamcx/newtype_system_tables branch 3 times, most recently from c8e3f98 to 504a1ef Compare October 18, 2023 15:14

mamcx marked this pull request as draft October 18, 2023 20:51

mamcx force-pushed the mamcx/newtype_system_tables branch 4 times, most recently from 22a4523 to 0021da4 Compare October 20, 2023 17:07

mamcx marked this pull request as ready for review October 20, 2023 17:07

kulakowski reviewed Oct 20, 2023

View reviewed changes

crates/core/src/vm.rs Show resolved Hide resolved

crates/core/src/vm.rs Outdated Show resolved Hide resolved

crates/bindings-macro/src/lib.rs Outdated Show resolved Hide resolved

crates/vm/src/expr.rs Show resolved Hide resolved

Centril requested changes Oct 20, 2023

View reviewed changes

mamcx mentioned this pull request Oct 24, 2023

Moving object defs to SATS #460

Merged

4 tasks

Centril force-pushed the mamcx/newtype_system_tables branch from 0021da4 to 7dba4dc Compare October 29, 2023 16:52

mamcx added the release-0.8 label Oct 30, 2023

Centril force-pushed the mamcx/newtype_system_tables branch from 7dba4dc to b0d8b18 Compare October 30, 2023 16:43

Centril mentioned this pull request Oct 30, 2023

Misc refactoring preparing for multi col indices #497

Merged

4 tasks

mamcx force-pushed the mamcx/newtype_system_tables branch from b0d8b18 to 1cee02c Compare November 13, 2023 18:19

mamcx added the abi-break A PR that makes an ABI breaking change label Nov 13, 2023

mamcx self-assigned this Nov 13, 2023

Centril self-requested a review November 13, 2023 18:39

mamcx marked this pull request as draft November 13, 2023 18:43

mamcx marked this pull request as ready for review November 13, 2023 19:11

Centril force-pushed the mamcx/newtype_system_tables branch 3 times, most recently from ef2f883 to 2813cef Compare November 13, 2023 19:32

Centril force-pushed the mamcx/newtype_system_tables branch 6 times, most recently from 4b04100 to aa3bdc3 Compare November 13, 2023 19:53

Centril approved these changes Nov 13, 2023

View reviewed changes

kulakowski suggested changes Nov 13, 2023

View reviewed changes

kulakowski approved these changes Nov 14, 2023

View reviewed changes

mamcx force-pushed the mamcx/newtype_system_tables branch from d678943 to 9cd06cd Compare November 14, 2023 20:58

mamcx and others added 4 commits November 15, 2023 09:32

Refactoring IDs for system objects

e149b68

fixup

ce3d040

Fix stray constraints text on refactor

877a1ec

Fix conflict

b11669d

mamcx force-pushed the mamcx/newtype_system_tables branch from 9cd06cd to b11669d Compare November 15, 2023 14:54

Merge

051d6f6

mamcx merged commit b3ccc13 into master Nov 15, 2023
5 checks passed

mamcx mentioned this pull request Nov 22, 2023

Refactoring bootstrapping and adding full support for constraints & multi-column indexes #596

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring IDs for system objects #439

Refactoring IDs for system objects #439

mamcx commented Oct 17, 2023

Centril left a comment

kulakowski left a comment

kulakowski Nov 13, 2023

kulakowski Nov 13, 2023

mamcx Nov 14, 2023

kulakowski Nov 13, 2023

kulakowski Nov 13, 2023

mamcx Nov 14, 2023

kulakowski Nov 13, 2023

kulakowski Nov 13, 2023

kulakowski Nov 13, 2023

kulakowski Nov 13, 2023

kulakowski Nov 14, 2023

kulakowski Nov 14, 2023

kulakowski Nov 14, 2023

mamcx Nov 14, 2023

kulakowski commented Nov 14, 2023

Refactoring IDs for system objects #439

Refactoring IDs for system objects #439

Conversation

mamcx commented Oct 17, 2023

Description of Changes

API and ABI

Centril left a comment

Choose a reason for hiding this comment

kulakowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kulakowski commented Nov 14, 2023