Merge #57584 #57606 #57622

57584: sql: lift multi-region functions on to immutable descriptors r=ajwerner a=arulajmani

Previously, we added some accessors on type and database descriptors
that operated on the raw proto. Since, I've learned that this stuff
should really be on immutable types instead, as we're moving to a new
world where we move away from raw proto accesses. This patch lifts
accessors from the raw proto and on to the immutable types.

Release note: None

57606: sql: make CREATE STATISTICS work properly with EXPLAIN ANALYZE (DEBUG) r=RaduBerinde a=RaduBerinde

#### sql: migrate other EXPLAIN variants to new exec factory interface

This change migrates the other EXPLAIN variants to the new interface,
which uses an explain factory to build the explained plan.

This fixes a TODO in the experimental factory.

Release note: None

#### opt: add operator for CREATE STATISTICS

This change adds an optimizer operator for CREATE STATISTICS (instead
of using the opaque operator).

Release note: None

#### sql: reimplement EXPLAIN handling of CREATE STATISTICS

CREATE STATISTICS runs the statistics plan inside a job. The plan node
tree ends in a dead end at the createStatsNode. For explain purposes,
we want to see information about the actual plan that runs inside the
job.

The current solution is to pretend that we build the plan directly in
certain contexts. This solution is hard to extend for the new
top-level instrumentation for EXPLAIN ANALYZE.

This commit changes this solution to add a `runAsJob` flag and
actually plan the distributed plan directly when we don't run as a
job. We automatically set `runAsJob=false` whenever we are building
inside Explain.

Release note: None

#### sql: make CREATE STATISTICS work properly with EXPLAIN ANALYZE (DEBUG)

With this change, the bundle now contains the diagram for the
underlying plan.

Release note: None

57622: opt: parser, test catalog, query support for virtual columns r=RaduBerinde a=RaduBerinde

This set of changes add parser, testcat, and query support for virtual columns. I have come full circle and realized that even though user-defined virtual columns don't help with the optimizer-side work for expression-based indexes, they help with integrating with the existing sql code. In particular, we can carve out pieces of work and write more targeted tests related to virtual columns, without having to define them through an expression-based index.

Informs #57608.

#### sql: support virtual columns in parser

Support VIRTUAL keyword in the parser. We still error out if it is
ever used.

Release note: None

#### opt: clean up hack in test catalog

To ensure that PK columns are non-nullable, the test catalog
reinitializes them to override the nullable flag. We replace this
hacky mechanism with figuring out the PK columns beforehand and
marking them as non-nullable in the definition before creating the
table columns.

Release note: None

#### opt: test catalog support for virtual columns

Release note: None

#### opt: optbuilder support for scanning virtual columns

This change adds support for scanning tables with virtual columns. The
virtual columns are projected on top of the Scan.

Release note: None

Co-authored-by: arulajmani <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>

docs/generated/sql/bnf/col_qualification.bnf

@@ -9,6 +9,8 @@ col_qualification ::=
 	| 'CONSTRAINT' constraint_name 'REFERENCES' table_name opt_name_parens key_match reference_actions
 	| 'CONSTRAINT' constraint_name 'AS' '(' a_expr ')' 'STORED'
 	| 'CONSTRAINT' constraint_name 'GENERATED_ALWAYS' 'ALWAYS' 'AS' '(' a_expr ')' 'STORED'
+	| 'CONSTRAINT' constraint_name 'AS' '(' a_expr ')' 'VIRTUAL'
+	| 'CONSTRAINT' constraint_name 'GENERATED_ALWAYS' 'ALWAYS' 'AS' '(' a_expr ')' 'VIRTUAL'
 	| 'NOT' 'NULL'
 	| 'NULL'
 	| 'UNIQUE' opt_without_index
@@ -19,6 +21,8 @@ col_qualification ::=
 	| 'REFERENCES' table_name opt_name_parens key_match reference_actions
 	| 'AS' '(' a_expr ')' 'STORED'
 	| 'GENERATED_ALWAYS' 'ALWAYS' 'AS' '(' a_expr ')' 'STORED'
+	| 'AS' '(' a_expr ')' 'VIRTUAL'
+	| 'GENERATED_ALWAYS' 'ALWAYS' 'AS' '(' a_expr ')' 'VIRTUAL'
 	| 'COLLATE' collation_name
 	| 'FAMILY' family_name
 	| 'CREATE' 'FAMILY' family_name

docs/generated/sql/bnf/stmt_block.bnf

@@ -2874,6 +2874,7 @@ col_qualification_elem ::=
 	| 'DEFAULT' b_expr
 	| 'REFERENCES' table_name opt_name_parens key_match reference_actions
 	| generated_as '(' a_expr ')' 'STORED'
+	| generated_as '(' a_expr ')' 'VIRTUAL'
 
 family_name ::=
 	name

pkg/sql/add_column.go

@@ -18,6 +18,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
+	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
 	"github.com/cockroachdb/errors"
 )
 
@@ -142,6 +143,9 @@ func (p *planner) addColumnImpl(
 	}
 
 	if d.IsComputed() {
+		if d.IsVirtual() {
+			return unimplemented.NewWithIssue(57608, "virtual computed columns")
+		}
 		computedColValidator := schemaexpr.MakeComputedColumnValidator(
 			params.ctx,
 			n.tableDesc,

pkg/sql/catalog/dbdesc/database_desc.go

@@ -206,6 +206,30 @@ func (desc *Immutable) DescriptorProto() *descpb.Descriptor {
 	}
 }
 
+// IsMultiRegion returns whether the database has multi-region properties
+// configured. If so, desc.RegionConfig can be used.
+func (desc *Immutable) IsMultiRegion() bool {
+	return desc.RegionConfig != nil
+}
+
+// Regions returns the multi-region regions that have been added to a database.
+func (desc *Immutable) Regions() (descpb.Regions, error) {
+	if !desc.IsMultiRegion() {
+		return nil, errors.AssertionFailedf(
+			"can not get regions of a non multi-region database")
+	}
+	return desc.RegionConfig.Regions, nil
+}
+
+// PrimaryRegion returns the primary region for a multi-region database.
+func (desc *Immutable) PrimaryRegion() (descpb.Region, error) {
+	if !desc.IsMultiRegion() {
+		return "", errors.AssertionFailedf(
+			"can not get the primary region of a non multi-region database")
+	}
+	return desc.RegionConfig.PrimaryRegion, nil
+}
+
 // SetName sets the name on the descriptor.
 func (desc *Mutable) SetName(name string) {
 	desc.Name = name

pkg/sql/catalog/descpb/structured.go

@@ -324,36 +324,3 @@ func (DescriptorState) SafeValue() {}
 
 // SafeValue implements the redact.SafeValue interface.
 func (ConstraintType) SafeValue() {}
-
-// IsMultiRegion returns whether the database has multi-region properties
-// configured. If so, desc.RegionConfig can be used.
-func (desc *DatabaseDescriptor) IsMultiRegion() bool {
-	return desc.RegionConfig != nil
-}
-
-// Regions returns the multi-region regions that have been added to a database.
-func (desc *DatabaseDescriptor) Regions() (Regions, error) {
-	if !desc.IsMultiRegion() {
-		return nil, errors.AssertionFailedf(
-			"can not get regions of a non multi-region database")
-	}
-	return desc.RegionConfig.Regions, nil
-}
-
-// PrimaryRegion returns the primary region for a multi-region database.
-func (desc *DatabaseDescriptor) PrimaryRegion() (Region, error) {
-	if !desc.IsMultiRegion() {
-		return "", errors.AssertionFailedf(
-			"can not get the primary region of a non multi-region database")
-	}
-	return desc.RegionConfig.PrimaryRegion, nil
-}
-
-// PrimaryRegion returns the primary region for a multi-region type descriptor.
-func (desc *TypeDescriptor) PrimaryRegion() (Region, error) {
-	if desc.Kind != TypeDescriptor_MULTIREGION_ENUM {
-		return "", errors.AssertionFailedf(
-			"can not get primary region of a non multi-region type desc")
-	}
-	return desc.RegionConfig.PrimaryRegion, nil
-}

pkg/sql/catalog/typedesc/type_desc.go

@@ -201,6 +201,15 @@ func (desc *Immutable) DescriptorProto() *descpb.Descriptor {
 	}
 }
 
+// PrimaryRegion returns the primary region for a multi-region type descriptor.
+func (desc *Immutable) PrimaryRegion() (descpb.Region, error) {
+	if desc.Kind != descpb.TypeDescriptor_MULTIREGION_ENUM {
+		return "", errors.AssertionFailedf(
+			"can not get primary region of a non multi-region type desc")
+	}
+	return desc.RegionConfig.PrimaryRegion, nil
+}
+
 // SetDrainingNames implements the MutableDescriptor interface.
 func (desc *Mutable) SetDrainingNames(names []descpb.NameInfo) {
 	desc.DrainingNames = names

pkg/sql/create_stats.go

@@ -56,39 +56,6 @@ var featureStatsEnabled = settings.RegisterPublicBoolSetting(
 	"set to true to enable CREATE STATISTICS/ANALYZE, false to disable; default is true",
 	featureflag.FeatureFlagEnabledDefault)
 
-func (p *planner) CreateStatistics(ctx context.Context, n *tree.CreateStats) (planNode, error) {
-	if err := featureflag.CheckEnabled(
-		ctx,
-		featureStatsEnabled,
-		&p.ExecCfg().Settings.SV,
-		"ANALYZE/CREATE STATISTICS",
-	); err != nil {
-		return nil, err
-	}
-
-	return &createStatsNode{
-		CreateStats: *n,
-		p:           p,
-	}, nil
-}
-
-// Analyze is syntactic sugar for CreateStatistics.
-func (p *planner) Analyze(ctx context.Context, n *tree.Analyze) (planNode, error) {
-	if err := featureflag.CheckEnabled(
-		ctx,
-		featureStatsEnabled,
-		&p.ExecCfg().Settings.SV,
-		"ANALYZE/CREATE STATISTICS",
-	); err != nil {
-		return nil, err
-	}
-
-	return &createStatsNode{
-		CreateStats: tree.CreateStats{Table: n.Table},
-		p:           p,
-	}, nil
-}
-
 const defaultHistogramBuckets = 200
 const nonIndexColHistogramBuckets = 2
 
@@ -100,6 +67,13 @@ type createStatsNode struct {
 	tree.CreateStats
 	p *planner
 
+	// runAsJob is true by default, and causes the code below to be executed,
+	// which sets up a job and waits for it.
+	//
+	// If it is false, the flow for create statistics is planned directly; this
+	// is used when the statement is under EXPLAIN or EXPLAIN ANALYZE.
+	runAsJob bool
+
 	run createStatsRun
 }
 
@@ -480,20 +454,6 @@ func makeColStatKey(cols []descpb.ColumnID) string {
 	return colSet.String()
 }
 
-// newPlanForExplainDistSQL is part of the distSQLExplainable interface.
-func (n *createStatsNode) newPlanForExplainDistSQL(
-	planCtx *PlanningCtx, distSQLPlanner *DistSQLPlanner,
-) (*PhysicalPlan, error) {
-	// Create a job record but don't actually start the job.
-	record, err := n.makeJobRecord(planCtx.ctx)
-	if err != nil {
-		return nil, err
-	}
-	job := n.p.ExecCfg().JobRegistry.NewJob(*record)
-
-	return distSQLPlanner.createPlanForCreateStats(planCtx, job)
-}
-
 // createStatsResumer implements the jobs.Resumer interface for CreateStats
 // jobs. A new instance is created for each job.
 type createStatsResumer struct {

pkg/sql/create_table.go

@@ -1297,6 +1297,10 @@ func NewTableDesc(
 				n.Defs = append(n.Defs, checkConstraint)
 				columnDefaultExprs = append(columnDefaultExprs, nil)
 			}
+			if d.IsVirtual() {
+				return nil, unimplemented.NewWithIssue(57608, "virtual computed columns")
+			}
+
 			col, idx, expr, err := tabledesc.MakeColumnDefDescs(ctx, d, semaCtx, evalCtx)
 			if err != nil {
 				return nil, err

pkg/sql/distsql_physical_planner.go

@@ -17,6 +17,7 @@ import (
 	"sort"
 
 	"github.com/cockroachdb/cockroach/pkg/gossip"
+	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord"
@@ -525,6 +526,12 @@ func checkSupportForPlanNode(node planNode) (distRecommendation, error) {
 		}
 		return shouldDistribute, nil
 
+	case *createStatsNode:
+		if n.runAsJob {
+			return cannotDistribute, planNodeNotSupportedErr
+		}
+		return shouldDistribute, nil
+
 	default:
 		return cannotDistribute, planNodeNotSupportedErr
 	}
@@ -2697,6 +2704,20 @@ func (dsp *DistSQLPlanner) createPhysPlanForPlanNode(
 	case *zigzagJoinNode:
 		plan, err = dsp.createPlanForZigzagJoin(planCtx, n)
 
+	case *createStatsNode:
+		if n.runAsJob {
+			plan, err = dsp.wrapPlan(planCtx, n)
+		} else {
+			// Create a job record but don't actually start the job.
+			var record *jobs.Record
+			record, err = n.makeJobRecord(planCtx.ctx)
+			if err != nil {
+				return nil, err
+			}
+			job := n.p.ExecCfg().JobRegistry.NewJob(*record)
+			plan, err = dsp.createPlanForCreateStats(planCtx, job)
+		}
+
 	default:
 		// Can't handle a node? We wrap it and continue on our way.
 		plan, err = dsp.wrapPlan(planCtx, n)

pkg/sql/distsql_spec_exec_factory.go

@@ -19,13 +19,15 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/cat"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/constraint"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/exec"
+	"github.com/cockroachdb/cockroach/pkg/sql/opt/exec/explain"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/invertedexpr"
 	"github.com/cockroachdb/cockroach/pkg/sql/physicalplan"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/builtins"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/span"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
+	"github.com/cockroachdb/errors"
 )
 
 type distSQLSpecExecFactory struct {
@@ -741,23 +743,30 @@ func (e *distSQLSpecExecFactory) ConstructExplainOpt(
 }
 
 func (e *distSQLSpecExecFactory) ConstructExplain(
-	options *tree.ExplainOptions, analyze bool, stmtType tree.StatementType, plan exec.Plan,
+	options *tree.ExplainOptions,
+	analyze bool,
+	stmtType tree.StatementType,
+	buildFn exec.BuildPlanForExplainFn,
 ) (exec.Node, error) {
+	if options.Flags[tree.ExplainFlagEnv] {
+		return nil, errors.New("ENV only supported with (OPT) option")
+	}
+	if options.Mode == tree.ExplainPlan {
+		// TODO(radu): all we need to do here is wrap an explainPlanNode.
+		return nil, unimplemented.NewWithIssue(47473, "experimental opt-driven distsql planning: explain plan")
+	}
+
 	// We cannot create the explained plan in the same PlanInfrastructure with the
-	// "outer" plan. Create a new PlanningCtx for the rest of the plan.
-	// TODO(radu): this is a hack and won't work if the result of the explain
-	// feeds into a join or union (on the right-hand side). Move to a model like
-	// ConstructExplainPlan, where we can build the inner plan using a separate
-	// factory instance.
-	planCtxCopy := *e.planCtx
-	planCtxCopy.infra = physicalplan.MakePhysicalInfrastructure(e.planCtx.infra.FlowID, e.planCtx.infra.GatewayNodeID)
-	e.planCtx = &planCtxCopy
+	// "outer" plan. Create a separate factory.
+	explainFactory := explain.NewFactory(newDistSQLSpecExecFactory(e.planner, e.planningMode))
+	plan, err := buildFn(explainFactory)
+	if err != nil {
+		return nil, err
+	}
 
 	// TODO(yuzefovich): make sure to return the same nice error in some
 	// variants of EXPLAIN when subqueries are present as we do in the old path.
-	// TODO(yuzefovich): make sure that local plan nodes that create
-	// distributed jobs are shown as "distributed". See distSQLExplainable.
-	p := plan.(*planComponents)
+	p := plan.(*explain.Plan).WrappedPlan.(*planComponents)
 	explain, err := constructExplainDistSQLOrVecNode(options, analyze, stmtType, p, e.planner)
 	if err != nil {
 		return nil, err
@@ -776,12 +785,6 @@ func (e *distSQLSpecExecFactory) ConstructExplain(
 	return makePlanMaybePhysical(physPlan, []planNode{explainNode}), nil
 }
 
-func (e *distSQLSpecExecFactory) ConstructExplainPlan(
-	options *tree.ExplainOptions, buildFn exec.BuildPlanForExplainFn,
-) (exec.Node, error) {
-	return nil, unimplemented.NewWithIssue(47473, "experimental opt-driven distsql planning: explain plan")
-}
-
 func (e *distSQLSpecExecFactory) ConstructShowTrace(
 	typ tree.ShowTraceType, compact bool,
 ) (exec.Node, error) {
@@ -977,6 +980,12 @@ func (e *distSQLSpecExecFactory) ConstructCancelSessions(
 	return nil, unimplemented.NewWithIssue(47473, "experimental opt-driven distsql planning: cancel sessions")
 }
 
+func (e *distSQLSpecExecFactory) ConstructCreateStatistics(
+	cs *tree.CreateStats,
+) (exec.Node, error) {
+	return nil, unimplemented.NewWithIssue(47473, "experimental opt-driven distsql planning: create statistics")
+}
+
 func (e *distSQLSpecExecFactory) ConstructExport(
 	input exec.Node, fileName tree.TypedExpr, fileFormat string, options []exec.KVOption,
 ) (exec.Node, error) {