From da18718af233f45e4cfd1cc1c206afe62eef36d0 Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Wed, 27 Jan 2021 16:23:49 -0500 Subject: [PATCH] sql: make tabledesc.Immutable private 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 --- pkg/ccl/backupccl/targets_test.go | 2 +- .../schematestutils/schema_test_utils.go | 4 +- .../catalog/hydratedtables/hydratedcache.go | 2 +- pkg/sql/catalog/tabledesc/helpers_test.go | 16 ++++---- pkg/sql/catalog/tabledesc/safe_format.go | 6 +-- pkg/sql/catalog/tabledesc/safe_format_test.go | 4 +- pkg/sql/catalog/tabledesc/structured.go | 40 +++++++++---------- pkg/sql/catalog/tabledesc/table_desc.go | 16 ++++---- pkg/sql/check.go | 2 +- pkg/sql/colfetcher/colbatch_scan.go | 4 +- 10 files changed, 48 insertions(+), 48 deletions(-) diff --git a/pkg/ccl/backupccl/targets_test.go b/pkg/ccl/backupccl/targets_test.go index e9f8d8f0495d..b5012d366bbe 100644 --- a/pkg/ccl/backupccl/targets_test.go +++ b/pkg/ccl/backupccl/targets_test.go @@ -37,7 +37,7 @@ func TestDescriptorsMatchingTargets(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - // TODO(ajwerner): There should be a constructor for an Immutable + // TODO(ajwerner): There should be a constructor for an immutable // and really all of the leasable descriptor types which includes its initial // DescriptorMeta. This refactoring precedes the actual adoption of // DescriptorMeta. diff --git a/pkg/ccl/changefeedccl/schemafeed/schematestutils/schema_test_utils.go b/pkg/ccl/changefeedccl/schemafeed/schematestutils/schema_test_utils.go index 3e6cbe0e3100..8f91952fe4ad 100644 --- a/pkg/ccl/changefeedccl/schemafeed/schematestutils/schema_test_utils.go +++ b/pkg/ccl/changefeedccl/schemafeed/schematestutils/schema_test_utils.go @@ -50,7 +50,7 @@ func MakeColumnDesc(id descpb.ColumnID) *descpb.ColumnDescriptor { } // AddColumnDropBackfillMutation adds a mutation to desc to drop a column. -// Yes, this does modify an Immutable. +// Yes, this does modify an immutable. func AddColumnDropBackfillMutation(desc catalog.TableDescriptor) catalog.TableDescriptor { desc.TableDesc().Mutations = append(desc.TableDesc().Mutations, descpb.DescriptorMutation{ State: descpb.DescriptorMutation_DELETE_AND_WRITE_ONLY, @@ -61,7 +61,7 @@ func AddColumnDropBackfillMutation(desc catalog.TableDescriptor) catalog.TableDe } // AddNewColumnBackfillMutation adds a mutation to desc to add a column. -// Yes, this does modify an Immutable. +// Yes, this does modify an immutable. func AddNewColumnBackfillMutation(desc catalog.TableDescriptor) catalog.TableDescriptor { desc.TableDesc().Mutations = append(desc.TableDesc().Mutations, descpb.DescriptorMutation{ Descriptor_: &descpb.DescriptorMutation_Column{Column: MakeColumnDesc(desc.GetNextColumnID())}, diff --git a/pkg/sql/catalog/hydratedtables/hydratedcache.go b/pkg/sql/catalog/hydratedtables/hydratedcache.go index bdea66bf3556..036d7928d43d 100644 --- a/pkg/sql/catalog/hydratedtables/hydratedcache.go +++ b/pkg/sql/catalog/hydratedtables/hydratedcache.go @@ -45,7 +45,7 @@ import ( // of the referenced types which ensures that user always uses properly leased // descriptors. While all of the types will need to be resolved, they should // already be cached so, in this way, this cache prevents the need to copy -// and re-construct the tabledesc.Immutable in most cases. +// and re-construct the tabledesc.immutable in most cases. type Cache struct { settings *cluster.Settings g singleflight.Group diff --git a/pkg/sql/catalog/tabledesc/helpers_test.go b/pkg/sql/catalog/tabledesc/helpers_test.go index 67f18b1e922e..024f573dfcef 100644 --- a/pkg/sql/catalog/tabledesc/helpers_test.go +++ b/pkg/sql/catalog/tabledesc/helpers_test.go @@ -18,9 +18,9 @@ import ( ) func ValidateTable(ctx context.Context, immI catalog.TableDescriptor) error { - imm, ok := immI.(*Immutable) + imm, ok := immI.(*immutable) if !ok { - return errors.Errorf("expected Immutable descriptor") + return errors.Errorf("expected immutable descriptor") } return imm.ValidateTable(ctx) } @@ -28,17 +28,17 @@ func ValidateTable(ctx context.Context, immI catalog.TableDescriptor) error { func ValidateCrossReferences( ctx context.Context, dg catalog.DescGetter, immI catalog.TableDescriptor, ) error { - imm, ok := immI.(*Immutable) + imm, ok := immI.(*immutable) if !ok { - return errors.Errorf("expected Immutable descriptor") + return errors.Errorf("expected immutable descriptor") } return imm.validateCrossReferences(ctx, dg) } func ValidatePartitioning(immI catalog.TableDescriptor) error { - imm, ok := immI.(*Immutable) + imm, ok := immI.(*immutable) if !ok { - return errors.Errorf("expected Immutable descriptor") + return errors.Errorf("expected immutable descriptor") } return imm.validatePartitioning() } @@ -46,9 +46,9 @@ func ValidatePartitioning(immI catalog.TableDescriptor) error { func GetPostDeserializationChanges( immI catalog.TableDescriptor, ) (PostDeserializationTableDescriptorChanges, error) { - imm, ok := immI.(*Immutable) + imm, ok := immI.(*immutable) if !ok { - return PostDeserializationTableDescriptorChanges{}, errors.Errorf("expected Immutable descriptor") + return PostDeserializationTableDescriptorChanges{}, errors.Errorf("expected immutable descriptor") } return imm.GetPostDeserializationChanges(), nil } diff --git a/pkg/sql/catalog/tabledesc/safe_format.go b/pkg/sql/catalog/tabledesc/safe_format.go index b24f1193a0ed..bc97db69e7cd 100644 --- a/pkg/sql/catalog/tabledesc/safe_format.go +++ b/pkg/sql/catalog/tabledesc/safe_format.go @@ -16,9 +16,9 @@ import ( "github.com/cockroachdb/redact" ) -// SafeMessage makes Immutable a SafeMessager. -func (desc *Immutable) SafeMessage() string { - return formatSafeTableDesc("tabledesc.Immutable", desc) +// SafeMessage makes immutable a SafeMessager. +func (desc *immutable) SafeMessage() string { + return formatSafeTableDesc("tabledesc.immutable", desc) } // SafeMessage makes Mutable a SafeMessager. diff --git a/pkg/sql/catalog/tabledesc/safe_format_test.go b/pkg/sql/catalog/tabledesc/safe_format_test.go index cc5d7c38116d..4f841a072d3a 100644 --- a/pkg/sql/catalog/tabledesc/safe_format_test.go +++ b/pkg/sql/catalog/tabledesc/safe_format_test.go @@ -59,7 +59,7 @@ func TestSafeMessage(t *testing.T) { id: 12, parentID: 21, schema: "CREATE TABLE foo (i INT PRIMARY KEY, j INT, j_str STRING AS (j::STRING) STORED, INDEX (j_str))", - exp: `tabledesc.Immutable: {` + + exp: `tabledesc.immutable: {` + `ID: 12, Version: 1, ModificationTime: "1.000000000,0", ` + `ParentID: 21, ParentSchemaID: 29, State: PUBLIC, ` + `NextColumnID: 6, ` + @@ -236,7 +236,7 @@ func TestSafeMessage(t *testing.T) { id: 12, parentID: 21, schema: "CREATE TABLE foo ()", - exp: "tabledesc.Immutable: {" + + exp: "tabledesc.immutable: {" + "ID: 12, Version: 1, " + "ModificationTime: \"0,0\", " + "ParentID: 21, ParentSchemaID: 29, " + diff --git a/pkg/sql/catalog/tabledesc/structured.go b/pkg/sql/catalog/tabledesc/structured.go index 27649b861a23..37daaff8b284 100644 --- a/pkg/sql/catalog/tabledesc/structured.go +++ b/pkg/sql/catalog/tabledesc/structured.go @@ -107,13 +107,13 @@ func NewFilledInExistingMutable( return &Mutable{wrapper: w, ClusterVersion: *tbl}, nil } -// makeImmutable returns an Immutable from the given TableDescriptor. -func makeImmutable(tbl descpb.TableDescriptor) Immutable { +// makeImmutable returns an immutable from the given TableDescriptor. +func makeImmutable(tbl descpb.TableDescriptor) immutable { publicAndNonPublicCols := tbl.Columns readableCols := tbl.Columns - desc := Immutable{wrapper: wrapper{TableDescriptor: tbl, indexCache: newIndexCache(&tbl)}} + desc := immutable{wrapper: wrapper{TableDescriptor: tbl, indexCache: newIndexCache(&tbl)}} if len(tbl.Mutations) > 0 { publicAndNonPublicCols = make([]descpb.ColumnDescriptor, 0, len(tbl.Columns)+len(tbl.Mutations)) @@ -171,17 +171,17 @@ func makeImmutable(tbl descpb.TableDescriptor) Immutable { return desc } -// NewImmutable returns a Immutable from the given TableDescriptor. +// NewImmutable returns a immutable from the given TableDescriptor. // This function assumes that this descriptor has not been modified from the // version stored in the key-value store. func NewImmutable(tbl descpb.TableDescriptor) catalog.TableDescriptor { return NewImmutableWithIsUncommittedVersion(tbl, false /* isUncommittedVersion */) } -// NewImmutableWithIsUncommittedVersion returns a Immutable from the given +// NewImmutableWithIsUncommittedVersion returns a immutable from the given // TableDescriptor and allows the caller to mark the table as corresponding to // an uncommitted version. This should be used when constructing a new copy of -// an Immutable from an existing descriptor which may have a new version. +// an immutable from an existing descriptor which may have a new version. func NewImmutableWithIsUncommittedVersion( tbl descpb.TableDescriptor, isUncommittedVersion bool, ) catalog.TableDescriptor { @@ -190,7 +190,7 @@ func NewImmutableWithIsUncommittedVersion( return &desc } -// NewFilledInImmutable will construct an Immutable and potentially perform +// NewFilledInImmutable will construct an immutable and potentially perform // post-deserialization upgrades. func NewFilledInImmutable( ctx context.Context, dg catalog.DescGetter, tbl *descpb.TableDescriptor, @@ -804,7 +804,7 @@ func ForEachExprStringInTableDesc(descI catalog.TableDescriptor, f func(expr *st switch descV := descI.(type) { case *wrapper: desc = descV - case *Immutable: + case *immutable: desc = &descV.wrapper case *Mutable: desc = &descV.wrapper @@ -1433,7 +1433,7 @@ func (desc *wrapper) validateCrossReferences(ctx context.Context, dg catalog.Des unupgradedFKsPresent := false if err := catalog.ForEachIndex(referencedTable, catalog.IndexOpts{}, func(referencedIdx catalog.Index) error { if found { - // TODO (lucy): If we ever revisit the tabledesc.Immutable methods, add + // TODO (lucy): If we ever revisit the tabledesc.immutable methods, add // a way to break out of the index loop. return nil } @@ -1512,7 +1512,7 @@ func (desc *wrapper) validateCrossReferences(ctx context.Context, dg catalog.Des unupgradedFKsPresent := false if err := catalog.ForEachIndex(originTable, catalog.IndexOpts{}, func(originIdx catalog.Index) error { if found { - // TODO (lucy): If we ever revisit the tabledesc.Immutable methods, add + // TODO (lucy): If we ever revisit the tabledesc.immutable methods, add // a way to break out of the index loop. return nil } @@ -2918,15 +2918,15 @@ func (desc *wrapper) ContainsUserDefinedTypes() bool { // ContainsUserDefinedTypes returns whether or not this table descriptor has // any columns of user defined types. -// This method is re-implemented for Immutable only for the purpose of calling +// This method is re-implemented for immutable only for the purpose of calling // the correct GetColumnOrdinalsWithUserDefinedTypes() method on desc. -func (desc *Immutable) ContainsUserDefinedTypes() bool { +func (desc *immutable) ContainsUserDefinedTypes() bool { return len(desc.GetColumnOrdinalsWithUserDefinedTypes()) > 0 } // GetColumnOrdinalsWithUserDefinedTypes returns a slice of column ordinals // of columns that contain user defined types. -func (desc *Immutable) GetColumnOrdinalsWithUserDefinedTypes() []int { +func (desc *immutable) GetColumnOrdinalsWithUserDefinedTypes() []int { return desc.columnsWithUDTs } @@ -2950,10 +2950,10 @@ func (desc *wrapper) UserDefinedTypeColsHaveSameVersion(otherDesc catalog.TableD // with user defined type metadata have the same versions of metadata as in the // other descriptor. Note that this function is only valid on two descriptors // representing the same table at the same version. -// This method is re-implemented for Immutable only for the purpose of calling +// This method is re-implemented for immutable only for the purpose of calling // the correct DeletableColumns() and GetColumnOrdinalsWithUserDefinedTypes() // methods on desc. -func (desc *Immutable) UserDefinedTypeColsHaveSameVersion(otherDesc catalog.TableDescriptor) bool { +func (desc *immutable) UserDefinedTypeColsHaveSameVersion(otherDesc catalog.TableDescriptor) bool { thisCols := desc.DeletableColumns() otherCols := otherDesc.DeletableColumns() for _, idx := range desc.GetColumnOrdinalsWithUserDefinedTypes() { @@ -3747,7 +3747,7 @@ const IgnoreConstraints = false const IncludeConstraints = true // MakeFirstMutationPublic creates a Mutable from the -// Immutable by making the first mutation public. +// immutable by making the first mutation public. // This is super valuable when trying to run SQL over data associated // with a schema mutation that is still not yet public: Data validation, // error reporting. @@ -4089,22 +4089,22 @@ func (desc *wrapper) FindAllReferences() (map[descpb.ID]struct{}, error) { // ActiveChecks returns a list of all check constraints that should be enforced // on writes (including constraints being added/validated). The columns // referenced by the returned checks are writable, but not necessarily public. -func (desc *Immutable) ActiveChecks() []descpb.TableDescriptor_CheckConstraint { +func (desc *immutable) ActiveChecks() []descpb.TableDescriptor_CheckConstraint { return desc.allChecks } // WritableColumns returns a list of public and write-only mutation columns. -func (desc *Immutable) WritableColumns() []descpb.ColumnDescriptor { +func (desc *immutable) WritableColumns() []descpb.ColumnDescriptor { return desc.publicAndNonPublicCols[:len(desc.Columns)+desc.writeOnlyColCount] } // DeletableColumns returns a list of public and non-public columns. -func (desc *Immutable) DeletableColumns() []descpb.ColumnDescriptor { +func (desc *immutable) DeletableColumns() []descpb.ColumnDescriptor { return desc.publicAndNonPublicCols } // MutationColumns returns a list of mutation columns. -func (desc *Immutable) MutationColumns() []descpb.ColumnDescriptor { +func (desc *immutable) MutationColumns() []descpb.ColumnDescriptor { return desc.publicAndNonPublicCols[len(desc.Columns):] } diff --git a/pkg/sql/catalog/tabledesc/table_desc.go b/pkg/sql/catalog/tabledesc/table_desc.go index 08d2b39316c8..79311a3c7f8f 100644 --- a/pkg/sql/catalog/tabledesc/table_desc.go +++ b/pkg/sql/catalog/tabledesc/table_desc.go @@ -18,19 +18,19 @@ import ( "github.com/cockroachdb/errors" ) -var _ catalog.TableDescriptor = (*Immutable)(nil) +var _ catalog.TableDescriptor = (*immutable)(nil) var _ catalog.TableDescriptor = (*Mutable)(nil) var _ catalog.MutableDescriptor = (*Mutable)(nil) var _ catalog.TableDescriptor = (*wrapper)(nil) // wrapper is the base implementation of the catalog.Descriptor -// interface, which is overloaded by Immutable and Mutable. +// interface, which is overloaded by immutable and Mutable. type wrapper struct { descpb.TableDescriptor // indexCache, when not nil, points to a struct containing precomputed // catalog.Index slices. This can therefore only be set when creating an - // Immutable. + // immutable. indexCache *indexCache postDeserializationChanges PostDeserializationTableDescriptorChanges @@ -132,10 +132,10 @@ func (desc *wrapper) GetColumnOrdinalsWithUserDefinedTypes() []int { return ords } -// Immutable is a custom type for TableDescriptors +// immutable is a custom type for TableDescriptors // It holds precomputed values and the underlying TableDescriptor // should be const. -type Immutable struct { +type immutable struct { wrapper // publicAndNonPublicCols is a list of public and non-public columns. @@ -166,7 +166,7 @@ type Immutable struct { } // IsUncommittedVersion implements the Descriptor interface. -func (desc *Immutable) IsUncommittedVersion() bool { +func (desc *immutable) IsUncommittedVersion() bool { return desc.isUncommittedVersion } @@ -214,7 +214,7 @@ func (desc *wrapper) ReadableColumns() []descpb.ColumnDescriptor { // ReadableColumns returns a list of columns (including those undergoing a // schema change) which can be scanned. -func (desc *Immutable) ReadableColumns() []descpb.ColumnDescriptor { +func (desc *immutable) ReadableColumns() []descpb.ColumnDescriptor { return desc.readableColumns } @@ -223,7 +223,7 @@ func (desc *Mutable) ImmutableCopy() catalog.Descriptor { // TODO (lucy): Should the immutable descriptor constructors always make a // copy, so we don't have to do it here? imm := NewImmutable(*protoutil.Clone(desc.TableDesc()).(*descpb.TableDescriptor)) - imm.(*Immutable).isUncommittedVersion = desc.IsUncommittedVersion() + imm.(*immutable).isUncommittedVersion = desc.IsUncommittedVersion() return imm } diff --git a/pkg/sql/check.go b/pkg/sql/check.go index a2541655aa78..516ae10dad29 100644 --- a/pkg/sql/check.go +++ b/pkg/sql/check.go @@ -317,7 +317,7 @@ func formatValues(colNames []string, values tree.Datums) string { } // checkSet contains a subset of checks, as ordinals into -// Immutable.ActiveChecks. These checks have boolean columns +// immutable.ActiveChecks. These checks have boolean columns // produced as input to mutations, indicating the result of evaluating the // check. // diff --git a/pkg/sql/colfetcher/colbatch_scan.go b/pkg/sql/colfetcher/colbatch_scan.go index 72597adeaf73..282c03da4f81 100644 --- a/pkg/sql/colfetcher/colbatch_scan.go +++ b/pkg/sql/colfetcher/colbatch_scan.go @@ -170,10 +170,10 @@ func NewColBatchScan( limitHint := execinfra.LimitHint(spec.LimitHint, post) returnMutations := spec.Visibility == execinfra.ScanVisibilityPublicAndNotPublic - // TODO(ajwerner): The need to construct an Immutable here + // TODO(ajwerner): The need to construct an immutable here // indicates that we're probably doing this wrong. Instead we should be // just setting the ID and Version in the spec or something like that and - // retrieving the hydrated Immutable from cache. + // retrieving the hydrated immutable from cache. table := tabledesc.NewImmutable(spec.Table) typs := table.ColumnTypesWithMutationsAndVirtualCol(returnMutations, spec.VirtualColumn) columnIdxMap := table.ColumnIdxMapWithMutations(returnMutations)