sql: add catalog.Column interface, replace catalog.TableDescriptor methods for columns

[postamar]

In an effort to not directly use descpb.ColumnDescriptor protos everywhere, this multi-commit PR:

1. introduces a new catalog.Column interface to encapsulate them, and a bunch of new methods in catalog.TableDescriptor to replace those which have descpb.ColumnDescriptor in their signatures, this is done in the first commit;
2. replaces calls to the old methods with calls to the new methods throughout the codebase in the subsequent commits.

This breakdown into multiple commits is done to ease review, most of the substantial changes are in the first commit, the others follow through with the change and so are quite noisy.

Fixes #59805.

See also the parent issue #56306, as well as the related issue #57465 which tracks the same work I've already done for descpb.IndexDescriptor. In this PR I reused many of the same patterns I introduced in the corresponding PR #58678.

Subsequent work should consist in propagating this descpb.ColumnDescriptor -> catalog.Column change further throughout the codebase.

[cockroach-teamcity]

This change is 

[ajwerner]

just tiny comment things.

Reviewed 10 of 10 files at r1, 10 of 10 files at r2, 36 of 36 files at r3, 22 of 22 files at r4, 25 of 25 files at r5, 48 of 48 files at r6, 70 of 70 files at r7.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru and @postamar)

---

pkg/sql/opt_catalog.go, line 638 at r4 (raw file):

	}

	ot.columns = make([]cat.Column, len(cols), numCols)

this cat.Column thing is something to pay attention to.

---

pkg/sql/opt_exec_factory.go, line 1956 at r6 (raw file):

}

func makePublicColIdxToRetIdx(

The name of this function doesn't clearly describe to me its behavior; a comment would be helpful.

---

pkg/sql/catalog/descriptor.go, line 404 at r1 (raw file):

	// The ordinal of a column in a `tableDesc descpb.TableDescriptor` is
	// defined as follows:
	// - [0:len(tableDesc.Columns)] is the range of public columns,

Should we comment this in terms of methods rather than fields?

---

pkg/sql/catalog/descriptor.go, line 411 at r1 (raw file):

Quoted 15 lines of code…

	// Public returns true iff the column is active, i.e. readable, in the table
	// descriptor.
	Public() bool
	// WriteAndDeleteOnly returns true iff the column is a mutation in the
	// delete-and-write-only state in the table descriptor.
	WriteAndDeleteOnly() bool
	// DeleteOnly returns true iff the column is a mutation in the delete-only
	// state in the table descriptor.
	DeleteOnly() bool
	// Adding returns true iff the column is an add mutation in the table
	// descriptor.
	Adding() bool
	// Dropped returns true iff the column is a drop mutation in the table
	// descriptor.
	Dropped() bool

Are we happy with these as booleans? I know we did this on indexes but I have mixed feelings about it vs. an enum.

---

After having reviewed this a bit, I think these works out pretty well, 🤷

---

pkg/sql/catalog/tabledesc/table.go, line 466 at r1 (raw file):

// like FindPublicColumnWithName applied repeatedly to the names in the
// provided list.
func FindPublicColumnsWithNames(

Note that an error will be returned if any of the names do not exist.

[postamar]

Thanks for reviewing and for the quick turnaround. I'm going to hurry up and merge this before more conflicts arise. I already had to rebase once.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru and @ajwerner)

---

pkg/sql/opt_catalog.go, line 638 at r4 (raw file):

Previously, ajwerner wrote…

this cat.Column thing is something to pay attention to.

Yes, definitely! Unifying catalog.Column and cat.Column is a logical next step. I'll update the parent issue accordingly.

---

pkg/sql/opt_exec_factory.go, line 1956 at r6 (raw file):

Previously, ajwerner wrote…

The name of this function doesn't clearly describe to me its behavior; a comment would be helpful.

Done.

---

pkg/sql/catalog/descriptor.go, line 404 at r1 (raw file):

Previously, ajwerner wrote…

Should we comment this in terms of methods rather than fields?

We do, it's at the bottom of the comment block, which follows the same structure as in catalog.Index.Ordinal. I might revise all of them once we're done introducing these new interfaces.

---

pkg/sql/catalog/descriptor.go, line 411 at r1 (raw file):

Previously, ajwerner wrote…

	// Public returns true iff the column is active, i.e. readable, in the table
	// descriptor.
	Public() bool
	// WriteAndDeleteOnly returns true iff the column is a mutation in the
	// delete-and-write-only state in the table descriptor.
	WriteAndDeleteOnly() bool
	// DeleteOnly returns true iff the column is a mutation in the delete-only
	// state in the table descriptor.
	DeleteOnly() bool
	// Adding returns true iff the column is an add mutation in the table
	// descriptor.
	Adding() bool
	// Dropped returns true iff the column is a drop mutation in the table
	// descriptor.
	Dropped() bool

Are we happy with these as booleans? I know we did this on indexes but I have mixed feelings about it vs. an enum.

---

After having reviewed this a bit, I think these works out pretty well, 🤷

Thanks but I just copied the prevailing pattern. I usually like boolean method names to begin with Is or Has anyway.

---

pkg/sql/catalog/tabledesc/table.go, line 466 at r1 (raw file):

Previously, ajwerner wrote…

Note that an error will be returned if any of the names do not exist.

Good point. I updated the comment.

[postamar]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)