fix(frontend): require primary key for system table #15126
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
BugenZhao
force-pushed
the
bz/pg-settings-tab-completion
branch
from
51a0edd to
28364c9
Compare
BugenZhao
force-pushed
the
bz/require-primary-key-for-system-table
branch
from
32063cc to
70a31db
Compare
| @@ -19,6 +19,7 @@ use crate::catalog::system_catalog::SysCatalogReaderImpl; | |||
| use crate::error::Result; | |||
|
|
|||
| #[derive(Fields)] | |||
| #[primary_key(object_id, sst_id)] // TODO: is this correct? | |||
BugenZhao
requested review from
chenzl25,
wangrunji0408,
st1page,
zwang28 and
yezizp2012
| @@ -117,7 +117,9 @@ fn get_primary_key(input: &syn::DeriveInput) -> Option<Vec<usize>> { | |||
| return Some( | |||
| keys.to_string() | |||
| .split(',') | |||
| .map(|s| index(s.trim())) | |||
| .map(|s| s.trim()) | |||
| .filter(|s| !s.is_empty()) | |||
There was a problem hiding this comment.
So that we can attach #[primary_key()] on the struct to specify empty primary keys.
Signed-off-by: Bugen Zhao <[email protected]>
BugenZhao
force-pushed
the
bz/require-primary-key-for-system-table
branch
from
70a31db to
320f737
Compare
|
So does all systable have one primary key? If not, maybe we do not need this
And if we can add an hidden column for those systable without pk? |
What do you mean by "one"? At least after this PR, every system table will have non-empty primary key.
Need what? Can you elaborate more? |
Yeah, this could be another feasible approach. |
Sorry, I mean I prefer to make all the table must have a primary key instead of make the field optional. |
Signed-off-by: Bugen Zhao <[email protected]>
Merge queue setting changed
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
See #15125 for the background.
We should not default to empty primary key for
#[derive(Fields)]without specifying primary keys, or the semantics will be "at most one row".The ultimate solution could be making the
pkfield ofTableDescandTableCatalogto be anOption. However, this may require a much more significant refactor. Note that we're only misusing the primary key of the system table, so I simply add a requirement for specifying the primary key forsystem_tableproc macro in this PR.Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.