sql: use catalog.TableDescriptor instead of tabledesc.Immutable #59495
Conversation
|
This change is |
postamar
requested review from
adityamaru and
a team
and removed request for
a team
There was a problem hiding this comment.
opt_catalog and opt_exec_factory changes LGTM
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @adityamaru)
|
@ajwerner @lucy-zhang I realize that with this change, |
There was a problem hiding this comment.
LGTM, just a few minor comments.
Reviewed 1 of 1 files at r1, 117 of 117 files at r3, 2 of 2 files at r4, 2 of 2 files at r5, 6 of 6 files at r6, 3 of 3 files at r7, 5 of 5 files at r8, 119 of 119 files at r9, 10 of 10 files at r10.
Reviewable status:
pkg/sql/drop_sequence.go, line 180 at r5 (raw file):
affectsNoColumns := true canBeSafelyRemoved := false
I think these could benefit from some documentation comments on what they mean. Using the iterator makes the logic less obvious.
pkg/sql/catalog/descriptor.go, line 248 at r8 (raw file):
HasColumnBackfillMutation() bool MakeFirstMutationPublic(includeConstraints bool) (MutableDescriptor, error)
I suspect we should be able to convert all the callers of this function to just use catalog.TableDescriptor. I don't think constraint validation needs to mutate these descriptors. #58906 should also help.
There was a problem hiding this comment.
Thanks for the review!
Reviewable status:
pkg/sql/drop_sequence.go, line 180 at r5 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
affectsNoColumns := true canBeSafelyRemoved := falseI think these could benefit from some documentation comments on what they mean. Using the iterator makes the logic less obvious.
Done.
pkg/sql/catalog/descriptor.go, line 248 at r8 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
I suspect we should be able to convert all the callers of this function to just use
catalog.TableDescriptor. I don't think constraint validation needs to mutate these descriptors. #58906 should also help.
Done.
|
Rebased on master to resolve conflict, and edited the commit message for the |
Previously these methods were defined only on the concrete types in the tabledesc package as well as in descpb.TableDescriptor. This patch adds these to catalog.TableDescriptor. These specific methods are called in places where we'd rather be calling them on the interface type instead of one of these concrete types. This patch is a prerequisite to that change. Release note: None
|
Rebased again to fix a few more conflicts. |
|
bors r+ |
|
Build failed (retrying...): |
This patch is a refactor which replaces field accesses on tabledesc.Immutable types with catalog.TableDescriptor method calls. This is a prerequisite to using the interface type instead. Field reads are replaced with corresponding getter method calls and field writes are done on the TableDesc() method return value. Release note: None
Previously this method only had one call site. This patch moves its logic next to that call site and removes the method. There are enough methods already. Release note: None
Previously, there was only one call site for the GetDependedOnBy method on a tabledesc.Immutable receiver. This patch replaces it with an equivalent call to ForeachDependedOnBy with a suitable closure. This allows us to avoid declaing the method on the catalog.TableDescriptor interface. Release note: None
This patch replaces this method with recently-introduced methods for catalog.Index types. The motivation for this refactor is to avoid bringing this deprecated method into the catalog.TableDescriptor interface. Release note: None
Previously, the test helper functions in the tabledesc package were defined as methods on Immutable receivers. This refactor transforms them into functions on catalog.TableDescriptor interfaces. This is a prerequisite to making the Immutable concrete type private. Release note: None
Previously, this method returned a tabledesc.Mutable type and this made it impossible to add it to the catalog.TableDescriptor interface. This patch does it by changing it to a catalog.TableDescriptor. This is a prerequisite to using the catalog.TableDescriptor interface type instead of the tabledesc.Immutable concrete type wherever possible. Release note: None
Previously we tended to use the concrete tabledesc.Immutable type rather than the catalog.TableDescriptor type. This was mainly for historical reasons because the former predates the latter. This patch builds on my previous commits by switching to the interface type everywhere outside of the tabledesc package. This required making the MakeImmutable factory function in tabledesc private, and changing the signatures of the NewImmutable factory functions and using them instead of MakeImmutable. Fixes cockroachdb#59486. Release note: None
Previously, this concrete type implementing catalog.TableDescriptor was exported and, until my previous commit, could be found throughout the codebase. This patch makes it private to ensure that the interface type continues to be used instead. Release note: None
|
Build failed: |
|
Sorry for the noise, just silly build failures due to having had to rebase. |
|
bors r+ |
|
Build succeeded: |
This PR makes the tabledesc.Immutable type private and makes use of the
catalog.TableDescriptor interface instead.
Fixes #59486.
Release note: None