clockworklabs / SpacetimeDB

Multiplayer at the speed of light
https://spacetimedb.com
Other
4.41k stars 110 forks source link

Rip useless names out of RawDef #1918

Closed kazimuth closed 1 day ago

kazimuth commented 3 weeks ago

Description of Changes

Addresses https://github.com/clockworklabs/SpacetimeDB/issues/1893 Does NOT address https://github.com/clockworklabs/SpacetimeDB/issues/1891 since the special names turned out to be used throughout the codebase and I think addressing that will require a separate big PR.

While we're reworking naming, we may want to revisit the naming scheme here. It has a problem. Consider the following declaration:

#[spacetimedb(table)]
#[derive(Clone, Debug)]
#[spacetimedb(index(name = "idx1", btree(columns = [a, b]))]
#[spacetimedb(index(name = "idx2", btree(columns = [a_b]))]
pub struct EdgeCase {
    pub a: u32,
    pub b: u32,
    pub a_b: u64,
}

These indexes will BOTH receive the generated name "idx_EdgeCase_btree_a_b". Weird errors result.

(idx1 and idx2 are now referred to as accessor_names and are stored separately from the name of the index itself.)

It would be better if we used out-of-band characters in the index name, like, say, "EdgeCase.index.btree([a,b])". However, if we do this, we will need to relax the Identifier validation rules for indexes, since normally those will reject special characters. Also, it means that these identifiers can no longer be used directly as identifiers in SQL. @mamcx, I would welcome your opinion on this. I think it's fine, since system table queries can just wrap names in quotes.

We could also use a more elaborate mangling scheme, but this may penalize readability of the system tables.

API and ABI breaking changes

ABI breaking.

Expected complexity level and risk

Reduces complexity overall so let's say 2->1.

Testing

Fixed all relevant tests.

mamcx commented 3 weeks ago

I don't understand why it will generate "idx_EdgeCase_btree_a_b". I expect it to be "idx1_EdgeCase_btree_a_b"/"idx2_EdgeCase_btree_a_b"

kazimuth commented 3 weeks ago

Ah, that's because the name in the macro is now called accessor_name and is separate from the name of the index itself, that name is intended to only be used in codegen.

kazimuth commented 2 weeks ago

See also https://github.com/clockworklabs/SpacetimeDBPrivate/pull/915

kazimuth commented 6 days ago

I think the remaining test failure here could be related to https://github.com/clockworklabs/SpacetimeDB/pull/1987 so I'm going to work on that first.

kazimuth commented 3 days ago

The internal test failures here seem unrelated to what I'm doing, it's a rocksdb compilation failure :/

kazimuth commented 1 day ago

@cloutiertyler fixed, now we're using the postgres convention everywhere