Merge #77875 #77878 #77968 #77995 #78013 #78023

77875: sql: use IndexFetchSpec for inverted joiner r=RaduBerinde a=RaduBerinde

#### rowexec: address TODO in inverted joiner

This commit addresses a TODO in the inverted joiner. We now reuse a
buffer for encoding the prefix key. In the general case we still need
to make a copy since we are storing multiple prefix keys; but now we
try to reuse the last copy if the prefix is the same.

We also remove the temporary use of `indexRow` which is confusing here
(it wasn't obvious at first but this code has nothing to do with index
rows and is just using `indexRow` as a temporary buffer).

Release note: None

#### geoindex: minor cleanup around Config

This change changes `IsEmptyConfig`, `IsGeographyConfig`,
`IsGeometryConfig` to methods on the `geoindex.Config` type.

We also switch to passing configs by value which is cleaner and avoids
some allocations.

Release note: None

#### sql: use IndexFetchSpec for inverted joiner

This commit reworks the InvertedJoiner to use an IndexFetchSpec
instead of table and index descriptors.

The "internal schema" now matches the fetched columns (leading to
simplifications in both planning and execution code).

Release note: None

77878: kvstreamer: re-enable streamer by default r=yuzefovich a=yuzefovich

Release note: None

77968: sql: keep experimental_enable_hash_sharded_indexes var as noop r=chengxiong-ruan a=chengxiong-ruan

Fixes #77954

Release justification: needed for session var backward compatibility.
Release note (sql change): experimental_enable_hash_sharded_indexes
used to be set to enable the hash sharded index feature. since we now
enable the feature by default, user don't need to set this var anymore.
But still keeping it as noop for backward compatibility.

77995: opt: add ContainedBy operator (<@) to FoldNullComparisonLeft norm rule r=msirek,mgartner a=michae2

Fixes: #77745

When we originally made the ContainedBy operator (<`@)` distinct from the
Contains operator (`@>)` we forgot to add ContainedBy to the
FoldNullComparisonLeft normalization rule. It was only added to the
FoldNullComparisonRight rule. Correct this.

Release note (bug fix): fix an optimizer bug that prevented expressions
of the form (NULL::STRING[] <@ ARRAY['x']) from being folded to NULL.

78013: roachtest: fix handling of test panics r=erikgrinaker a=tbg

- roachtest: fix handling of test panics
- roachtest: gracefully fail in sst-corruption test


78023: roachprod: update thrift artifact url for use with charybdefs r=tbg a=nicktrav

The version of Thrift used in tests that use `charybdefs` has
disappeared from the Apache artifacts repository that is used currently,
which causes any roachtest that depends on `charybdefs` to fail due to
not being able to fetch the artifact it needs.

Update the artifact URL to pull from the Apache archive instead.

Touches #78006,#78007,#78008,#78010,#78015,#78016.

Release note: None.

Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Chengxiong Ruan <[email protected]>
Co-authored-by: Michael Erickson <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Nick Travers <[email protected]>

pkg/ccl/logictestccl/testdata/logic_test/multi_region_query_behavior

@@ -404,7 +404,7 @@ vectorized: true
                   table: orders@orders_pkey
                   spans: [/'ap-southeast-2'/'94e4b847-8f2f-4ac5-83f1-641d6e3df727' - /'ap-southeast-2'/'94e4b847-8f2f-4ac5-83f1-641d6e3df727'] [/'us-east-1'/'94e4b847-8f2f-4ac5-83f1-641d6e3df727' - /'us-east-1'/'94e4b847-8f2f-4ac5-83f1-641d6e3df727']
 ·
-Diagram: https://cockroachdb.github.io/distsqlplan/decode.html#eJysVN1uGjsQvj9PYc1NzolM2L8AWSmSoxzaElFIIVIr1Qht1kPiBuyt7W23irjqs_W9qt0lpBsVpKS9ANsz83m-b3bG92A_LyGG_ofL4dlgRP79fzC9mr4b_kem_WH__IpoI9DMpaAkM3ql7VG1bAwyRXI2rTfzr9Ld6tzNq4AH7yER0qY6V64ZWEeRV5Px2zqFJRfjwWiThIwfdkdSLTRhPPe8EDeRte39m_6kv6VHTsnBSYTRdS_qtnqLYNGKkvS41QsXfqsT-aKDoVh0g-4BUFBa4ChZoYX4I0Qwo5AZnaK12pSm-ypgIAqIPQpSZbkrzTMKqTYI8T046ZYIMVwl10ucYCLQtD2gINAlclldWzNl9TLP7vAbUDjXy3ylbNwoqkyRklIRUJhmSeluc2Ac2hw4L04izgss_657rzkveov2xY_vnBcLX3Be-EKdlofuAYe2RxIliE-0u0UDszUFnbtH-tYlNwixv6Yvk-j_XYmlGO9FMndKC3ZKe1Rk0chkSXJVMUTREDVb_6YGI93SWTtoqh_KlXTE30nFe06VB-oLGofiQkuFph02U9WTwOplXtZxLkUBdAvrF5khrLMdE1ZeMM5dTJhPWUBZSFm0k2n4HKYlw007RPtYbtphqPVdnpFPWiqiVUxYCRqPCOs2yW7b5vF1eXg3tp0zQSXQVJoIO6aEBeXvkHV2Koueo2yCNtPK4pN-2PV1ZxRQ3GDdW1bnJsVLo9MqTX0cV7jKINC62hvUh4GqXNUo_gr2_wQc7AWHDbD3FBzuBUf7wdFe8PET8Gz9z88AAAD__y7iIUk=
+Diagram: https://cockroachdb.github.io/distsqlplan/decode.html#eJyslM1uGjEQx-99CmsuaSMT9otAVorkKKUtEYUUIrVSjdBmPSRuwN7a3nariFOfre9V7S4hISpUSXsA2-P5M_Mbz3AL9uscYuh-Ou-f9Abk5eve-GL8of-KjLv97ukF0UagmUpBSWb0QtuDalkZZIrkZFxvpt-lu9a5m1YOd7f7REib6ly5Tcfai7wZDd_XISw5G_YGqyBkeLc7kGqmCeO554W48qxtH991R911euSY7B1FGF12onajMwtmjShJW41OOPMbh5EvDjEUs3bQ3gMKSgscJAu0EH-GCCYUMqNTtFab0nRbOfREAbFHQaosd6V5QiHVBiG-BSfdHCGGi-RyjiNMBJqmBxQEukTOq5-tM2X1Ms1u8AdQONXzfKFsvFFUmSIlJRFQGGdJed3kwDg0OXBeHEWcF1h-XXbecl50Zs2zXz85L2a-4LzwhTouD-09Dk2PJEoQn2h3jQYmSwo6d_fpW5dcIcT-kj4P0f-_iCWM9yzMrWjBVrR7IotGJnOSqypDFBtQk-UfajDQDZ01g036vlxIR_ytqXhPqXJPfUPjUJxpqdA0w81Q9SSwepmWdZxKUQBdy7pFZghrrceEhQ9f4n5gV08wzF1MmE9ZQFlIWbSVIXwKQ5n7qlGiXfmvGqWv9U2ekS9aKqJVTFgpGg4Ia_8V4-4fZQ00QiXQVEyEtShhQfnZZ4dbyaKnkI3QZlpZfNQp2959QgHFFdZdZ3VuUjw3Oq3C1MdhpasMAq2rb4P60FPVVTWkD8X-v4iDneJwQ-w9Foc7xdFucbRT3Hoknixf_A4AAP__TIEqRA==
 
 # Regression test for #74890. Code should not panic due to distribution already
 # provided by input.

pkg/cmd/roachtest/test_runner.go

@@ -857,8 +857,8 @@ func (r *testRunner) runTest(
 			// We only have to record panics if the panic'd value is not the sentinel
 			// produced by t.Fatal*().
 			if r := recover(); r != nil && r != errTestFatal {
-				// TODO(andreimatei): prevent the cluster from being reused.
-				t.Fatalf("test panicked: %v", r)
+				// NB: we're careful to avoid t.Fatalf here, which re-panics.
+				t.Errorf("test panicked: %v", r)
 			}
 		}()
 

pkg/cmd/roachtest/test_test.go

@@ -14,6 +14,7 @@ import (
 	"bytes"
 	"context"
 	"io/ioutil"
+	"math/rand"
 	"regexp"
 	"sort"
 	"strings"
@@ -126,6 +127,18 @@ func TestRunnerRun(t *testing.T) {
 		},
 		Cluster: r.MakeClusterSpec(0),
 	})
+	r.Add(registry.TestSpec{
+		Name:  "panic",
+		Owner: OwnerUnitTest,
+		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
+			sl := []int{0}
+			// We need to throw the RoachVet linter off our scent since it's pretty
+			// good at figuring out static out of bound indexing.
+			idx := rand.Intn(2) + 1 // definitely out of bounds
+			t.L().Printf("boom %d", sl[idx])
+		},
+		Cluster: r.MakeClusterSpec(0),
+	})
 
 	testCases := []struct {
 		filters []string
@@ -139,6 +152,7 @@ func TestRunnerRun(t *testing.T) {
 		{filters: []string{"pass", "fail"}, expErr: "some tests failed"},
 		{filters: []string{"notests"}, expErr: "no test"},
 		{filters: []string{"errors"}, expErr: "some tests failed", expOut: "second error"},
+		{filters: []string{"panic"}, expErr: "some tests failed", expOut: "index out of range"},
 	}
 	for _, c := range testCases {
 		t.Run("", func(t *testing.T) {

pkg/cmd/roachtest/tests/sstable_corruption.go

@@ -76,6 +76,9 @@ func runSSTableCorruption(ctx context.Context, t test.Test, c cluster.Cluster) {
 		for _, sstLine := range tableSSTs {
 			sstLine = strings.TrimSpace(sstLine)
 			firstFileIdx := strings.Index(sstLine, ":")
+			if firstFileIdx < 0 {
+				t.Fatalf("unexpected format for sst line: %s", sstLine)
+			}
 			_, err = strconv.Atoi(sstLine[:firstFileIdx])
 			if err != nil {
 				t.Fatalf("error when converting %s to int: %s", sstLine[:firstFileIdx], err.Error())

pkg/geo/geoindex/BUILD.bazel

@@ -5,6 +5,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 go_library(
     name = "geoindex",
     srcs = [
+        "config.go",
         "geoindex.go",
         "s2_geography_index.go",
         "s2_geometry_index.go",

pkg/geo/geoindex/config.go

@@ -0,0 +1,29 @@
+// Copyright 2022 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package geoindex
+
+// IsEmpty returns whether the config contains a geospatial index
+// configuration.
+func (cfg Config) IsEmpty() bool {
+	return cfg.S2Geography == nil && cfg.S2Geometry == nil
+}
+
+// IsGeography returns whether the config is a geography geospatial index
+// configuration.
+func (cfg Config) IsGeography() bool {
+	return cfg.S2Geography != nil
+}
+
+// IsGeometry returns whether the config is a geometry geospatial index
+// configuration.
+func (cfg Config) IsGeometry() bool {
+	return cfg.S2Geometry != nil
+}

pkg/geo/geoindex/geoindex.go

@@ -239,33 +239,6 @@ func (gr RelationshipType) String() string {
 	return geoRelationshipTypeStr[gr]
 }
 
-// IsEmptyConfig returns whether the given config contains a geospatial index
-// configuration.
-func IsEmptyConfig(cfg *Config) bool {
-	if cfg == nil {
-		return true
-	}
-	return cfg.S2Geography == nil && cfg.S2Geometry == nil
-}
-
-// IsGeographyConfig returns whether the config is a geography geospatial
-// index configuration.
-func IsGeographyConfig(cfg *Config) bool {
-	if cfg == nil {
-		return false
-	}
-	return cfg.S2Geography != nil
-}
-
-// IsGeometryConfig returns whether the config is a geometry geospatial
-// index configuration.
-func IsGeometryConfig(cfg *Config) bool {
-	if cfg == nil {
-		return false
-	}
-	return cfg.S2Geometry != nil
-}
-
 // Key is one entry under which a geospatial shape is stored on behalf of an
 // Index. The index is of the form (Key, Primary Key).
 type Key uint64

pkg/roachprod/install/install.go

@@ -39,7 +39,7 @@ sudo service cassandra stop;
     sudo mkdir -p "${thrift_dir}"
     sudo chmod 777 "${thrift_dir}"
     cd "${thrift_dir}"
-    curl "https://downloads.apache.org/thrift/0.13.0/thrift-0.13.0.tar.gz" | sudo tar xvz --strip-components 1
+    curl "https://archive.apache.org/dist/thrift/0.13.0/thrift-0.13.0.tar.gz" | sudo tar xvz --strip-components 1
     sudo ./configure --prefix=/usr
     sudo make -j$(nproc)
     sudo make install

pkg/sql/backfill.go

@@ -17,7 +17,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/cockroachdb/cockroach/pkg/geo/geoindex"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/keys"
@@ -1597,7 +1596,7 @@ func countExpectedRowsForInvertedIndex(
 	if err := runHistoricalTxn(ctx, func(ctx context.Context, txn *kv.Txn, ie sqlutil.InternalExecutor) error {
 		var stmt string
 		geoConfig := idx.GetGeoConfig()
-		if geoindex.IsEmptyConfig(&geoConfig) {
+		if geoConfig.IsEmpty() {
 			stmt = fmt.Sprintf(
 				`SELECT coalesce(sum_int(crdb_internal.num_inverted_index_entries(%s, %d)), 0) FROM [%d AS t]`,
 				colNameOrExpr, idx.GetVersion(), desc.GetID(),

pkg/sql/catalog/descpb/index_fetch.go

@@ -10,6 +10,8 @@
 
 package descpb
 
+import "github.com/cockroachdb/cockroach/pkg/sql/types"
+
 // IndexFetchSpecVersionInitial is the initial IndexFetchSpec version.
 const IndexFetchSpecVersionInitial = 1
 
@@ -35,3 +37,21 @@ func (s *IndexFetchSpec) KeyFullColumns() []IndexFetchSpec_KeyColumn {
 func (s *IndexFetchSpec) KeySuffixColumns() []IndexFetchSpec_KeyColumn {
 	return s.KeyAndSuffixColumns[len(s.KeyAndSuffixColumns)-int(s.NumKeySuffixColumns):]
 }
+
+// FetchedColumnTypes returns the types of the fetched columns in a slice.
+func (s *IndexFetchSpec) FetchedColumnTypes() []*types.T {
+	res := make([]*types.T, len(s.FetchedColumns))
+	for i := range res {
+		res[i] = s.FetchedColumns[i].Type
+	}
+	return res
+}
+
+// DatumEncoding returns the datum encoding that corresponds to the key column
+// direction.
+func (c *IndexFetchSpec_KeyColumn) DatumEncoding() DatumEncoding {
+	if c.Direction == IndexDescriptor_DESC {
+		return DatumEncoding_DESCENDING_KEY
+	}
+	return DatumEncoding_ASCENDING_KEY
+}

pkg/sql/catalog/descpb/index_fetch.proto

@@ -16,6 +16,7 @@ option go_package = "descpb";
 import "gogoproto/gogo.proto";
 import "sql/types/types.proto";
 import "sql/catalog/descpb/structured.proto";
+import "geo/geoindex/config.proto";
 
 // IndexFetchSpec contains the subset of information (from TableDescriptor and
 // IndexDescriptor) that is necessary to decode KVs into SQL keys and values.
@@ -87,6 +88,9 @@ message IndexFetchSpec {
   optional bool is_secondary_index = 6 [(gogoproto.nullable) = false];
   optional bool is_unique_index = 7 [(gogoproto.nullable) = false];
 
+  // GeoConfig is used if we are fetching an inverted geospatial index.
+  optional geo.geoindex.Config geo_config = 16 [(gogoproto.nullable) = false];
+
   // EncodingType represents what sort of k/v encoding is used to store the
   // table data.
   optional uint32 encoding_type = 8 [(gogoproto.nullable) = false,

pkg/sql/create_index.go

@@ -279,10 +279,10 @@ func makeIndexDescriptor(
 	// Increment telemetry once a descriptor has been successfully created.
 	if indexDesc.Type == descpb.IndexDescriptor_INVERTED {
 		telemetry.Inc(sqltelemetry.InvertedIndexCounter)
-		if geoindex.IsGeometryConfig(&indexDesc.GeoConfig) {
+		if indexDesc.GeoConfig.IsGeometry() {
 			telemetry.Inc(sqltelemetry.GeometryInvertedIndexCounter)
 		}
-		if geoindex.IsGeographyConfig(&indexDesc.GeoConfig) {
+		if indexDesc.GeoConfig.IsGeography() {
 			telemetry.Inc(sqltelemetry.GeographyInvertedIndexCounter)
 		}
 		if indexDesc.IsPartial() {

pkg/sql/create_table.go

@@ -2195,10 +2195,10 @@ func NewTableDesc(
 		if idx.GetType() == descpb.IndexDescriptor_INVERTED {
 			telemetry.Inc(sqltelemetry.InvertedIndexCounter)
 			geoConfig := idx.GetGeoConfig()
-			if !geoindex.IsEmptyConfig(&geoConfig) {
-				if geoindex.IsGeographyConfig(&geoConfig) {
+			if !geoConfig.IsEmpty() {
+				if geoConfig.IsGeography() {
 					telemetry.Inc(sqltelemetry.GeographyInvertedIndexCounter)
-				} else if geoindex.IsGeometryConfig(&geoConfig) {
+				} else if geoConfig.IsGeometry() {
 					telemetry.Inc(sqltelemetry.GeometryInvertedIndexCounter)
 				}
 			}