Skip to content

Commit

Permalink
sql: remove deprecated FindColumn-methods from TableDescriptor
Browse files Browse the repository at this point in the history 
Previously, this method would be called on a table descriptor interface to
return a target *descpb.ColumDescriptor.

This patch removes these calls, along with the method definitions, in
favour of new methods and functions which use the catalog.Column interface
type instead.

Fixes cockroachdb#59805.

Release note: None
  • Loading branch information
Marius Posta committed Feb 5, 2021
1 parent c5c0035 commit f3ffee8
Show file tree
Hide file tree
Showing 38 changed files with 290 additions and 406 deletions.
6 changes: 3 additions & 3 deletions pkg/ccl/importccl/import_stmt.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -718,13 +718,13 @@ func importPlanHook(
var intoCols []string
var isTargetCol = make(map[string]bool)
for _, name := range importStmt.IntoCols {
active, err := found.FindActiveColumnsByNames(tree.NameList{name})
active, err := tabledesc.FindPublicColumnsWithNames(found, tree.NameList{name})
if err != nil {
return errors.Wrap(err, "verifying target columns")
}

isTargetCol[active[0].Name] = true
intoCols = append(intoCols, active[0].Name)
isTargetCol[active[0].GetName()] = true
intoCols = append(intoCols, active[0].GetName())
}

// Ensure that non-target columns that don't have default
Expand Down
33 changes: 20 additions & 13 deletions pkg/ccl/partitionccl/partition.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,12 +183,16 @@ func createPartitioningImpl(
}
// Search by name because some callsites of this method have not
// allocated ids yet (so they are still all the 0 value).
col, err := tableDesc.FindActiveColumnByName(indexDesc.ColumnNames[colOffset+i])
name := indexDesc.ColumnNames[colOffset+i]
col, err := tableDesc.FindColumnWithName(tree.Name(name))
if err != nil {
return partDesc, err
}
cols = append(cols, *col)
if string(partBy.Fields[i]) != col.Name {
if !col.Public() {
return partDesc, colinfo.NewUndefinedColumnError(name)
}
cols = append(cols, *col.ColumnDesc())
if string(partBy.Fields[i]) != col.GetName() {
// This used to print the first `colOffset + len(partBy.Fields)` fields
// but there might not be this many columns in the index. See #37682.
n := colOffset + i + 1
Expand Down Expand Up @@ -276,20 +280,23 @@ func detectImplicitPartitionColumns(
break
}

col, err := tableDesc.FindActiveColumnByName(string(field))
col, err := tableDesc.FindColumnWithName(field)
if err != nil {
return indexDesc, 0, err
}
if _, ok := seenImplicitColumnNames[col.Name]; ok {
if !col.Public() {
return indexDesc, 0, colinfo.NewUndefinedColumnError(string(field))
}
if _, ok := seenImplicitColumnNames[col.GetName()]; ok {
return indexDesc, 0, pgerror.Newf(
pgcode.InvalidObjectDefinition,
`found multiple definitions in partition using column "%s"`,
col.Name,
col.GetName(),
)
}
seenImplicitColumnNames[col.Name] = struct{}{}
implicitColumns = append(implicitColumns, col.Name)
implicitColumnIDs = append(implicitColumnIDs, col.ID)
seenImplicitColumnNames[col.GetName()] = struct{}{}
implicitColumns = append(implicitColumns, col.GetName())
implicitColumnIDs = append(implicitColumnIDs, col.GetID())
implicitColumnDirections = append(implicitColumnDirections, descpb.IndexDescriptor_ASC)
}

Expand Down Expand Up @@ -378,11 +385,11 @@ func selectPartitionExprs(
// dummy IndexVars. Swap them out for actual column references.
finalExpr, err := tree.SimpleVisit(expr, func(e tree.Expr) (recurse bool, newExpr tree.Expr, _ error) {
if ivar, ok := e.(*tree.IndexedVar); ok {
col, err := tableDesc.FindColumnByID(descpb.ColumnID(ivar.Idx))
col, err := tableDesc.FindColumnWithID(descpb.ColumnID(ivar.Idx))
if err != nil {
return false, nil, err
}
return false, &tree.ColumnItem{ColumnName: tree.Name(col.Name)}, nil
return false, &tree.ColumnItem{ColumnName: tree.Name(col.GetName())}, nil
}
return true, e, nil
})
Expand Down Expand Up @@ -442,11 +449,11 @@ func selectPartitionExprsByName(
// the column ordinal references, so reconstruct them here.
colVars = make(tree.Exprs, len(prefixDatums)+int(partDesc.NumColumns))
for i := range colVars {
col, err := tableDesc.FindActiveColumnByID(idxDesc.ColumnIDs[i])
col, err := tabledesc.FindPublicColumnWithID(tableDesc, idxDesc.ColumnIDs[i])
if err != nil {
return err
}
colVars[i] = tree.NewTypedOrdinalReference(int(col.ID), col.Type)
colVars[i] = tree.NewTypedOrdinalReference(int(col.GetID()), col.GetType())
}
}

Expand Down
47 changes: 26 additions & 21 deletions pkg/sql/add_column.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
package sql

import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
Expand Down Expand Up @@ -104,29 +105,11 @@ func (p *planner) addColumnImpl(
return sqlerrors.NewNonNullViolationError(col.Name)
}
}
_, err = n.tableDesc.FindActiveColumnByName(string(d.Name))
if m := n.tableDesc.FindColumnMutationByName(d.Name); m != nil {
switch m.Direction {
case descpb.DescriptorMutation_ADD:
return pgerror.Newf(pgcode.DuplicateColumn,
"duplicate: column %q in the middle of being added, not yet public",
col.Name)
case descpb.DescriptorMutation_DROP:
return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
"column %q being dropped, try again later", col.Name)
default:
if err != nil {
return errors.AssertionFailedf(
"mutation in state %s, direction %s, and no column descriptor",
errors.Safe(m.State), errors.Safe(m.Direction))
}
}
}
if err == nil {
if t.IfNotExists {
if isPublic, err := checkColumnDoesNotExist(n.tableDesc, d.Name); err != nil {
if isPublic && t.IfNotExists {
return nil
}
return sqlerrors.NewColumnAlreadyExistsError(string(d.Name), n.tableDesc.Name)
return err
}

n.tableDesc.AddColumnMutation(col, descpb.DescriptorMutation_ADD)
Expand Down Expand Up @@ -163,3 +146,25 @@ func (p *planner) addColumnImpl(

return nil
}

func checkColumnDoesNotExist(
tableDesc catalog.TableDescriptor, name tree.Name,
) (isPublic bool, err error) {
col, _ := tableDesc.FindColumnWithName(name)
if col == nil {
return false, nil
}
if col.Public() {
return true, sqlerrors.NewColumnAlreadyExistsError(tree.ErrString(&name), tableDesc.GetName())
}
if col.Adding() {
return false, pgerror.Newf(pgcode.DuplicateColumn,
"duplicate: column %q in the middle of being added, not yet public",
col.GetName())
}
if col.Dropped() {
return false, pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
"column %q being dropped, try again later", col.GetName())
}
return false, errors.AssertionFailedf("mutation in direction NONE")
}
2 changes: 1 addition & 1 deletion pkg/sql/alter_column_type.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ func alterColumnTypeGeneral(
}

nameExists := func(name string) bool {
_, _, err := tableDesc.FindColumnByName(tree.Name(name))
_, err := tableDesc.FindColumnWithName(tree.Name(name))
return err == nil
}

Expand Down
14 changes: 7 additions & 7 deletions pkg/sql/alter_primary_key.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,16 +82,16 @@ func (p *planner) AlterPrimaryKey(
}

for _, elem := range alterPKNode.Columns {
col, dropped, err := tableDesc.FindColumnByName(elem.Column)
col, err := tableDesc.FindColumnWithName(elem.Column)
if err != nil {
return err
}
if dropped {
if col.Dropped() {
return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
"column %q is being dropped", col.Name)
"column %q is being dropped", col.GetName())
}
if col.Nullable {
return pgerror.Newf(pgcode.InvalidSchemaDefinition, "cannot use nullable column %q in primary key", col.Name)
if col.IsNullable() {
return pgerror.Newf(pgcode.InvalidSchemaDefinition, "cannot use nullable column %q in primary key", col.GetName())
}
}

Expand Down Expand Up @@ -317,11 +317,11 @@ func (p *planner) AlterPrimaryKey(
}
if idx.Unique {
for _, colID := range idx.ColumnIDs {
col, err := tableDesc.FindColumnByID(colID)
col, err := tableDesc.FindColumnWithID(colID)
if err != nil {
return false, err
}
if col.Nullable {
if col.IsNullable() {
return true, nil
}
}
Expand Down
52 changes: 26 additions & 26 deletions pkg/sql/alter_table.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ func (n *alterTableNode) startExec(params runParams) error {

// Check if the columns exist on the table.
for _, column := range d.Columns {
if _, _, err := n.tableDesc.FindColumnByName(column.Column); err != nil {
if _, err := n.tableDesc.FindColumnWithName(column.Column); err != nil {
return err
}
}
Expand Down Expand Up @@ -395,32 +395,32 @@ func (n *alterTableNode) startExec(params runParams) error {
return err
}

colToDrop, dropped, err := n.tableDesc.FindColumnByName(t.Column)
colToDrop, err := n.tableDesc.FindColumnWithName(t.Column)
if err != nil {
if t.IfExists {
// Noop.
continue
}
return err
}
if dropped {
if colToDrop.Dropped() {
continue
}

// If the dropped column uses a sequence, remove references to it from that sequence.
if len(colToDrop.UsesSequenceIds) > 0 {
if err := params.p.removeSequenceDependencies(params.ctx, n.tableDesc, colToDrop); err != nil {
if colToDrop.NumUsesSequences() > 0 {
if err := params.p.removeSequenceDependencies(params.ctx, n.tableDesc, colToDrop.ColumnDesc()); err != nil {
return err
}
}

// You can't remove a column that owns a sequence that is depended on
// by another column
if err := params.p.canRemoveAllColumnOwnedSequences(params.ctx, n.tableDesc, colToDrop, t.DropBehavior); err != nil {
if err := params.p.canRemoveAllColumnOwnedSequences(params.ctx, n.tableDesc, colToDrop.ColumnDesc(), t.DropBehavior); err != nil {
return err
}

if err := params.p.dropSequencesOwnedByCol(params.ctx, colToDrop, true /* queueJob */); err != nil {
if err := params.p.dropSequencesOwnedByCol(params.ctx, colToDrop.ColumnDesc(), true /* queueJob */); err != nil {
return err
}

Expand All @@ -429,7 +429,7 @@ func (n *alterTableNode) startExec(params runParams) error {
for _, ref := range n.tableDesc.DependedOnBy {
found := false
for _, colID := range ref.ColumnIDs {
if colID == colToDrop.ID {
if colID == colToDrop.GetID() {
found = true
break
}
Expand Down Expand Up @@ -471,13 +471,13 @@ func (n *alterTableNode) startExec(params runParams) error {
&params.p.semaCtx,
tn,
)
if err := computedColValidator.ValidateNoDependents(colToDrop); err != nil {
if err := computedColValidator.ValidateNoDependents(colToDrop.ColumnDesc()); err != nil {
return err
}

if n.tableDesc.GetPrimaryIndex().ContainsColumnID(colToDrop.ID) {
if n.tableDesc.GetPrimaryIndex().ContainsColumnID(colToDrop.GetID()) {
return pgerror.Newf(pgcode.InvalidColumnReference,
"column %q is referenced by the primary key", colToDrop.Name)
"column %q is referenced by the primary key", colToDrop.GetName())
}
var idxNamesToDelete []string
for _, idx := range n.tableDesc.NonDropIndexes() {
Expand All @@ -490,7 +490,7 @@ func (n *alterTableNode) startExec(params runParams) error {

// Analyze the index.
for j := 0; j < idx.NumColumns(); j++ {
if idx.GetColumnID(j) == colToDrop.ID {
if idx.GetColumnID(j) == colToDrop.GetID() {
containsThisColumn = true
break
}
Expand All @@ -506,7 +506,7 @@ func (n *alterTableNode) startExec(params runParams) error {
// sufficient reason to reject the DROP.
continue
}
if id == colToDrop.ID {
if id == colToDrop.GetID() {
containsThisColumn = true
break
}
Expand All @@ -517,7 +517,7 @@ func (n *alterTableNode) startExec(params runParams) error {
// loop below is for the new encoding (where the STORING columns are
// always in the value part of a KV).
for j := 0; j < idx.NumStoredColumns(); j++ {
if idx.GetStoredColumnID(j) == colToDrop.ID {
if idx.GetStoredColumnID(j) == colToDrop.GetID() {
containsThisColumn = true
break
}
Expand All @@ -537,7 +537,7 @@ func (n *alterTableNode) startExec(params runParams) error {
return err
}

if colIDs.Contains(colToDrop.ID) {
if colIDs.Contains(colToDrop.GetID()) {
containsThisColumn = true
}
}
Expand Down Expand Up @@ -566,7 +566,7 @@ func (n *alterTableNode) startExec(params runParams) error {
constraint := &n.tableDesc.UniqueWithoutIndexConstraints[i]
n.tableDesc.UniqueWithoutIndexConstraints[sliceIdx] = *constraint
sliceIdx++
if descpb.ColumnIDs(constraint.ColumnIDs).Contains(colToDrop.ID) {
if descpb.ColumnIDs(constraint.ColumnIDs).Contains(colToDrop.GetID()) {
sliceIdx--

// If this unique constraint is used on the referencing side of any FK
Expand All @@ -585,7 +585,7 @@ func (n *alterTableNode) startExec(params runParams) error {
// Drop check constraints which reference the column.
validChecks := n.tableDesc.Checks[:0]
for _, check := range n.tableDesc.AllActiveAndInactiveChecks() {
if used, err := n.tableDesc.CheckConstraintUsesColumn(check, colToDrop.ID); err != nil {
if used, err := n.tableDesc.CheckConstraintUsesColumn(check, colToDrop.GetID()); err != nil {
return err
} else if used {
if check.Validity == descpb.ConstraintValidity_Validating {
Expand All @@ -605,7 +605,7 @@ func (n *alterTableNode) startExec(params runParams) error {
if err != nil {
return err
}
if err := params.p.removeColumnComment(params.ctx, n.tableDesc.ID, colToDrop.ID); err != nil {
if err := params.p.removeColumnComment(params.ctx, n.tableDesc.ID, colToDrop.GetID()); err != nil {
return err
}

Expand All @@ -619,7 +619,7 @@ func (n *alterTableNode) startExec(params runParams) error {
n.tableDesc.OutboundFKs[sliceIdx] = n.tableDesc.OutboundFKs[i]
sliceIdx++
fk := &n.tableDesc.OutboundFKs[i]
if descpb.ColumnIDs(fk.OriginColumnIDs).Contains(colToDrop.ID) {
if descpb.ColumnIDs(fk.OriginColumnIDs).Contains(colToDrop.GetID()) {
sliceIdx--
if err := params.p.removeFKBackReference(params.ctx, n.tableDesc, fk); err != nil {
return err
Expand All @@ -631,8 +631,8 @@ func (n *alterTableNode) startExec(params runParams) error {

found := false
for i := range n.tableDesc.Columns {
if n.tableDesc.Columns[i].ID == colToDrop.ID {
n.tableDesc.AddColumnMutation(colToDrop, descpb.DescriptorMutation_DROP)
if n.tableDesc.Columns[i].ID == colToDrop.GetID() {
n.tableDesc.AddColumnMutation(colToDrop.ColumnDesc(), descpb.DescriptorMutation_DROP)
// Use [:i:i] to prevent reuse of existing slice, or outstanding refs
// to ColumnDescriptors may unexpectedly change.
n.tableDesc.Columns = append(n.tableDesc.Columns[:i:i], n.tableDesc.Columns[i+1:]...)
Expand Down Expand Up @@ -777,16 +777,16 @@ func (n *alterTableNode) startExec(params runParams) error {

case tree.ColumnMutationCmd:
// Column mutations
col, dropped, err := n.tableDesc.FindColumnByName(t.GetColumn())
col, err := n.tableDesc.FindColumnWithName(t.GetColumn())
if err != nil {
return err
}
if dropped {
if col.Dropped() {
return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
"column %q in the middle of being dropped", t.GetColumn())
}
// Apply mutations to copy of column descriptor.
if err := applyColumnMutation(params.ctx, n.tableDesc, col, t, params, n.n.Cmds, tn); err != nil {
if err := applyColumnMutation(params.ctx, n.tableDesc, col.ColumnDesc(), t, params, n.n.Cmds, tn); err != nil {
return err
}
descriptorChanged = true
Expand Down Expand Up @@ -1206,11 +1206,11 @@ func injectTableStats(

columnIDs := tree.NewDArray(types.Int)
for _, colName := range s.Columns {
colDesc, _, err := desc.FindColumnByName(tree.Name(colName))
col, err := desc.FindColumnWithName(tree.Name(colName))
if err != nil {
return err
}
if err := columnIDs.Append(tree.NewDInt(tree.DInt(colDesc.ID))); err != nil {
if err := columnIDs.Append(tree.NewDInt(tree.DInt(col.GetID()))); err != nil {
return err
}
}
Expand Down
Loading

0 comments on commit f3ffee8

Please sign in to comment.