opt, cat: cleaner API for mutation columns by RaduBerinde · Pull Request #51400 · cockroachdb/cockroach

RaduBerinde · 2020-07-14T00:19:44Z

NOTE I used AllColumnCount so that the compiler forces me to look at all callsites, and left it like this so they are also apparent in the diff. I intend to rename it to ColumnCount before merging.

The catalog currently relies on a particular ordering to separate
different kinds of columns (ordinary, write-only, delete-only). This
is unfortunate because it restricts the underlying implementation; in
particular, it makes it tricky to add a new kind of column (like the
mvcc_timestamp system column) without potentially breaking existing
code in subtle ways.

This change reworks the cat API to have a single ColumnCount method
and a separate ColumnKind method used to figure out the column kind.
This forces all code that cares about the kind to check it explicitly
rather than relying on a potentially faulty column count.

Fixes #51323.

Release note: None

cockroach-teamcity · 2020-07-14T00:19:52Z

This change is

andy-kimball

LGTM, with some nits.

andy-kimball · 2020-07-14T00:25:54Z

pkg/sql/opt/cat/utils.go

 // ConvertColumnIDsToOrdinals converts a list of ColumnIDs (such as from a
-// tree.TableRef), to a list of ordinal positions of columns within the given
-// table. See tree.Table for more information on column ordinals.
+// tree.TableRef), to a list of ordinal positions of non-mutation columns within


It seems like an API bug that this only looks at non-mutation cols. Why not let it handle mutation cols as well, since the API is generic? I don't think any existing caller calls it with mutation cols, so there should be no panics.

andy-kimball · 2020-07-14T00:30:02Z

pkg/sql/opt/cat/utils.go

@@ -147,8 +147,8 @@ func ConvertColumnIDsToOrdinals(tab Table, columns []tree.ColumnID) (ordinals []
 // FindTableColumnByName returns the ordinal of the non-mutation column having
 // the given name, if one exists in the given table. Otherwise, it returns -1.
 func FindTableColumnByName(tab Table, name tree.Name) int {


Should we make this return mutation cols as well and require caller to test for mutation/system cols? The more col types we add, the more confusing it is when some generic-sounding APIs filter cols and others don't.

andy-kimball · 2020-07-14T00:34:42Z

pkg/sql/opt/memo/logical_props_builder.go

-	for i, n := 0, tab.ColumnCount(); i < n; i++ {
-		if private.IsColumnOutput(i) {
+	for i, n := 0, tab.AllColumnCount(); i < n; i++ {
+		if !cat.IsMutationColumn(tab, i) && private.IsColumnOutput(i) {


Actually, I think we can just get rid of the comment and the check for IsMutationColumn. private.IsColumnOutput should always return false for mutation columns, or else there's a bug somewhere else in the code. We always set private.ReturnCols entries to zero for mutation cols.

andy-kimball · 2020-07-14T00:36:17Z

pkg/sql/opt/memo/logical_props_builder.go

 	// the column must be not null.
-	for i, n := 0, tab.ColumnCount(); i < n; i++ {
+	for i, n := 0, tab.AllColumnCount(); i < n; i++ {
+		if cat.IsMutationColumn(tab, i) {


The OutputCols check should already take care of this case, since mutation cols will never be output cols by the check above. I think it's good to let the mutation builder decide which cols are output or not, and let this props builder code just blindly process the cols without checking again.

andy-kimball · 2020-07-14T00:39:48Z

pkg/sql/opt/memo/multiplicity_builder.go

-			for i, cnt := 0, baseTable.ColumnCount(); i < cnt; i++ {
-				unfilteredCols.Add(t.Table.ColumnID(i))
+			for i, cnt := 0, baseTable.AllColumnCount(); i < cnt; i++ {
+				if !cat.IsMutationColumn(baseTable, i) {


I don't see any harm in adding mutation cols here. I think Drew just used ColumnCount b/c it seemed like the right thing. But really we want all columns.

andy-kimball · 2020-07-14T00:41:06Z

pkg/sql/opt/memo/statistics_builder_test.go


 		var cols opt.ColSet
-		for i := 0; i < tab.ColumnCount(); i++ {
+		for i := 0; i < tab.AllColumnCount(); i++ {


I wonder how mutation cols play with stats. A good example of a case where using ColumnCount can cause us to not consider a case that is potentially interesting to think about, or at least document.

andy-kimball · 2020-07-14T00:44:41Z

pkg/sql/opt/optbuilder/insert.go

@@ -1054,9 +1057,9 @@ func (mb *mutationBuilder) mapColumnNamesToOrdinals(names tree.NameList) util.Fa
 	var ords util.FastIntSet


Can you add to the mapColumnNamesToOrdinals to make it clear that it skips mutation columns?

andy-kimball · 2020-07-14T00:46:22Z

pkg/sql/opt/optbuilder/mutation_builder.go


 	if needResults {
-		// Only non-mutation columns are output columns. ReturnCols needs to have
 		// DeletableColumnCount entries, but only the first ColumnCount entries


Need to update the comment to remove ref to DeletableColumnCount.

andy-kimball · 2020-07-14T00:54:02Z

pkg/sql/opt/optbuilder/select.go

@@ -577,8 +578,8 @@ func (b *Builder) addCheckConstraintsForTable(tabMeta *opt.TableMeta) {

 	// Find the non-nullable table columns.


Add parenthetical to the comment, saying that mutation cols can be NULL during backfill, so they should be excluded.

andy-kimball · 2020-07-14T00:56:36Z

pkg/sql/opt/optbuilder/select.go

 	var tableScope *scope
 	tab := tabMeta.Table
-	for i, n := 0, tab.ColumnCount(); i < n; i++ {
+	for i, n := 0, tab.AllColumnCount(); i < n; i++ {


Add comment to explain why mutation cols should not be included here.

rohany · 2020-07-14T01:37:21Z

From an outside perspective, I like this a lot. It's much more explicit about what count to use/what columns are considered in an operation, and will also make it easier for me to catch all of the cases where we should or shouldn't include system columns.

The catalog currently relies on a particular ordering to separate different kinds of columns (ordinary, write-only, delete-only). This is unfortunate because it restricts the underlying implementation; in particular, it makes it tricky to add a new kind of column (like the `mvcc_timestamp` system column) without potentially breaking existing code in subtle ways. This change reworks the cat API to have a single `ColumnCount` method and a separate `ColumnKind` method used to figure out the column kind. This forces all code that cares about the kind to check it explicitly rather than relying on a potentially faulty column count. Fixes cockroachdb#51323. Release note: None

Also add some tests that fail if we don't ignore mutation columns. Release note: None

RaduBerinde

Updated.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @mgartner, @RaduBerinde, @rohany, and @rytaft)

pkg/sql/opt/cat/utils.go, line 125 at r1 (raw file):