Skip to content

Commit

Permalink
sql: make tabledesc.Immutable private
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Marius Posta committed Jan 29, 2021
1 parent 367f206 commit da18718
Show file tree
Hide file tree
Showing 10 changed files with 48 additions and 48 deletions.
2 changes: 1 addition & 1 deletion pkg/ccl/backupccl/targets_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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())},
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/catalog/hydratedtables/hydratedcache.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 8 additions & 8 deletions pkg/sql/catalog/tabledesc/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,37 +18,37 @@ 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)
}

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()
}

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
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/sql/catalog/tabledesc/safe_format.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/catalog/tabledesc/safe_format_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, ` +
Expand Down Expand Up @@ -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, " +
Expand Down
40 changes: 20 additions & 20 deletions pkg/sql/catalog/tabledesc/structured.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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 {
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}

Expand All @@ -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() {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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):]
}

Expand Down
16 changes: 8 additions & 8 deletions pkg/sql/catalog/tabledesc/table_desc.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand All @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/colfetcher/colbatch_scan.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit da18718

Please sign in to comment.