[WIP] catalog,tabledesc: add & adopt Constraint subtypes #91997
Conversation
|
This change is |
postamar
changed the title
postamar
force-pushed
the
constraint-subtypes
branch
2 times, most recently
from
308989d to
36f6979
Compare
These new interfaces help encapsulate the descriptor protobufs for table constraints. Informs cockroachdb#91918. Release note: None
postamar
force-pushed
the
constraint-subtypes
branch
from
36f6979 to
2d570e2
Compare
| // AsPrimaryKey returns the index descriptor backing the PRIMARY KEY | ||
| // constraint, if there is one. | ||
| AsPrimaryKey() Index |
There was a problem hiding this comment.
why do we want to delete this method? How are we going to represent a PRIMARY KEY constraint?
There was a problem hiding this comment.
nvm, I see you are going to use the AsUnique() method below for both UNIQUE and PRIMARY KEY
There was a problem hiding this comment.
Thanks for pointing that out. It's confusing and instead I'm just going to introduce a UniqueWithIndexConstraint subtype that includes PKs and Uniques.
| // AsConstraint returns the corresponding Constraint if the mutation | ||
| // is on a constraint, nil otherwise. | ||
| AsConstraint() Constraint | ||
| // AsConstraintWithoutIndex returns the corresponding WithoutIndexConstraint | ||
| // if the mutation is on a constraint not backed by an index, nil otherwise. | ||
| AsConstraintWithoutIndex() WithoutIndexConstraint |
There was a problem hiding this comment.
Good, The change in name is really needed! (I did similar things myself when hacking, and I call them NonIndexBackedConstraint)
| // UniqueWithIndexConstraint is an interface around a unique constraint | ||
| // which backed by an index. |
There was a problem hiding this comment.
nit: should the comment say "... around a UNIQUE or PRIMARY KEY constraint ..."?
|
|
||
| // GetReferencedColumnID returns the ID of the column referenced in the check | ||
| // expression at ordinal `columnOrdinal`. | ||
| GetReferencedColumnID(columnOrdinal int) descpb.ColumnID |
There was a problem hiding this comment.
I see this pattern of having NumReferencedColumns and GetReferencedColumnID methods in both Check and FK constraint interface. Do you expect users to use these two methods together to retrieve all referenced columns?
Why don't we add a method, say called GetColumnIDs() in catalog.Constraint interface? It will return
- check: columns in the check expression
- FK: columns in the original table (say the FK is from A --> B, then that method returns columns in A)
- UniqueWithoutIndex: column(s) that are unique
- PK: the primary index's key columns
- UNIQUE: this unique index's key columns
There was a problem hiding this comment.
We don't like to return slices because the ownership of the slice is not clear and it's easy to mess things up terribly by append-ing something. Most of the time we just want to loop through the slice contents or we want to check set properties, hence a long time ago we settled on this NumColID/GetColID/CollectColIDs pattern.
| // GetReferencedTableID returns the ID of the table referenced by this | ||
| // foreign key. | ||
| GetReferencedTableID() descpb.ID |
There was a problem hiding this comment.
We probably will also need a method that retrieves referenced columns in the referenced table. Or, do you want the user to grab the underlying protobuf (via ForeignKeyDesc()) and retrieve it from there?
There was a problem hiding this comment.
The idea is to fall back to the descpb-protobuf only as a last resort. I'm adding these.
| type checkConstraint struct { | ||
| constraintBase | ||
| desc *descpb.TableDescriptor_CheckConstraint | ||
| notNullColumnID descpb.ColumnID |
There was a problem hiding this comment.
I always have this confusion here: If we are dealing with a NOT NULL constraint, which will be represented as a Check constraint in our code, will it be that desc.ColumnIDs will be exactly length 1 and that only column ID is the not null column ID?
Here, clearly, notNullColumnID (singular form) seems to imply in our code each NOT NULL constraint is only on one column. If that true?
There was a problem hiding this comment.
Maybe we can remove this notNullColumnID field altogether bc desc seems to contain all the information as well.
There was a problem hiding this comment.
You're correct & I hadn't realized this yet when I pushed this branch last week.
| if c.notNullColumnID != 0 { | ||
| return 1 | ||
| } | ||
| return len(c.desc.ColumnIDs) |
There was a problem hiding this comment.
I guess I asked the question above: if c.notNullColumnID != 0, do we assure len(c.desc.ColumnIDs) is also 1? If not, that will be non-sensical.
There was a problem hiding this comment.
All these are very good changes! Thanks for doing that. I left a few questions and comments.
I know what future modification I need to do (from my hacking experience) to unblock myself after this is merged, so let's iterate on this for a few more rounds quickly and get it merged 🥳 !
One more question: I didn't see any optimization made to the code where we populate the constraintCache. Is that work in progress, or do you plan to do it in a different pr?
Indeed it's going to be in a different PR, which I'll be submitting in a few minutes. When I opened this work-in-progress PR I was hoping to be able to refactor things incrementally, but it turns out that wasn't straightforward, so I just left this be. |
|
Closed in favour of #92289 |
These new interfaces help encapsulate the descriptor protobufs for table
constraints.
Informs #91918.
Release note: None