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

feat(binder): support “primary key (a, b)” syntax #631

Merged
merged 1 commit into from
Apr 19, 2022
Merged

feat(binder): support “primary key (a, b)” syntax #631

merged 1 commit into from
Apr 19, 2022

Conversation

shmiwy
Copy link
Contributor

@shmiwy shmiwy commented Apr 19, 2022

Signed-off-by: Shmiwy [email protected]

#625 1st editon

support primary key (a, b) syntax by adding a field called primary_key_ids in TableCatalog without removng is_primary field from ColumnDesc. I will remove is_primary and refactor code when I want to extend the case to multiple sort keys.

Copy link
Member

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

LGTM, good work!

table_catalog.add_column(col_catalog).unwrap();
}

table_catalog.set_primary_key_ids(&pk_ids);
Copy link
Member

@skyzh skyzh Apr 19, 2022

Choose a reason for hiding this comment

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

It seems that this logic still cannot tell the difference between primary key (a, b) and primary key (b, a)? May fix this in later PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

primary_key_ids is a vector, maybe we can tell the difference by index?

@skyzh skyzh changed the title feat: support primary key (a, b) syntax. feat(binder): support primary key (a, b) syntax. Apr 19, 2022
@skyzh skyzh changed the title feat(binder): support primary key (a, b) syntax. feat(binder): support primary key (a, b) syntax Apr 19, 2022
@skyzh skyzh merged commit d28389a into risinglightdb:main Apr 19, 2022
@shmiwy
Copy link
Contributor Author

shmiwy commented Apr 19, 2022

@skyzh

primary_key_ids is vector, maybe we can tell the difference by index?

struct Inner {
name: String,
/// Mapping from column names to column ids
column_idxs: HashMap<String, ColumnId>,
columns: BTreeMap<ColumnId, ColumnCatalog>,
#[allow(dead_code)]
is_materialized_view: bool,
next_column_id: ColumnId,
primary_key_ids: Vec<ColumnId>,
}

@shmiwy shmiwy changed the title feat(binder): support primary key (a, b) syntax feat(binder): support “primary key (a, b)” syntax Apr 19, 2022
@skyzh
Copy link
Member

skyzh commented Apr 19, 2022

But when generating primary_key_ids in TableCatalog::new, we simply iterate from the first column to the end. So for both cases, it will produce the same order. It seems that the order in Unique constraint is not retained?

@shmiwy
Copy link
Contributor Author

shmiwy commented Apr 19, 2022

oh, I forget this...

@shmiwy shmiwy deleted the sort_key_feature branch April 19, 2022 03:27
@shmiwy shmiwy restored the sort_key_feature branch April 19, 2022 03:28
@shmiwy shmiwy deleted the sort_key_feature branch April 19, 2022 03:31
@skyzh
Copy link
Member

skyzh commented Apr 19, 2022

oh, I forget this...

May also add a test case for this in the future :)

@shmiwy shmiwy restored the sort_key_feature branch April 19, 2022 05:21
@shmiwy shmiwy deleted the sort_key_feature branch April 19, 2022 05:22
@shmiwy shmiwy restored the sort_key_feature branch April 19, 2022 05:22
@shmiwy shmiwy deleted the sort_key_feature branch April 19, 2022 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants