sql/stats: handle range counts when skipping dropped enum hist buckets

In #136538 we changed DecodeHistogramBuckets to skip over histogram
buckets for dropped enum values. This change assumed that histogram
buckets for enum types would never contain range counts, but this is not
true. For example, if the number of distinct enum values exceeds the
number of histogram buckets there could be range counts.

Fixes: #154461

Release note (bug fix): Fix a bug in which range counts in table
statistics histograms were not handled correctly after a user-defined
enum type was modified.

Author: michae2

pkg/sql/logictest/testdata/logic_test/enums

@@ -1987,3 +1987,132 @@ CREATE TABLE public.t (
   rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
   CONSTRAINT t_pkey PRIMARY KEY (rowid ASC)
 ) WITH (schema_locked = true);
+
+# Testcase for issue 154461: drop an enum value when the histogram contains
+# range counts.
+subtest 154461
+
+statement ok
+USE test
+
+statement ok
+CREATE TYPE e154461 AS ENUM ('e', 'f', 'g')
+
+statement ok
+CREATE TABLE t154461 (a e154461, INDEX (a)) WITH (sql_stats_histogram_buckets_count = 2)
+
+statement ok
+INSERT INTO t154461 VALUES ('e'), ('e'), ('f'), ('g'), ('g')
+
+statement ok
+CREATE STATISTICS s FROM t154461
+
+query T
+SELECT * FROM t154461 WHERE a != 'g' ORDER BY a
+----
+e
+e
+f
+
+let $hist_id_1
+SELECT histogram_id FROM [SHOW STATISTICS FOR TABLE t154461]
+WHERE statistics_name = 's' AND column_names = '{a}'
+
+query TIRI colnames,nosort
+SHOW HISTOGRAM $hist_id_1
+----
+upper_bound  range_rows  distinct_range_rows  equal_rows
+'e'          0           0                    2
+'g'          1           1                    2
+
+query T
+SELECT jsonb_pretty(stat)
+FROM (
+  SELECT json_array_elements(statistics) - 'created_at' - 'id' - 'avg_size' AS stat
+  FROM [SHOW STATISTICS USING JSON FOR TABLE t154461]
+)
+WHERE stat->>'columns' = '["a"]'
+----
+{
+    "columns": [
+        "a"
+    ],
+    "distinct_count": 3,
+    "histo_buckets": [
+        {
+            "distinct_range": 0,
+            "num_eq": 2,
+            "num_range": 0,
+            "upper_bound": "e"
+        },
+        {
+            "distinct_range": 1,
+            "num_eq": 2,
+            "num_range": 1,
+            "upper_bound": "g"
+        }
+    ],
+    "histo_col_type": "test.public.e154461",
+    "histo_version": 3,
+    "name": "s",
+    "null_count": 0,
+    "row_count": 5
+}
+
+# Now drop the enum value that is the first bucket in the 2-bucket histogram.
+
+statement ok
+DELETE FROM t154461 WHERE a = 'e'
+
+statement ok
+ALTER TYPE e154461 DROP VALUE 'e'
+
+query T
+SELECT * FROM t154461 WHERE a != 'g' ORDER BY a
+----
+f
+
+let $hist_id_1
+SELECT histogram_id FROM [SHOW STATISTICS FOR TABLE t154461]
+WHERE statistics_name = 's' AND column_names = '{a}'
+
+query TIRI colnames,nosort
+SHOW HISTOGRAM $hist_id_1
+----
+upper_bound  range_rows  distinct_range_rows  equal_rows
+'f'          0           0                    1
+'g'          0           0                    2
+
+query T
+SELECT jsonb_pretty(stat)
+FROM (
+  SELECT json_array_elements(statistics) - 'created_at' - 'id' - 'avg_size' AS stat
+  FROM [SHOW STATISTICS USING JSON FOR TABLE t154461]
+)
+WHERE stat->>'columns' = '["a"]'
+----
+{
+    "columns": [
+        "a"
+    ],
+    "distinct_count": 2,
+    "histo_buckets": [
+        {
+            "distinct_range": 0,
+            "num_eq": 1,
+            "num_range": 0,
+            "upper_bound": "f"
+        },
+        {
+            "distinct_range": 0,
+            "num_eq": 2,
+            "num_range": 0,
+            "upper_bound": "g"
+        }
+    ],
+    "histo_col_type": "test.public.e154461",
+    "histo_version": 3,
+    "name": "s",
+    "null_count": 0,
+    "row_count": 5
+}

pkg/sql/opt/exec/execbuilder/testdata/forecast

@@ -396,23 +396,11 @@ WHERE stat->>'name' = '__forecast__'
     "created_at": "1988-08-08 00:00:00",
     "distinct_count": 1,
     "histo_buckets": [
-        {
-            "distinct_range": 0,
-            "num_eq": 0,
-            "num_range": 0,
-            "upper_bound": "-1"
-        },
         {
             "distinct_range": 0,
             "num_eq": 1,
             "num_range": 0,
             "upper_bound": "0"
-        },
-        {
-            "distinct_range": 0,
-            "num_eq": 0,
-            "num_range": 0,
-            "upper_bound": "1"
         }
     ],
     "histo_col_type": "INT8",

pkg/sql/opt/exec/execbuilder/testdata/partial_stats

@@ -375,12 +375,6 @@ WHERE stat->>'name' = '__forecast__';
             "num_range": 0,
             "upper_bound": "9"
         },
-        {
-            "distinct_range": 0,
-            "num_eq": 0,
-            "num_range": 0,
-            "upper_bound": "10"
-        },
         {
             "distinct_range": 0,
             "num_eq": 2,
@@ -411,12 +405,6 @@ WHERE stat->>'name' = '__forecast__';
             "num_range": 0,
             "upper_bound": "15"
         },
-        {
-            "distinct_range": 0,
-            "num_eq": 0,
-            "num_range": 0,
-            "upper_bound": "16"
-        },
         {
             "distinct_range": 0,
             "num_eq": 1,

pkg/sql/randgen/mutator.go

@@ -226,7 +226,7 @@ func statisticsMutator(
 			colType := tree.MustBeStaticallyKnownType(col.Type)
 			h := randHistogram(rng, colType)
 			statIdx := colNameToStatIdx[col.Name]
-			if err := allStats[statIdx].SetHistogram(&h); err != nil {
+			if err := allStats[statIdx].SetHistogram(context.Background(), &h); err != nil {
 				panic(err)
 			}
 		}

pkg/sql/show_histogram.go

@@ -90,20 +90,17 @@ func (p *planner) ShowHistogram(ctx context.Context, n *tree.ShowHistogram) (pla
 			if err := typedesc.EnsureTypeIsHydrated(ctx, histogram.ColumnType, &resolver); err != nil {
 				return nil, err
 			}
-			var a tree.DatumAlloc
-			for _, b := range histogram.Buckets {
-				var upperBound string
-				datum, err := stats.DecodeUpperBound(histogram.Version, histogram.ColumnType, &a, b.UpperBound)
-				if err != nil {
-					upperBound = fmt.Sprintf("<error: %v>", err)
-				} else {
-					upperBound = datum.String()
-				}
+			decodedBuckets, _, err := histogram.DecodeBuckets(ctx)
+			if err != nil {
+				return nil, err
+			}
+			for _, b := range decodedBuckets {
+				upperBound := b.UpperBound.String()
 				row := tree.Datums{
 					tree.NewDString(upperBound),
-					tree.NewDInt(tree.DInt(b.NumRange)),
+					tree.NewDInt(tree.DInt(int64(b.NumRange))),
 					tree.NewDFloat(tree.DFloat(b.DistinctRange)),
-					tree.NewDInt(tree.DInt(b.NumEq)),
+					tree.NewDInt(tree.DInt(int64(b.NumEq))),
 				}
 				if _, err := v.rows.AddRow(ctx, row); err != nil {
 					v.Close(ctx)

pkg/sql/show_stats.go

@@ -182,7 +182,7 @@ func (p *planner) ShowTableStats(ctx context.Context, n *tree.ShowTableStats) (p
 					}
 					obs := &stats.TableStatistic{TableStatisticProto: *stat}
 					if obs.HistogramData != nil && !obs.HistogramData.ColumnType.UserDefined() {
-						if err := stats.DecodeHistogramBuckets(obs); err != nil {
+						if err := stats.DecodeHistogramBuckets(ctx, obs); err != nil {
 							return nil, err
 						}
 					}

pkg/sql/stats/histogram.go

@@ -140,6 +140,37 @@ func DecodeUpperBound(
 	return datum, err
 }
 
+// enumValueExistsBetweenEncodedUpperBounds finds whether any values exist in
+// the enum type with physical representation greater than lowerBound and less
+// than upperBound, exclusive. The encoded bounds could represent old values
+// that were dropped from the enum type, or current values that exist in the
+// enum type.
+//
+// If there are any values in the enum type with physical representation between
+// the bounds, nil is returned. Otherwise an error is returned. If the bounds
+// cannot be decoded as enum physical representations an error is returned.
+func enumValueExistsBetweenEncodedUpperBounds(
+	version HistogramVersion, enumType *types.T, lowerBound, upperBound []byte,
+) error {
+	// If the encoded bounds represent old values that were dropped from the enum
+	// type, we won't be able to decode them as enums. Instead, decode them as
+	// bytes, which should give the physical representation. (This relies on enum
+	// encoding being simply the encoding of the physical representation bytes.)
+	var a tree.DatumAlloc
+	lowerPhys, err := DecodeUpperBound(version, types.Bytes, &a, lowerBound)
+	if err != nil {
+		return err
+	}
+	upperPhys, err := DecodeUpperBound(version, types.Bytes, &a, upperBound)
+	if err != nil {
+		return err
+	}
+	_, err = enumType.EnumGetFirstIdxOfPhysicalBetween(
+		[]byte(*lowerPhys.(*tree.DBytes)), []byte(*upperPhys.(*tree.DBytes)),
+	)
+	return err
+}
+
 // GetDefaultHistogramBuckets gets the default number of histogram buckets to
 // create for the given table.
 func GetDefaultHistogramBuckets(sv *settings.Values, desc catalog.TableDescriptor) uint32 {

pkg/sql/stats/histogram_test.go

@@ -1265,6 +1265,7 @@ func TestUpperBoundsRoundTrip(t *testing.T) {
 	h.buckets = make([]cat.HistogramBucket, numBuckets)
 	for i := 0; i < numBuckets; i++ {
 		h.buckets[i].UpperBound = upperBounds[i]
+		h.buckets[i].NumEq = 1
 	}
 	hd, err := h.toHistogramData(context.Background(), typ, st)
 	if err != nil {
@@ -1274,7 +1275,7 @@ func TestUpperBoundsRoundTrip(t *testing.T) {
 	// original ones.
 	var stat TableStatistic
 	stat.HistogramData = &hd
-	if err = DecodeHistogramBuckets(&stat); err != nil {
+	if err = DecodeHistogramBuckets(context.Background(), &stat); err != nil {
 		t.Fatal(err)
 	}
 	evalCtx := eval.MakeTestingEvalContext(st)

pkg/sql/stats/json.go

@@ -8,6 +8,7 @@ package stats
 import (
 	"context"
 	"fmt"
+	"math"
 
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
@@ -55,7 +56,7 @@ type JSONHistoBucket struct {
 }
 
 // SetHistogram fills in the HistogramColumnType and HistogramBuckets fields.
-func (js *JSONStatistic) SetHistogram(h *HistogramData) error {
+func (js *JSONStatistic) SetHistogram(ctx context.Context, h *HistogramData) error {
 	typ := h.ColumnType
 	if typ == nil {
 		return fmt.Errorf("histogram type is unset")
@@ -66,26 +67,19 @@ func (js *JSONStatistic) SetHistogram(h *HistogramData) error {
 	js.HistogramColumnType = typ.SQLStringFullyQualified()
 	js.HistogramBuckets = make([]JSONHistoBucket, 0, len(h.Buckets))
 	js.HistogramVersion = h.Version
-	var a tree.DatumAlloc
-	for i := range h.Buckets {
-		b := &h.Buckets[i]
-		if b.UpperBound == nil {
-			return fmt.Errorf("histogram bucket upper bound is unset")
-		}
-		datum, err := DecodeUpperBound(h.Version, typ, &a, b.UpperBound)
-		if err != nil {
-			if h.ColumnType.Family() == types.EnumFamily && errors.Is(err, types.EnumValueNotFound) {
-				// Skip over buckets for enum values that were dropped.
-				continue
-			}
-			return err
-		}
 
+	decodedBuckets, distinctAdjustment, err := h.DecodeBuckets(ctx)
+	if err != nil {
+		return err
+	}
+	js.DistinctCount = uint64(math.Max(0, float64(js.DistinctCount)+distinctAdjustment))
+	for i := range decodedBuckets {
+		b := &decodedBuckets[i]
 		js.HistogramBuckets = append(js.HistogramBuckets, JSONHistoBucket{
-			NumEq:         b.NumEq,
-			NumRange:      b.NumRange,
+			NumEq:         int64(b.NumEq),
+			NumRange:      int64(b.NumRange),
 			DistinctRange: b.DistinctRange,
-			UpperBound:    tree.AsStringWithFlags(datum, tree.FmtExport|tree.FmtAlwaysQualifyUserDefinedTypeNames),
+			UpperBound:    tree.AsStringWithFlags(b.UpperBound, tree.FmtExport|tree.FmtAlwaysQualifyUserDefinedTypeNames),
 		})
 	}
 	return nil
@@ -123,7 +117,7 @@ func (js *JSONStatistic) DecodeAndSetHistogram(
 		}
 		h.ColumnType = typ
 	}
-	return js.SetHistogram(h)
+	return js.SetHistogram(ctx, h)
 }
 
 // GetHistogram converts the json histogram into HistogramData.