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.

Sign up for GitHub

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

Jump to bottom

Rip useless names out of RawDef #1918

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Rip useless names out of RawDef #1918

wants to merge 3 commits into from

Conversation

kazimuth
Copy link
Contributor

@kazimuth kazimuth commented Oct 28, 2024 •
edited
Loading

Description of Changes

Addresses #1893
Does NOT address #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.

@kazimuth kazimuth added the abi-break A PR that makes an ABI breaking change label Oct 28, 2024
RReverser
RReverser approved these changes Oct 30, 2024
View reviewed changes
Copy link
Member

@RReverser RReverser left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM. My only comments would be what's already covered by your TODOs.

@mamcx
Copy link
Contributor

mamcx commented Oct 30, 2024

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
Copy link
Contributor Author

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.

@RReverser RReverser mentioned this pull request Nov 5, 2024
Open
@kazimuth
Copy link
Contributor Author

kazimuth commented Nov 5, 2024

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

@kazimuth
Copy link
Contributor Author

I think the remaining test failure here could be related to #1987 so I'm going to work on that first.

@kazimuth kazimuth force-pushed the jgilles/unname branch 2 times, most recently from ec275ef to 0d8145a Compare November 18, 2024 19:42
@kazimuth
Rip useless names out of RawDef
ad9593b 
Fix CLI jankily

Remove dead comment

Implement name generation policy change (private #915 WIP)

Update bindings and cli to handle new name format

Clippy my nemesis

Finish updating csharp module bindings

And fix names

WIP

Placate csharpier

Final fixes

Fix schema doctests

Ah, C# is seeing the autogenerated names despite being on v8 still

Add test

WIP

Previous c# fix could not work due to v8. This one can work until the upgrade

Format
@kazimuth
Copy link
Contributor Author

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

kazimuth
kazimuth commented Nov 18, 2024
View reviewed changes
crates/core/src/db/datastore/locking_tx_datastore/datastore.rs
Comment on lines +1453 to +1462
ConstraintRow { constraint_id: 2, table_id: ST_TABLE_ID.into(), unique_columns: col(1), constraint_name: "constraint.unique(st_table,[table_name])", },
ConstraintRow { constraint_id: 3, table_id: ST_COLUMN_ID.into(), unique_columns: col_list![0, 1], constraint_name: "constraint.unique(st_column,[table_id,col_pos])", },
ConstraintRow { constraint_id: 4, table_id: ST_SEQUENCE_ID.into(), unique_columns: col(0), constraint_name: "constraint.unique(st_sequence,[sequence_id])", },
ConstraintRow { constraint_id: 5, table_id: ST_INDEX_ID.into(), unique_columns: col(0), constraint_name: "constraint.unique(st_index,[index_id])", },
ConstraintRow { constraint_id: 6, table_id: ST_CONSTRAINT_ID.into(), unique_columns: col(0), constraint_name: "constraint.unique(st_constraint,[constraint_id])", },
ConstraintRow { constraint_id: 7, table_id: ST_CLIENT_ID.into(), unique_columns: col_list![0, 1], constraint_name: "constraint.unique(st_client,[identity,address])", },
ConstraintRow { constraint_id: 8, table_id: ST_VAR_ID.into(), unique_columns: col(0), constraint_name: "constraint.unique(st_var,[name])", },
ConstraintRow { constraint_id: 9, table_id: ST_SCHEDULED_ID.into(), unique_columns: col(0), constraint_name: "constraint.unique(st_scheduled,[schedule_id])", },
ConstraintRow { constraint_id: 10, table_id: ST_SCHEDULED_ID.into(), unique_columns: col(1), constraint_name: "constraint.unique(st_scheduled,[table_id])", },
ConstraintRow { constraint_id: 11, table_id: ST_ROW_LEVEL_SECURITY_ID.into(), unique_columns: col(1), constraint_name: "constraint.unique(st_row_level_security,[sql])", },
Copy link
Contributor Author

@kazimuth kazimuth Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Here are examples of the new auto-generated default names in action. (Scroll right.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the Postgres standard naming scheme: https://stackoverflow.com/a/4108266/1445441

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is sensitive to name collisions but it's easy to adopt.

cloutiertyler
cloutiertyler requested changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please see comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cloutiertyler cloutiertyler cloutiertyler requested changes

@coolreader18 coolreader18 coolreader18 approved these changes

@RReverser RReverser RReverser approved these changes

@mamcx mamcx Awaiting requested review from mamcx

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
abi-break A PR that makes an ABI breaking change release-1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kazimuth @mamcx @RReverser @cloutiertyler @coolreader18 @bfops