Merge #94116 #94342

94116: fetchpb: extract index fetch spec into a separate package r=yuzefovich a=yuzefovich

This will allow us to import the index fetch spec into `roachpb` which
is needed for the KV projection pushdown work.

This required changing the custom cast types used in the spec from type
aliases defined in `descpb` to one level lower ones from `catid`.

Epic: CRDB-14837

Release note: None

94342: optbuilder: disable RANGE window mode with offsets and NULLS LAST r=yuzefovich a=yuzefovich

This commit makes it so that we return a regular error when building the window frame with RANGE mode with offsets when NULLS LAST ordering is requested. The execution engine assumes that exactly one column is included in the ordering for such a frame, and we recently fixed how we handle NULLS LAST (by projecting another column), so at the moment with such queries we encounter a scary-looking internal error, and now we'll get a regular error. Teaching the execution engine about this is not exactly trivial, so for now we'll just prohibit such queries.

Addresses: #94032.

Epic: None

Release note (bug fix): Previously, CockroachDB could encounter an internal error when evaluating window functions with RANGE window frame mode with OFFSET PRECEDING or OFFSET FOLLOWING boundary when ORDER BY clause has NULLS LAST option. This will now result in a regular error since the feature is marked as unsupported.

Co-authored-by: Yahor Yuzefovich <[email protected]>

docs/generated/http/BUILD.bazel

@@ -21,6 +21,7 @@ genrule(
         "//pkg/sql/catalog/catenumpb:catenumpb_proto",
         "//pkg/sql/catalog/catpb:catpb_proto",
         "//pkg/sql/catalog/descpb:descpb_proto",
+        "//pkg/sql/catalog/fetchpb:fetchpb_proto",
         "//pkg/sql/contentionpb:contentionpb_proto",
         "//pkg/sql/execinfrapb:execinfrapb_proto",
         "//pkg/sql/inverted:inverted_proto",

pkg/BUILD.bazel

@@ -1422,6 +1422,7 @@ GO_TARGETS = [
     "//pkg/sql/catalog/descs:descs",
     "//pkg/sql/catalog/descs:descs_test",
     "//pkg/sql/catalog/desctestutils:desctestutils",
+    "//pkg/sql/catalog/fetchpb:fetchpb",
     "//pkg/sql/catalog/funcdesc:funcdesc",
     "//pkg/sql/catalog/funcdesc:funcdesc_test",
     "//pkg/sql/catalog/hydrateddesc:hydrateddesc",
@@ -2692,6 +2693,7 @@ GET_X_DATA_TARGETS = [
     "//pkg/sql/catalog/descpb:get_x_data",
     "//pkg/sql/catalog/descs:get_x_data",
     "//pkg/sql/catalog/desctestutils:get_x_data",
+    "//pkg/sql/catalog/fetchpb:get_x_data",
     "//pkg/sql/catalog/funcdesc:get_x_data",
     "//pkg/sql/catalog/hydrateddesc:get_x_data",
     "//pkg/sql/catalog/ingesting:get_x_data",

pkg/ccl/changefeedccl/cdcevent/BUILD.bazel

@@ -23,6 +23,7 @@ go_library(
         "//pkg/sql/catalog/colinfo",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/descs",
+        "//pkg/sql/catalog/fetchpb",
         "//pkg/sql/catalog/lease",
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",

pkg/ccl/changefeedccl/cdcevent/rowfetcher_cache.go

@@ -18,6 +18,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/fetchpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
 	"github.com/cockroachdb/cockroach/pkg/sql/row"
 	"github.com/cockroachdb/cockroach/pkg/sql/rowenc"
@@ -216,7 +217,7 @@ func (c *rowFetcherCache) RowFetcherForColumnFamily(
 		}
 	}
 
-	var spec descpb.IndexFetchSpec
+	var spec fetchpb.IndexFetchSpec
 
 	var relevantColumns descpb.ColumnIDs
 	if keyOnly {

pkg/gen/protobuf.bzl

@@ -51,6 +51,7 @@ PROTOBUF_SRCS = [
     "//pkg/sql/catalog/catenumpb:catenumpb_go_proto",
     "//pkg/sql/catalog/catpb:catpb_go_proto",
     "//pkg/sql/catalog/descpb:descpb_go_proto",
+    "//pkg/sql/catalog/fetchpb:fetchpb_go_proto",
     "//pkg/sql/catalog/schematelemetry/schematelemetrycontroller:schematelemetrycontroller_go_proto",
     "//pkg/sql/contentionpb:contentionpb_go_proto",
     "//pkg/sql/execinfrapb:execinfrapb_go_proto",

pkg/sql/BUILD.bazel

@@ -337,6 +337,7 @@ go_library(
         "//pkg/sql/catalog/descidgen",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/descs",
+        "//pkg/sql/catalog/fetchpb",
         "//pkg/sql/catalog/funcdesc",
         "//pkg/sql/catalog/lease",
         "//pkg/sql/catalog/multiregion",
@@ -712,6 +713,7 @@ go_test(
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/descs",
         "//pkg/sql/catalog/desctestutils",
+        "//pkg/sql/catalog/fetchpb",
         "//pkg/sql/catalog/funcdesc",
         "//pkg/sql/catalog/lease",
         "//pkg/sql/catalog/multiregion",

pkg/sql/backfill/BUILD.bazel

@@ -19,6 +19,7 @@ go_library(
         "//pkg/sql/catalog",
         "//pkg/sql/catalog/catenumpb",
         "//pkg/sql/catalog/descpb",
+        "//pkg/sql/catalog/fetchpb",
         "//pkg/sql/catalog/schemaexpr",
         "//pkg/sql/catalog/tabledesc",
         "//pkg/sql/catalog/typedesc",

pkg/sql/backfill/backfill.go

@@ -22,6 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/fetchpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfra"
@@ -146,7 +147,7 @@ func (cb *ColumnBackfiller) init(
 	}
 
 	cb.colIdxMap = catalog.ColumnIDToOrdinalMap(desc.PublicColumns())
-	var spec descpb.IndexFetchSpec
+	var spec fetchpb.IndexFetchSpec
 	if err := rowenc.InitIndexFetchSpec(&spec, evalCtx.Codec, desc, desc.GetPrimaryIndex(), cb.fetcherCols); err != nil {
 		return err
 	}
@@ -825,7 +826,7 @@ func (ib *IndexBackfiller) BuildIndexEntriesChunk(
 	// during the scan. Index entries in the new index are being
 	// populated and deleted by the OLTP commands but not otherwise
 	// read or used
-	var spec descpb.IndexFetchSpec
+	var spec fetchpb.IndexFetchSpec
 	if err := rowenc.InitIndexFetchSpec(
 		&spec, ib.evalCtx.Codec, tableDesc, tableDesc.GetPrimaryIndex(), fetcherCols,
 	); err != nil {

pkg/sql/catalog/BUILD.bazel

@@ -31,6 +31,7 @@ go_library(
         "//pkg/sql/catalog/catenumpb",
         "//pkg/sql/catalog/catpb",
         "//pkg/sql/catalog/descpb",
+        "//pkg/sql/catalog/fetchpb",
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/privilege",

pkg/sql/catalog/descpb/BUILD.bazel

@@ -12,7 +12,6 @@ go_library(
         "constraint.go",
         "descriptor.go",
         "index.go",
-        "index_fetch.go",
         "join_type.go",
         "locking.go",
         "structured.go",
@@ -32,7 +31,6 @@ go_library(
         "//pkg/sql/sem/catid",
         "//pkg/sql/sem/tree",
         "//pkg/sql/types",
-        "//pkg/util/encoding",
         "//pkg/util/errorutil/unimplemented",
         "//pkg/util/hlc",
         "//pkg/util/intsets",
@@ -51,7 +49,6 @@ go_test(
 proto_library(
     name = "descpb_proto",
     srcs = [
-        "index_fetch.proto",
         "join_type.proto",
         "locking.proto",
         "structured.proto",

pkg/sql/catalog/descriptor.go

@@ -18,6 +18,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/fetchpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
@@ -539,8 +540,8 @@ type TableDescriptor interface {
 	IndexKeysPerRow(idx Index) int
 
 	// IndexFetchSpecKeyAndSuffixColumns returns information about the key and
-	// suffix columns, suitable for populating a descpb.IndexFetchSpec.
-	IndexFetchSpecKeyAndSuffixColumns(idx Index) []descpb.IndexFetchSpec_KeyColumn
+	// suffix columns, suitable for populating a fetchpb.IndexFetchSpec.
+	IndexFetchSpecKeyAndSuffixColumns(idx Index) []fetchpb.IndexFetchSpec_KeyColumn
 
 	// FindColumnWithID returns the first column found whose ID matches the
 	// provided target ID, in the canonical order.
@@ -590,7 +591,7 @@ type TableDescriptor interface {
 
 	// FamilyDefaultColumns returns the default column IDs for families with a
 	// default column. See IndexFetchSpec.FamilyDefaultColumns.
-	FamilyDefaultColumns() []descpb.IndexFetchSpec_FamilyDefaultColumn
+	FamilyDefaultColumns() []fetchpb.IndexFetchSpec_FamilyDefaultColumn
 
 	// HasColumnBackfillMutation returns whether the table has any queued column
 	// mutations that require a backfill.

pkg/sql/catalog/fetchpb/BUILD.bazel

@@ -0,0 +1,49 @@
+load("//build/bazelutil/unused_checker:unused.bzl", "get_x_data")
+load("@rules_proto//proto:defs.bzl", "proto_library")
+load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "fetchpb",
+    srcs = ["index_fetch.go"],
+    embed = [":fetchpb_go_proto"],
+    importpath = "github.com/cockroachdb/cockroach/pkg/sql/catalog/fetchpb",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//pkg/sql/catalog/catenumpb",
+        "//pkg/sql/sem/catid",  # keep
+        "//pkg/sql/types",
+        "//pkg/util/encoding",
+    ],
+)
+
+proto_library(
+    name = "fetchpb_proto",
+    srcs = [
+        "index_fetch.proto",
+    ],
+    strip_import_prefix = "/pkg",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//pkg/geo/geoindex:geoindex_proto",
+        "//pkg/sql/catalog/catenumpb:catenumpb_proto",
+        "//pkg/sql/types:types_proto",
+        "@com_github_gogo_protobuf//gogoproto:gogo_proto",
+    ],
+)
+
+go_proto_library(
+    name = "fetchpb_go_proto",
+    compilers = ["//pkg/cmd/protoc-gen-gogoroach:protoc-gen-gogoroach_compiler"],
+    importpath = "github.com/cockroachdb/cockroach/pkg/sql/catalog/fetchpb",
+    proto = ":fetchpb_proto",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//pkg/geo/geoindex",
+        "//pkg/sql/catalog/catenumpb",
+        "//pkg/sql/types",
+        "@com_github_gogo_protobuf//gogoproto",
+    ],
+)
+
+get_x_data(name = "get_x_data")

pkg/sql/catalog/descpb/index_fetch.go → pkg/sql/catalog/fetchpb/index_fetch.go

@@ -8,14 +8,20 @@
 // by the Apache License, Version 2.0, included in the file
 // licenses/APL.txt.
 
-package descpb
+package fetchpb
 
 import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/encoding"
 )
 
+// TODO(yuzefovich): consider moving this package somewhere close to rowenc
+// given that effectively using this package in roachpb is saying that rows and
+// column encoding is a shared concept between sql and KV. Furthermore, we could
+// probably make keyside and valueside not depend on sql/catalog and then pull
+// them out of the sql tree altogether.
+
 // IndexFetchSpecVersionInitial is the initial IndexFetchSpec version.
 const IndexFetchSpecVersionInitial = 1
 

pkg/sql/catalog/descpb/index_fetch.proto → pkg/sql/catalog/fetchpb/index_fetch.proto

@@ -11,7 +11,7 @@
 // Cannot be proto3 because we use nullable primitives.
 syntax = "proto2";
 package cockroach.sql.sqlbase;
-option go_package = "descpb";
+option go_package = "fetchpb";
 
 import "gogoproto/gogo.proto";
 import "sql/types/types.proto";
@@ -24,7 +24,7 @@ message IndexFetchSpec {
   message Column {
     optional uint32 column_id = 1 [(gogoproto.nullable) = false,
                                    (gogoproto.customname) = "ColumnID",
-                                   (gogoproto.casttype) = "ColumnID"];
+                                   (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ColumnID"];
 
     // Name of the column, as it is expected to appear in debug and error
     // messages.
@@ -60,13 +60,13 @@ message IndexFetchSpec {
   message FamilyDefaultColumn {
     optional uint32 family_id = 1 [(gogoproto.nullable) = false,
                                    (gogoproto.customname) = "FamilyID",
-                                   (gogoproto.casttype) = "FamilyID"];
+                                   (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.FamilyID"];
 
     // DefaultColumnID is the implicit column ID when a single value is encoded
     // for the family. See ColumnFamilyDescriptor.DefaultColumnID.
     optional uint32 default_column_id = 2 [(gogoproto.nullable) = false,
                                            (gogoproto.customname) = "DefaultColumnID",
-                                           (gogoproto.casttype) = "ColumnID"];
+                                           (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ColumnID"];
   }
 
   // Version is used to allow providing backward compatibility if this spec
@@ -76,12 +76,12 @@ message IndexFetchSpec {
 
   optional uint32 table_id = 2 [(gogoproto.nullable) = false,
                                 (gogoproto.customname) = "TableID",
-                                (gogoproto.casttype) = "ID"];
+                                (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.DescID"];
   optional string table_name = 3 [(gogoproto.nullable) = false];
 
   optional uint32 index_id = 4 [(gogoproto.nullable) = false,
                                 (gogoproto.customname) = "IndexID",
-                                (gogoproto.casttype) = "IndexID"];
+                                (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.IndexID"];
 
   optional string index_name = 5 [(gogoproto.nullable) = false];
 
@@ -116,7 +116,7 @@ message IndexFetchSpec {
 
   optional uint32 max_family_id = 12 [(gogoproto.nullable) = false,
                                       (gogoproto.customname) = "MaxFamilyID",
-                                      (gogoproto.casttype) = "FamilyID"];
+                                      (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.FamilyID"];
 
   // FamilyDefaultColumns contains the default column IDs for families with a
   // default column. This is used to decode values that use the single column

pkg/sql/catalog/tabledesc/BUILD.bazel

@@ -32,6 +32,7 @@ go_library(
         "//pkg/sql/catalog/catprivilege",
         "//pkg/sql/catalog/colinfo",
         "//pkg/sql/catalog/descpb",
+        "//pkg/sql/catalog/fetchpb",
         "//pkg/sql/catalog/internal/validate",
         "//pkg/sql/catalog/multiregion",
         "//pkg/sql/catalog/schemaexpr",

pkg/sql/catalog/tabledesc/column.go

@@ -18,6 +18,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/fetchpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -296,7 +297,7 @@ type columnCache struct {
 	readable             []catalog.Column
 	withUDTs             []catalog.Column
 	system               []catalog.Column
-	familyDefaultColumns []descpb.IndexFetchSpec_FamilyDefaultColumn
+	familyDefaultColumns []fetchpb.IndexFetchSpec_FamilyDefaultColumn
 	index                []indexColumnCache
 }
 
@@ -309,7 +310,7 @@ type indexColumnCache struct {
 	keySuffix    []catalog.Column
 	full         []catalog.Column
 	fullDirs     []catenumpb.IndexColumn_Direction
-	keyAndSuffix []descpb.IndexFetchSpec_KeyColumn
+	keyAndSuffix []fetchpb.IndexFetchSpec_KeyColumn
 }
 
 // newColumnCache returns a fresh fully-populated columnCache struct for the
@@ -378,9 +379,9 @@ func newColumnCache(desc *descpb.TableDescriptor, mutations *mutationCache) *col
 	for i := range desc.Families {
 		if f := &desc.Families[i]; f.DefaultColumnID != 0 {
 			if c.familyDefaultColumns == nil {
-				c.familyDefaultColumns = make([]descpb.IndexFetchSpec_FamilyDefaultColumn, 0, len(desc.Families)-i)
+				c.familyDefaultColumns = make([]fetchpb.IndexFetchSpec_FamilyDefaultColumn, 0, len(desc.Families)-i)
 			}
-			c.familyDefaultColumns = append(c.familyDefaultColumns, descpb.IndexFetchSpec_FamilyDefaultColumn{
+			c.familyDefaultColumns = append(c.familyDefaultColumns, fetchpb.IndexFetchSpec_FamilyDefaultColumn{
 				FamilyID:        f.ID,
 				DefaultColumnID: f.DefaultColumnID,
 			})
@@ -436,7 +437,7 @@ func makeIndexColumnCache(idx *descpb.IndexDescriptor, all []catalog.Column) (ic
 	for _, colID := range idx.CompositeColumnIDs {
 		compositeIDs.Add(colID)
 	}
-	ic.keyAndSuffix = make([]descpb.IndexFetchSpec_KeyColumn, nKey+nKeySuffix)
+	ic.keyAndSuffix = make([]fetchpb.IndexFetchSpec_KeyColumn, nKey+nKeySuffix)
 	for i := range ic.keyAndSuffix {
 		col := ic.all[i]
 		if col == nil {
@@ -448,8 +449,8 @@ func makeIndexColumnCache(idx *descpb.IndexDescriptor, all []catalog.Column) (ic
 		if colID != 0 && colID == invertedColumnID {
 			typ = idx.InvertedColumnKeyType()
 		}
-		ic.keyAndSuffix[i] = descpb.IndexFetchSpec_KeyColumn{
-			IndexFetchSpec_Column: descpb.IndexFetchSpec_Column{
+		ic.keyAndSuffix[i] = fetchpb.IndexFetchSpec_KeyColumn{
+			IndexFetchSpec_Column: fetchpb.IndexFetchSpec_Column{
 				Name:          col.GetName(),
 				ColumnID:      colID,
 				Type:          typ,

pkg/sql/catalog/tabledesc/table_desc.go

@@ -17,6 +17,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/fetchpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
@@ -476,7 +477,7 @@ func (desc *wrapper) SystemColumns() []catalog.Column {
 }
 
 // FamilyDefaultColumns implements the TableDescriptor interface.
-func (desc *wrapper) FamilyDefaultColumns() []descpb.IndexFetchSpec_FamilyDefaultColumn {
+func (desc *wrapper) FamilyDefaultColumns() []fetchpb.IndexFetchSpec_FamilyDefaultColumn {
 	return desc.getExistingOrNewColumnCache().familyDefaultColumns
 }
 
@@ -605,7 +606,7 @@ func (desc *wrapper) UniqueWithoutIndexColumns(
 // IndexFetchSpecKeyAndSuffixColumns implements the TableDescriptor interface.
 func (desc *wrapper) IndexFetchSpecKeyAndSuffixColumns(
 	idx catalog.Index,
-) []descpb.IndexFetchSpec_KeyColumn {
+) []fetchpb.IndexFetchSpec_KeyColumn {
 	if ic := desc.getExistingOrNewIndexColumnCache(idx); ic != nil {
 		return ic.keyAndSuffix
 	}

pkg/sql/colencoding/BUILD.bazel

@@ -13,7 +13,7 @@ go_library(
         "//pkg/col/coldata",
         "//pkg/roachpb",
         "//pkg/sql/catalog/catenumpb",
-        "//pkg/sql/catalog/descpb",
+        "//pkg/sql/catalog/fetchpb",
         "//pkg/sql/rowenc/keyside",
         "//pkg/sql/rowenc/valueside",
         "//pkg/sql/sem/tree",