Merge #59633

59633: multiregion: replace TODO and unimplemented with issue numbers r=ajstorm a=otan In the spirit of Milestone planning, replace all remaining multiregion TODOs with issue numbers. Release note: None Co-authored-by: Oliver Tan <[email protected]>
cockroachdb · Feb 2, 2021 · 0e67278 · 0e67278
2 parents e854509 + 106632e
commit 0e67278
Show file tree

Hide file tree

Showing 7 changed files with 12 additions and 21 deletions.
diff --git a/pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality b/pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality
@@ -16,7 +16,7 @@ statement ok
 use alter_locality_test
 
 # Turn on experimental flag to allow REGIONAL BY ROW table creation
-# TODO(#multiregion): remove this once flag is no longer needed for REGIONAL BY ROW tables
+# TODO(#59629): remove this once flag is no longer needed for REGIONAL BY ROW tables
 statement ok
 SET experimental_enable_implicit_column_partitioning = true
 

diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row
@@ -81,7 +81,7 @@ CREATE TABLE regional_by_row_table (
   FAMILY (pk, pk2, a, b)
 ) LOCALITY REGIONAL BY ROW
 
-# TODO(#multiregion): determine how we should display the presence of a crdb_region column
+# TODO(#59630): determine how we should display the presence of a crdb_region column
 # to the user.
 # TODO(#59362): ensure this CREATE TABLE statement round trips.
 query T

diff --git a/pkg/sql/alter_database.go b/pkg/sql/alter_database.go
@@ -260,7 +260,7 @@ func (p *planner) AlterDatabaseDropRegion(
 	); err != nil {
 		return nil, err
 	}
-	return nil, unimplemented.New("alter database drop region", "implementation pending")
+	return nil, unimplemented.NewWithIssue(58333, "implementation pending")
 }
 
 type alterDatabasePrimaryRegionNode struct {

diff --git a/pkg/sql/alter_table_locality.go b/pkg/sql/alter_table_locality.go
@@ -181,11 +181,9 @@ func (n *alterTableSetLocalityNode) startExec(params runParams) error {
 	case *descpb.TableDescriptor_LocalityConfig_Global_:
 		switch newLocality.LocalityLevel {
 		case tree.LocalityLevelGlobal:
-			// GLOBAL to GLOBAL - no op.
 			return nil
 		case tree.LocalityLevelRow:
-			// GLOBAL to REGIONAL BY ROW
-			return unimplemented.New("alter table locality to REGIONAL BY ROW", "implementation pending")
+			return unimplemented.NewWithIssue(58341, "implementation pending")
 		case tree.LocalityLevelTable:
 			if err = n.alterTableLocalityGlobalToRegionalByTable(params, dbDesc); err != nil {
 				return err
@@ -201,7 +199,7 @@ func (n *alterTableSetLocalityNode) startExec(params runParams) error {
 				return err
 			}
 		case tree.LocalityLevelRow:
-			return unimplemented.New("alter table locality to REGIONAL BY ROW", "implementation pending")
+			return unimplemented.NewWithIssue(58341, "implementation pending")
 		case tree.LocalityLevelTable:
 			err = n.alterTableLocalityRegionalByTableToRegionalByTable(params, dbDesc)
 			if err != nil {
@@ -213,12 +211,11 @@ func (n *alterTableSetLocalityNode) startExec(params runParams) error {
 	case *descpb.TableDescriptor_LocalityConfig_RegionalByRow_:
 		switch newLocality.LocalityLevel {
 		case tree.LocalityLevelGlobal:
-			return unimplemented.New("alter table locality from REGIONAL BY ROW", "implementation pending")
+			return unimplemented.NewWithIssue(59632, "implementation pending")
 		case tree.LocalityLevelRow:
-			// Altering to same table locality pattern. We're done.
-			return unimplemented.New("alter table locality from REGIONAL BY ROW", "implementation pending")
+			return unimplemented.NewWithIssue(59632, "implementation pending")
 		case tree.LocalityLevelTable:
-			return unimplemented.New("alter table locality from REGIONAL BY ROW", "implementation pending")
+			return unimplemented.NewWithIssue(59632, "implementation pending")
 		default:
 			return errors.AssertionFailedf("unknown table locality: %v", newLocality)
 		}

diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go
@@ -362,8 +362,6 @@ func (n *createTableNode) startExec(params runParams) error {
 		return err
 	}
 
-	// TODO(otan): for MR databases with no locality set, set a default locality
-	// and add a notice.
 	if desc.LocalityConfig != nil {
 		dbDesc, err := params.p.Descriptors().GetImmutableDatabaseByID(
 			params.ctx,
@@ -1447,7 +1445,7 @@ func NewTableDesc(
 	locality := n.Locality
 	var partitionAllBy *tree.PartitionBy
 	if locality != nil && locality.LocalityLevel == tree.LocalityLevelRow {
-		// TODO(#multiregion): consider decoupling implicit column partitioning from the
+		// TODO(#59629): consider decoupling implicit column partitioning from the
 		// REGIONAL BY ROW experimental flag.
 		if !evalCtx.SessionData.ImplicitColumnPartitioningEnabled {
 			return nil, errors.WithHint(
@@ -1547,7 +1545,7 @@ func NewTableDesc(
 				)
 			}
 			oid := typedesc.TypeIDToOID(dbDesc.RegionConfig.RegionEnumID)
-			// TODO(#multiregion): set the column visibility to be hidden.
+			// TODO(#59630): set the column visibility to be hidden.
 			c := &tree.ColumnTableDef{
 				Name: tree.RegionalByRowRegionDefaultColName,
 				Type: &tree.OIDTypeReference{OID: oid},

diff --git a/pkg/sql/region_util.go b/pkg/sql/region_util.go
@@ -385,7 +385,7 @@ func zoneConfigForMultiRegionTable(
 	return ret, nil
 }
 
-// TODO(#multiregion): everything using this should instead use getZoneConfigRaw
+// TODO(#59631): everything using this should instead use getZoneConfigRaw
 // and writeZoneConfig instead of calling SQL for each query.
 // This removes the requirement to only call this function after writeSchemaChange
 // is called on creation of tables, and potentially removes the need for ReadingOwnWrites
@@ -394,9 +394,6 @@ func zoneConfigForMultiRegionTable(
 func (p *planner) applyZoneConfigForMultiRegion(
 	ctx context.Context, zs tree.ZoneSpecifier, zc *zonepb.ZoneConfig, desc string,
 ) error {
-	// TODO(#multiregion): We should check to see if the zone configuration has been updated
-	// by the user. If it has, we need to warn, and only proceed if sql_safe_updates is disabled.
-
 	// Convert the partially filled zone config to re-run as a SQL command.
 	// This avoid us having to modularize planNode logic from set_zone_config
 	// and the optimizer.

diff --git a/pkg/sql/sem/tree/region.go b/pkg/sql/sem/tree/region.go
@@ -38,8 +38,7 @@ const (
 	RegionalByRowRegionDefaultColName Name = Name(RegionalByRowRegionDefaultCol)
 	// PrimaryRegionLocalityName is the string denoting the primary region in the
 	// locality config.
-	// TODO(#multiregion): clean this up to use something nicer,
-	// see https://github.com/cockroachdb/cockroach/issues/59455.
+	// TODO(#59455): clean this up to use something nicer,
 	PrimaryRegionLocalityName Name = ""
 )