Adding multi-column indexes to the DB #243

mamcx · 2023-08-25T18:42:42Z

Description of Changes

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

Step:

IMPORTANT:

The use of multi-column indexes changes what is stored in the index according to how many fields are defined:

1 Field: Single AlgebraicValue
2 or more: AlgebraicValue::Product

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

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
This is a breaking change to the database system tables

cloutiertyler

Generally seems quite straightforward. Please see the comments.

cloutiertyler · 2023-09-08T22:01:14Z

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

        }
    }

+    pub(crate) fn get_fields(&self, row: &ProductValue) -> Result<AlgebraicValue, DBError> {


This is doing a project of a ProductValue right? Shouldn't it also be returning a product value? Why is it returning an AlgebraicValue?

cloutiertyler · 2023-09-08T22:02:26Z

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

+    pub(crate) cols: NonEmpty<u32>,
    pub(crate) name: String,
    pub(crate) is_unique: bool,
    idx: BTreeSet<IndexKey>,


Shouldn't the IndexKey have a ProductValue now rather than an AlgebraicValue given that it is indexing over multiple (but at least one) columns?

Ok, the major reason for this is because "single columns" are the most common case for a index key, and "multiple columns" is relatively rare.

So, when storing the key, most of the time will be like:

CREATE INDEX a ON t1 (column_i32); -- Most common case index `BuiltIn` CREATE INDEX b ON t1 (column_i32, column_i32); -- Sometimes we have more complex keys

Will be on memory:

[AlgebraicValue::I32(0)] = Row(ProductValue(...)) [AlgebraicValue::Product(AlgebraicValue::I32(0), AlgebraicValue::I32(1))] = Row(ProductValue(...))

The comment in product_value.rs explain the logic

Can this handle the difference between a field with a product value or several fields as a product value? Otherwise we could do something like enum OneOrMore<T> { One(T), More(Vec<T>) } for a tighter representation

Yes it does.

Could you go into how that is achieved / where in the code?

Yes, is here: https://github.com/clockworklabs/SpacetimeDB/pull/243/files#diff-81f8996ebd80b25ce7aa1420ae414b31d532b7dfc46d9aee281b753398b13e2eR77

cloutiertyler · 2023-09-08T22:04:10Z

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

    pub(crate) fn get_rows_that_violate_unique_constraint<'a>(
        &'a self,
-        row: &'a ProductValue,
+        row: &'a AlgebraicValue,


Are rows represented as AlgebraicValues now? Can a row be a SumType or a Builtin type?

cloutiertyler · 2023-09-08T22:57:28Z

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

-                table.indexes.remove(&ColId(col));
-                table.schema.indexes.retain(|x| x.col_id != col);
+                table.indexes.remove(&col.clone().map(ColId));
+                table.schema.indexes.retain(|x| x.cols != col);


This second part could be done by index_id instead of by cols, but I suppose this works as well.

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

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

crates/core/src/db/relational_db.rs

crates/core/src/error.rs

cloutiertyler · 2023-09-08T23:05:27Z

crates/core/src/host/wasm_common/module_host_actor.rs

            for index in table.indexes.iter() {
                let [index_col_id] = *index.col_ids else {
-                    anyhow::bail!("multi-column indexes not yet supported")
+                    //Ignore multi-column indexes


Why are we ignoring multi-column indexes?

This needs constraint to fully resolve this info, and that comes in the next PR.

crates/sats/src/product_value.rs

cloutiertyler · 2023-09-09T07:43:17Z

Looks like you should also give this PR a rebase

Centril · 2023-09-11T21:41:08Z

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

    pub(crate) index_id: IndexId,
    pub(crate) table_id: u32,
-    pub(crate) col_id: u32,
+    pub(crate) cols: NonEmpty<u32>,


Suggested change

pub(crate) cols: NonEmpty<u32>,

pub(crate) cols: NonEmpty<ColId>,

All this suggestions are already applied in the next PR

Centril · 2023-09-11T21:41:34Z

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


 impl BTreeIndex {
-    pub(crate) fn new(index_id: IndexId, table_id: u32, col_id: u32, name: String, is_unique: bool) -> Self {
+    pub(crate) fn new(index_id: IndexId, table_id: u32, cols: NonEmpty<u32>, name: String, is_unique: bool) -> Self {


Suggested change

pub(crate) fn new(index_id: IndexId, table_id: u32, cols: NonEmpty<u32>, name: String, is_unique: bool) -> Self {

pub(crate) fn new(index_id: IndexId, table_id: u32, cols: NonEmpty<ColId>, name: String, is_unique: bool) -> Self {

Centril · 2023-09-11T21:42:54Z

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

        &'a self,
        table_id: &TableId,
-        col_id: &ColId,
+        cols: NonEmpty<u32>,


Suggested change

cols: NonEmpty<u32>,

cols: NonEmpty<ColId>,

Centril · 2023-09-11T21:45:12Z

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

            let mut cols = vec![];
            for index in table.indexes.values_mut() {
                if index.index_id == *index_id {
-                    cols.push(index.col_id);
+                    cols.push(index.cols.clone());
                }
            }


Simpler: table.indexes.values().filter(|i| i.index_id == index_id).map(|i| i.cols.clone()).collect();

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

Centril · 2023-09-11T21:46:29Z

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

+                                col_names: index
+                                    .cols
+                                    .iter()
+                                    .map(|&x| insert_table.schema.columns[x as usize].col_name.clone())
+                                    .collect(),


Let's dedup the code here

Centril · 2023-09-11T21:46:45Z

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

+                        let value = row.project_not_empty(&index.cols)?;
                        return Err(IndexError::UniqueConstraintViolation {
                            constraint_name: index.name.clone(),
                            table_name: table.schema.table_name.clone(),
-                            col_name: table.schema.columns[index.col_id as usize].col_name.clone(),
-                            value: value.clone(),
+                            col_names: index
+                                .cols
+                                .iter()
+                                .map(|&x| insert_table.schema.columns[x as usize].col_name.clone())
+                                .collect(),
+                            value,
                        }
                        .into());


Centril · 2023-09-11T21:55:33Z

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

+    pub(crate) cols: NonEmpty<u32>,
    pub(crate) name: String,
    pub(crate) is_unique: bool,
    idx: BTreeSet<IndexKey>,


Can this handle the difference between a field with a product value or several fields as a product value? Otherwise we could do something like enum OneOrMore<T> { One(T), More(Vec<T>) } for a tighter representation

cloutiertyler

This generally LGTM.

mamcx requested a review from kulakowski August 25, 2023 21:07

mamcx changed the title ~~Mamcx/multi column index~~ Adding multi-column indexes to the DB Aug 25, 2023

mamcx self-assigned this Aug 25, 2023

mamcx marked this pull request as ready for review August 25, 2023 21:09

mamcx force-pushed the mamcx/multi-column-index branch from 9ba0b95 to 9282989 Compare August 30, 2023 14:09

mamcx mentioned this pull request Sep 6, 2023

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

Closed

9 tasks

cloutiertyler reviewed Sep 8, 2023

View reviewed changes

mamcx force-pushed the mamcx/multi-column-index branch from e10d191 to 410a125 Compare September 11, 2023 20:55

mamcx mentioned this pull request Sep 11, 2023

Benches refactor #131

Closed

3 tasks

Centril reviewed Sep 11, 2023

View reviewed changes

mamcx force-pushed the mamcx/multi-column-index branch from 410a125 to 5865a29 Compare September 12, 2023 16:18

mamcx added 6 commits September 14, 2023 13:21

Initial draft for multi-column indexes

2865d7e

Fix index seek for multi-columns

d698e2a

Restore use of ColId

f4a794a

Fix compilation

ec66917

Merge

891465d

Addressing some PR comments

4dc3597

mamcx force-pushed the mamcx/multi-column-index branch 2 times, most recently from ee4ca12 to 8be67fd Compare September 14, 2023 18:49

Clippy

c2e7a71

mamcx force-pushed the mamcx/multi-column-index branch from 8be67fd to c2e7a71 Compare September 14, 2023 18:51

Clarify usage of AlgebraicValue for indexes

1f0debb

cloutiertyler approved these changes Sep 25, 2023

View reviewed changes

mamcx added 2 commits September 25, 2023 11:13

Merge branch 'master' into mamcx/multi-column-index

e5a99d2

.

2803f5b

mamcx enabled auto-merge (squash) September 25, 2023 16:16

mamcx merged commit 9aecc25 into master Sep 25, 2023

mamcx mentioned this pull request Oct 17, 2023

Refactoring IDs for system objects #439

Merged

8 tasks

mamcx mentioned this pull request Nov 22, 2023

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

Merged

9 tasks

joshua-spacetime mentioned this pull request Apr 10, 2024

Multi-column indexes (and constraints) #1068

Closed

	pub(crate) cols: NonEmpty<u32>,
	pub(crate) cols: NonEmpty<ColId>,

	pub(crate) fn new(index_id: IndexId, table_id: u32, cols: NonEmpty<u32>, name: String, is_unique: bool) -> Self {
	pub(crate) fn new(index_id: IndexId, table_id: u32, cols: NonEmpty<ColId>, name: String, is_unique: bool) -> Self {

Adding multi-column indexes to the DB #243

Adding multi-column indexes to the DB #243

Uh oh!

Conversation

mamcx commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Changes

Uh oh!

cloutiertyler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mamcx Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cloutiertyler commented Sep 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloutiertyler left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mamcx commented Aug 25, 2023 •

edited

Loading

mamcx Sep 11, 2023 •

edited

Loading