sql/stats: miscellaneous minor cleanup

This commit contains a bunch of minor cleanups I noticed while reading
the stats related code:
- remove no longer used `ImportStatsName`
- remove redundant disabling of auto stats in logic tests (the framework
itself does it already)
- fix the pre-allocation slice for out types
- update TODOs with Faizaan's name to the corresponding issue numbers
- wrap some comments, remove a couple stale / redundant ones, collapse
some lines.

Release note: None

Author: yuzefovich

pkg/jobs/jobspb/wrap.go

@@ -151,10 +151,6 @@ const AutoStatsName = "__auto__"
 // automatically.
 const AutoPartialStatsName = "__auto_partial__"
 
-// ImportStatsName is the name to use for statistics created automatically
-// during import.
-const ImportStatsName = "__import__"
-
 // ForecastStatsName is the name to use for statistic forecasts.
 const ForecastStatsName = "__forecast__"
 

pkg/sql/create_stats.go

@@ -294,7 +294,7 @@ func (n *createStatsNode) makeJobRecord(ctx context.Context) (*jobs.Record, erro
 		virtColEnabled := statsOnVirtualCols.Get(n.p.ExecCfg().SV())
 		// Disable multi-column stats and deleting stats if partial statistics at
 		// the extremes are requested.
-		// TODO(faizaanmadhani): Add support for multi-column stats.
+		// TODO(#94076): add support for creating multi-column stats.
 		var multiColEnabled bool
 		if !n.Options.UsingExtremes {
 			multiColEnabled = stats.MultiColumnStatisticsClusterMode.Get(n.p.ExecCfg().SV())

pkg/sql/distsql_plan_stats.go

@@ -137,14 +137,10 @@ func computeNumberSamples(ctx context.Context, numRows uint64, st *cluster.Setti
 		minSampleSize = minAutoHistogramSamples.Default()
 	}
 
-	numSamples := math.Max(
-		math.Min(
-			582.0*math.Pow(float64(numRows), 0.29),
-			float64(maxSampleSize),
-		),
+	return uint32(max(
+		min(582.0*math.Pow(float64(numRows), 0.29), float64(maxSampleSize)),
 		float64(minSampleSize),
-	)
-	return uint32(numSamples)
+	))
 }
 
 func (dsp *DistSQLPlanner) createAndAttachSamplers(
@@ -167,7 +163,6 @@ func (dsp *DistSQLPlanner) createAndAttachSamplers(
 		if autoStatsFractionStaleRowsForTable, ok := desc.AutoStatsFractionStaleRows(); ok {
 			overhead = autoStatsFractionStaleRowsForTable
 		}
-		// Convert to a signed integer first to make the linter happy.
 		if details.UsingExtremes {
 			rowsExpected = uint64(int64(
 				// The total expected number of rows is the estimated number of stale
@@ -188,23 +183,19 @@ func (dsp *DistSQLPlanner) createAndAttachSamplers(
 	sampler := &execinfrapb.SamplerSpec{
 		Sketches:         sketchSpec,
 		InvertedSketches: invSketchSpec,
+		MaxFractionIdle:  details.MaxFractionIdle,
 	}
-	sampler.MaxFractionIdle = details.MaxFractionIdle
-	// For partial statistics this loop should only iterate once
-	// since we only support one reqStat at a time.
+	// For partial statistics this loop should only iterate once since we only
+	// support one reqStat at a time.
 	for _, s := range reqStats {
 		if s.histogram {
 			var histogramSamplesCount uint32
 			if tableSampleCount, ok := desc.HistogramSamplesCount(); ok {
 				histogramSamplesCount = tableSampleCount
-			} else if clusterSampleCount := histogramSamples.Get(&dsp.st.SV); clusterSampleCount != histogramSamples.Default() {
+			} else if clusterSampleCount := histogramSamples.Get(&dsp.st.SV); clusterSampleCount != 0 {
 				histogramSamplesCount = uint32(clusterSampleCount)
 			} else {
-				histogramSamplesCount = computeNumberSamples(
-					ctx,
-					rowsExpected,
-					dsp.st,
-				)
+				histogramSamplesCount = computeNumberSamples(ctx, rowsExpected, dsp.st)
 				log.Dev.Infof(ctx, "using computed sample size of %d for histogram construction", histogramSamplesCount)
 			}
 			sampler.SampleSize = histogramSamplesCount
@@ -219,7 +210,7 @@ func (dsp *DistSQLPlanner) createAndAttachSamplers(
 
 	// The sampler outputs the original columns plus a rank column, five
 	// sketch columns, and two inverted histogram columns.
-	outTypes := make([]*types.T, 0, len(p.GetResultTypes())+5)
+	outTypes := make([]*types.T, 0, len(p.GetResultTypes())+8)
 	outTypes = append(outTypes, p.GetResultTypes()...)
 	// An INT column for the rank of each row.
 	outTypes = append(outTypes, types.Int)
@@ -292,18 +283,15 @@ func (dsp *DistSQLPlanner) createPartialStatsPlan(
 	// so we only support one requested stat at a time here.
 	if len(reqStats) > 1 {
 		return nil, unimplemented.NewWithIssue(
-			128904,
-			"cannot process multiple partial statistics requests at once",
+			128904, "cannot process multiple partial statistics requests at once",
 		)
 	}
 
 	reqStat := reqStats[0]
-
 	if len(reqStat.columns) > 1 {
-		// TODO (faizaanmadhani): Add support for creating multi-column stats
+		// TODO(#94076): add support for creating multi-column stats.
 		return nil, pgerror.Newf(pgcode.FeatureNotSupported, "multi-column partial statistics are not currently supported")
 	}
-
 	if !reqStat.histogram {
 		return nil, pgerror.Newf(pgcode.FeatureNotSupported, "partial statistics without histograms are not supported")
 	}
@@ -324,9 +312,9 @@ func (dsp *DistSQLPlanner) createPartialStatsPlan(
 		return nil, err
 	}
 
-	// Calculate the column we need to scan
-	// TODO (faizaanmadhani): Iterate through all columns in a requested stat when
-	// when we add support for multi-column statistics.
+	// Calculate the column we need to scan.
+	// TODO(#94076): iterate through all columns in a requested stat when we add
+	// support for multi-column statistics.
 	var colCfg scanColumnsConfig
 	colCfg.wantedColumns = append(colCfg.wantedColumns, column.GetID())
 
@@ -341,19 +329,18 @@ func (dsp *DistSQLPlanner) createPartialStatsPlan(
 	if err != nil {
 		return nil, err
 	}
-	// Map the ColumnIDs to their ordinals in scan.cols
-	// This loop should only iterate once, since we only
-	// handle single column partial statistics.
-	// TODO(faizaanmadhani): Add support for multi-column partial stats next
+	// Map the ColumnIDs to their ordinals in scan.cols. This loop should only
+	// iterate once, since we only handle single column partial statistics.
+	// TODO(#94076): add support for creating multi-column stats.
 	var colIdxMap catalog.TableColMap
 	for i, c := range scan.catalogCols {
 		colIdxMap.Set(c.GetID(), i)
 	}
 
 	var stat *stats.TableStatistic
 	// Find the statistic from the newest table statistic for our column that is
-	// not partial and not forecasted. The first one we find will be the latest
-	// due to the newest to oldest ordering property of the cache.
+	// not partial, not merged, and not forecasted. The first one we find will
+	// be the latest due to the newest to oldest ordering property of the cache.
 	for _, t := range tableStats {
 		if len(t.ColumnIDs) == 1 && column.GetID() == t.ColumnIDs[0] &&
 			!t.IsPartial() && !t.IsMerged() && !t.IsForecast() {
@@ -380,7 +367,8 @@ func (dsp *DistSQLPlanner) createPartialStatsPlan(
 		return nil, pgerror.Newf(
 			pgcode.ObjectNotInPrerequisiteState,
 			"column %s does not have a prior statistic",
-			column.GetName())
+			column.GetName(),
+		)
 	}
 	if len(stat.Histogram) == 1 && stat.Histogram[0].UpperBound == tree.DNull {
 		return nil, pgerror.Newf(
@@ -398,8 +386,9 @@ func (dsp *DistSQLPlanner) createPartialStatsPlan(
 			planCtx.EvalContext(), planCtx.ExtendedEvalCtx.Codec, desc, scan.index,
 		)
 
-		lowerBound, upperBound, err := bounds.GetUsingExtremesBounds(ctx,
-			planCtx.EvalContext(), stat.Histogram)
+		lowerBound, upperBound, err := bounds.GetUsingExtremesBounds(
+			ctx, planCtx.EvalContext(), stat.Histogram,
+		)
 		if err != nil {
 			return nil, err
 		}
@@ -413,13 +402,13 @@ func (dsp *DistSQLPlanner) createPartialStatsPlan(
 		}
 		prevLowerBound = lowerBound
 
-		extremesSpans, err := bounds.ConstructUsingExtremesSpans(lowerBound,
-			upperBound, scan.index)
+		extremesSpans, err := bounds.ConstructUsingExtremesSpans(
+			lowerBound, upperBound, scan.index,
+		)
 		if err != nil {
 			return nil, err
 		}
 		predicate = bounds.ConstructUsingExtremesPredicate(lowerBound, upperBound, column.GetName())
-		// Get roachpb.Spans from constraint.Spans
 		scan.spans, err = sb.SpansFromConstraintSpan(&extremesSpans, span.NoopSplitter())
 		if err != nil {
 			return nil, err
@@ -456,9 +445,9 @@ func (dsp *DistSQLPlanner) createPartialStatsPlan(
 		spec.FullStatisticID = stat.StatisticID
 	}
 
-	// For now, this loop should iterate only once, as we only
-	// handle single-column partial statistics.
-	// TODO(faizaanmadhani): Add support for multi-column partial stats next
+	// For now, this loop should iterate only once, as we only handle
+	// single-column partial statistics.
+	// TODO(#94076): add support for creating multi-column stats.
 	for i, colID := range reqStat.columns {
 		colIdx, ok := colIdxMap.Get(colID)
 		if !ok {
@@ -497,16 +486,9 @@ func (dsp *DistSQLPlanner) createPartialStatsPlan(
 		sketchSpec = append(sketchSpec, spec)
 	}
 	return dsp.createAndAttachSamplers(
-		ctx,
-		p,
-		desc,
-		tableStats,
-		details,
-		sampledColumnIDs,
-		jobID,
-		reqStats,
-		sketchSpec, invSketchSpec,
-		numIndexes, curIndex), nil
+		ctx, p, desc, tableStats, details, sampledColumnIDs, jobID, reqStats,
+		sketchSpec, invSketchSpec, numIndexes, curIndex,
+	), nil
 }
 
 func (dsp *DistSQLPlanner) createStatsPlan(
@@ -748,16 +730,9 @@ func (dsp *DistSQLPlanner) createStatsPlan(
 	}
 
 	return dsp.createAndAttachSamplers(
-		ctx,
-		p,
-		desc,
-		tableStats,
-		details,
-		sampledColumnIDs,
-		jobID,
-		reqStats,
-		sketchSpecs, invSketchSpecs,
-		numIndexes, curIndex), nil
+		ctx, p, desc, tableStats, details, sampledColumnIDs, jobID, reqStats,
+		sketchSpecs, invSketchSpecs, numIndexes, curIndex,
+	), nil
 }
 
 // createPlanForCreateStats creates the DistSQL plan to perform the table stats

pkg/sql/logictest/testdata/logic_test/constrained_stats

@@ -2,9 +2,6 @@
 
 # Tests for creating partial table statistics with a WHERE clause.
 
-statement ok
-SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false
-
 statement ok
 CREATE TABLE products (
     id INT PRIMARY KEY,

pkg/sql/logictest/testdata/logic_test/distsql_automatic_partial_stats

@@ -2,10 +2,6 @@
 
 # Test a simple update and insert case for partial statistics
 
-# Disable automatic stats
-statement ok
-SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false
-
 statement ok
 SET CLUSTER SETTING sql.stats.automatic_partial_collection.min_stale_rows = 5
 

pkg/sql/opt/optbuilder/misc_statements.go

@@ -178,11 +178,9 @@ func (b *Builder) buildCreateStatistics(n *tree.CreateStats, inScope *scope) (ou
 		tabMeta := b.addTable(t, &tn)
 		tabID = tabMeta.MetaID
 	} else if _, ok = ds.(cat.View); ok {
-		panic(pgerror.New(
-			pgcode.WrongObjectType, "cannot create statistics on views"))
+		panic(pgerror.New(pgcode.WrongObjectType, "cannot create statistics on views"))
 	} else {
-		panic(pgerror.Newf(pgcode.WrongObjectType,
-			"cannot create statistics on %T", ds))
+		panic(pgerror.Newf(pgcode.WrongObjectType, "cannot create statistics on %T", ds))
 	}
 
 	indexOrd := cat.PrimaryIndex

pkg/sql/stats/forecast.go

@@ -292,7 +292,7 @@ func forecastColumnStatistics(
 	forecast = &TableStatistic{
 		TableStatisticProto: TableStatisticProto{
 			TableID:       tableID,
-			StatisticID:   0, // TODO(michae2): Add support for SHOW HISTOGRAM.
+			StatisticID:   0, // TODO(#86358): add support for SHOW HISTOGRAM.
 			Name:          jobspb.ForecastStatsName,
 			ColumnIDs:     columnIDs,
 			CreatedAt:     at,

pkg/sql/stats/merge.go

@@ -259,7 +259,7 @@ func mergePartialStatistic(
 	mergedTableStatistic := &TableStatistic{
 		TableStatisticProto: TableStatisticProto{
 			TableID:         fullStat.TableID,
-			StatisticID:     0, // TODO (faizaanmadhani): Add support for SHOW HISTOGRAM.
+			StatisticID:     0, // TODO(#86358): add support for SHOW HISTOGRAM.
 			Name:            jobspb.MergedStatsName,
 			ColumnIDs:       fullStat.ColumnIDs,
 			CreatedAt:       partialStat.CreatedAt,