sql/multiregion,sql/sem/builtins: minor refactorings to improve build

pkg/BUILD.bazel

@@ -229,6 +229,7 @@ ALL_TESTS = [
     "//pkg/sql/backfill:backfill_test",
     "//pkg/sql/catalog/catalogkeys:catalogkeys_test",
     "//pkg/sql/catalog/catformat:catformat_test",
+    "//pkg/sql/catalog/catpb:catpb_disallowed_imports_test",
     "//pkg/sql/catalog/catpb:catpb_test",
     "//pkg/sql/catalog/catprivilege:catprivilege_test",
     "//pkg/sql/catalog/colinfo:colinfo_test",
@@ -386,6 +387,7 @@ ALL_TESTS = [
     "//pkg/sql/schemachanger/screl:screl_test",
     "//pkg/sql/schemachanger/scrun:scrun_test",
     "//pkg/sql/schemachanger:schemachanger_test",
+    "//pkg/sql/sem/builtins:builtins_disallowed_imports_test",
     "//pkg/sql/sem/builtins:builtins_test",
     "//pkg/sql/sem/cast:cast_test",
     "//pkg/sql/sem/catconstants:catconstants_disallowed_imports_test",

pkg/sql/alter_table_locality.go

@@ -462,14 +462,14 @@ func (n *alterTableSetLocalityNode) startExec(params runParams) error {
 	newLocality := n.n.Locality
 	existingLocality := n.tableDesc.LocalityConfig
 
-	existingLocalityTelemetryName, err := existingLocality.TelemetryName()
+	existingLocalityTelemetryName, err := multiregion.TelemetryNameForLocalityConfig(existingLocality)
 	if err != nil {
 		return err
 	}
 	telemetry.Inc(
 		sqltelemetry.AlterTableLocalityCounter(
 			existingLocalityTelemetryName,
-			newLocality.TelemetryName(),
+			multiregion.TelemetryNameForLocality(newLocality),
 		),
 	)
 

pkg/sql/catalog/catpb/BUILD.bazel

@@ -1,6 +1,7 @@
 load("@rules_proto//proto:defs.bzl", "proto_library")
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")
+load("//pkg/testutils/buildutil:buildutil.bzl", "disallowed_imports_test")
 load("//build:STRINGER.bzl", "stringer")
 
 proto_library(
@@ -75,3 +76,10 @@ stringer(
     src = "privilege.go",
     typ = "PrivilegeDescVersion",
 )
+
+disallowed_imports_test(
+    src = "catpb",
+    disallowed_list = [
+        "//pkg/sql/sem/tree",
+    ],
+)

pkg/sql/catalog/catpb/multiregion.go

@@ -10,11 +10,6 @@
 
 package catpb
 
-import (
-	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
-	"github.com/cockroachdb/errors"
-)
-
 // RegionName is an alias for a region stored on the database.
 type RegionName string
 
@@ -34,25 +29,3 @@ func (regions RegionNames) ToStrings() []string {
 	}
 	return ret
 }
-
-// TelemetryName returns the name to use for the given locality.
-func (cfg *LocalityConfig) TelemetryName() (string, error) {
-	switch l := cfg.Locality.(type) {
-	case *LocalityConfig_Global_:
-		return tree.TelemetryNameGlobal, nil
-	case *LocalityConfig_RegionalByTable_:
-		if l.RegionalByTable.Region != nil {
-			return tree.TelemetryNameRegionalByTableIn, nil
-		}
-		return tree.TelemetryNameRegionalByTable, nil
-	case *LocalityConfig_RegionalByRow_:
-		if l.RegionalByRow.As != nil {
-			return tree.TelemetryNameRegionalByRowAs, nil
-		}
-		return tree.TelemetryNameRegionalByRow, nil
-	}
-	return "", errors.AssertionFailedf(
-		"unknown locality config TelemetryName: type %T",
-		cfg.Locality,
-	)
-}

pkg/sql/catalog/multiregion/BUILD.bazel

@@ -4,6 +4,7 @@ go_library(
     name = "multiregion",
     srcs = [
         "region_config.go",
+        "telemetry.go",
         "validate_table.go",
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/sql/catalog/multiregion",

pkg/sql/catalog/multiregion/telemetry.go

@@ -0,0 +1,71 @@
+// Copyright 2022 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package multiregion
+
+import (
+	"fmt"
+
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/errors"
+)
+
+// Constants to use for telemetry for multi-region table localities.
+const (
+	TelemetryNameGlobal            = "global"
+	TelemetryNameRegionalByTable   = "regional_by_table"
+	TelemetryNameRegionalByTableIn = "regional_by_table_in"
+	TelemetryNameRegionalByRow     = "regional_by_row"
+	TelemetryNameRegionalByRowAs   = "regional_by_row_as"
+)
+
+// TelemetryNameForLocality returns the telemetry name for a given locality level.
+func TelemetryNameForLocality(node *tree.Locality) string {
+	switch node.LocalityLevel {
+	case tree.LocalityLevelGlobal:
+		return TelemetryNameGlobal
+	case tree.LocalityLevelTable:
+		if node.TableRegion != tree.RegionalByRowRegionNotSpecifiedName {
+			return TelemetryNameRegionalByTableIn
+		}
+		return TelemetryNameRegionalByTable
+	case tree.LocalityLevelRow:
+		if node.RegionalByRowColumn != tree.PrimaryRegionNotSpecifiedName {
+			return TelemetryNameRegionalByRowAs
+		}
+		return TelemetryNameRegionalByRow
+	default:
+		panic(fmt.Sprintf("unknown locality: %#v", node.LocalityLevel))
+	}
+}
+
+// TelemetryNameForLocalityConfig returns the name to use for the given
+// locality config.
+func TelemetryNameForLocalityConfig(cfg *catpb.LocalityConfig) (string, error) {
+	switch l := cfg.Locality.(type) {
+	case *catpb.LocalityConfig_Global_:
+		return TelemetryNameGlobal, nil
+	case *catpb.LocalityConfig_RegionalByTable_:
+		if l.RegionalByTable.Region != nil {
+			return TelemetryNameRegionalByTableIn, nil
+		}
+		return TelemetryNameRegionalByTable, nil
+	case *catpb.LocalityConfig_RegionalByRow_:
+		if l.RegionalByRow.As != nil {
+			return TelemetryNameRegionalByRowAs, nil
+		}
+		return TelemetryNameRegionalByRow, nil
+	}
+	return "", errors.AssertionFailedf(
+		"unknown locality config TelemetryNameForLocality: type %T",
+		cfg.Locality,
+	)
+}

pkg/sql/create_table.go

@@ -2253,7 +2253,7 @@ func NewTableDesc(
 	if regionConfig != nil || n.Locality != nil {
 		localityTelemetryName := "unspecified"
 		if n.Locality != nil {
-			localityTelemetryName = n.Locality.TelemetryName()
+			localityTelemetryName = multiregion.TelemetryNameForLocality(n.Locality)
 		}
 		telemetry.Inc(sqltelemetry.CreateTableLocalityCounter(localityTelemetryName))
 		if n.Locality == nil {

pkg/sql/sem/builtins/BUILD.bazel

@@ -1,4 +1,5 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+load("//pkg/testutils/buildutil:buildutil.bzl", "disallowed_imports_test")
 
 go_library(
     name = "builtins",
@@ -50,7 +51,6 @@ go_library(
         "//pkg/sql/catalog",
         "//pkg/sql/catalog/catalogkeys",
         "//pkg/sql/catalog/descpb",
-        "//pkg/sql/catalog/descs",
         "//pkg/sql/colexecerror",
         "//pkg/sql/lex",
         "//pkg/sql/lexbase",
@@ -168,3 +168,10 @@ go_test(
         "@com_github_stretchr_testify//require",
     ],
 )
+
+disallowed_imports_test(
+    src = "builtins",
+    disallowed_list = [
+        "//pkg/sql/catalog/descs",
+    ],
+)

pkg/sql/sem/builtins/builtins.go

@@ -50,7 +50,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
 	"github.com/cockroachdb/cockroach/pkg/sql/colexecerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/lex"
 	"github.com/cockroachdb/cockroach/pkg/sql/lexbase"
@@ -5558,14 +5557,11 @@ value if you rely on the HLC for accuracy.`,
 				tableID := int(tree.MustBeDInt(args[0]))
 				indexID := int(tree.MustBeDInt(args[1]))
 				g := tree.MustBeDGeography(args[2])
-				// TODO(postamar): give the eval.Context a useful interface
-				// instead of cobbling a descs.Collection in this way.
-				cf := descs.NewBareBonesCollectionFactory(ctx.Settings, ctx.Codec)
-				descsCol := cf.MakeCollection(ctx.Context, descs.NewTemporarySchemaProvider(ctx.SessionDataStack), nil /* monitor */)
-				tableDesc, err := descsCol.Direct().MustGetTableDescByID(ctx.Ctx(), ctx.Txn, descpb.ID(tableID))
+				tableI, err := ctx.Planner.GetImmutableTableInterfaceByID(ctx.Ctx(), tableID)
 				if err != nil {
 					return nil, err
 				}
+				tableDesc := tableI.(catalog.TableDescriptor)
 				index, err := tableDesc.FindIndexWithID(descpb.IndexID(indexID))
 				if err != nil {
 					return nil, err
@@ -5596,14 +5592,11 @@ value if you rely on the HLC for accuracy.`,
 				tableID := int(tree.MustBeDInt(args[0]))
 				indexID := int(tree.MustBeDInt(args[1]))
 				g := tree.MustBeDGeometry(args[2])
-				// TODO(postamar): give the eval.Context a useful interface
-				// instead of cobbling a descs.Collection in this way.
-				cf := descs.NewBareBonesCollectionFactory(ctx.Settings, ctx.Codec)
-				descsCol := cf.MakeCollection(ctx.Context, descs.NewTemporarySchemaProvider(ctx.SessionDataStack), nil /* monitor */)
-				tableDesc, err := descsCol.Direct().MustGetTableDescByID(ctx.Ctx(), ctx.Txn, descpb.ID(tableID))
+				tableI, err := ctx.Planner.GetImmutableTableInterfaceByID(ctx.Ctx(), tableID)
 				if err != nil {
 					return nil, err
 				}
+				tableDesc := tableI.(catalog.TableDescriptor)
 				index, err := tableDesc.FindIndexWithID(descpb.IndexID(indexID))
 				if err != nil {
 					return nil, err

pkg/sql/sem/tree/region.go

@@ -56,35 +56,6 @@ type Locality struct {
 	RegionalByRowColumn Name
 }
 
-// Constants to use for telemetry for multi-region table localities.
-const (
-	TelemetryNameGlobal            = "global"
-	TelemetryNameRegionalByTable   = "regional_by_table"
-	TelemetryNameRegionalByTableIn = "regional_by_table_in"
-	TelemetryNameRegionalByRow     = "regional_by_row"
-	TelemetryNameRegionalByRowAs   = "regional_by_row_as"
-)
-
-// TelemetryName returns the telemetry name for a given locality level.
-func (node *Locality) TelemetryName() string {
-	switch node.LocalityLevel {
-	case LocalityLevelGlobal:
-		return TelemetryNameGlobal
-	case LocalityLevelTable:
-		if node.TableRegion != RegionalByRowRegionNotSpecifiedName {
-			return TelemetryNameRegionalByTableIn
-		}
-		return TelemetryNameRegionalByTable
-	case LocalityLevelRow:
-		if node.RegionalByRowColumn != PrimaryRegionNotSpecifiedName {
-			return TelemetryNameRegionalByRowAs
-		}
-		return TelemetryNameRegionalByRow
-	default:
-		panic(fmt.Sprintf("unknown locality: %#v", node.LocalityLevel))
-	}
-}
-
 // Format implements the NodeFormatter interface.
 func (node *Locality) Format(ctx *FmtCtx) {
 	ctx.WriteString("LOCALITY ")