Merge cockroachdb#51241 cockroachdb#51306

51241: geoindex,sql: use SRID bounds for a geometry inverted index r=sumeerbhola a=sumeerbhola

Release note: None

51306: opt: move stub exec factory r=RaduBerinde a=RaduBerinde

Move the stub factory to the exec package, so it can be used in more
places.

Release note: None

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

Author: 3 people

pkg/geo/geoindex/s2_geometry_index.go

@@ -15,6 +15,8 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/geo"
 	"github.com/cockroachdb/cockroach/pkg/geo/geomfn"
+	"github.com/cockroachdb/cockroach/pkg/geo/geopb"
+	"github.com/cockroachdb/cockroach/pkg/geo/geoprojbase"
 	"github.com/cockroachdb/cockroach/pkg/geo/geos"
 	"github.com/cockroachdb/errors"
 	"github.com/golang/geo/r3"
@@ -32,15 +34,15 @@ type s2GeometryIndex struct {
 
 var _ GeometryIndex = (*s2GeometryIndex)(nil)
 
+// We adjust the clipping bounds to be smaller by this fraction, since using
+// the endpoints of face 0 in S2 causes coverings to spill out of that face.
+const clippingBoundsDelta = 0.01
+
 // NewS2GeometryIndex returns an index with the given configuration. All reads and
 // writes on this index must use the same config. Writes must use the same
 // config to correctly process deletions. Reads must use the same config since
 // the bounds affect when a read needs to look at the exceedsBoundsCellID.
 func NewS2GeometryIndex(cfg S2GeometryConfig) GeometryIndex {
-	// We adjust the clipping bounds to be smaller by this fraction, since using
-	// the endpoints of face 0 in S2 causes coverings to spill out of that face.
-	const boundsDelta = 0.01
-
 	// TODO(sumeer): Sanity check cfg.
 	return &s2GeometryIndex{
 		rc: &s2.RegionCoverer{
@@ -53,17 +55,19 @@ func NewS2GeometryIndex(cfg S2GeometryConfig) GeometryIndex {
 		maxX:   cfg.MaxX,
 		minY:   cfg.MinY,
 		maxY:   cfg.MaxY,
-		deltaX: boundsDelta * (cfg.MaxX - cfg.MinX),
-		deltaY: boundsDelta * (cfg.MaxY - cfg.MinY),
+		deltaX: clippingBoundsDelta * (cfg.MaxX - cfg.MinX),
+		deltaY: clippingBoundsDelta * (cfg.MaxY - cfg.MinY),
 	}
 }
 
+// TODO(sumeer): also support index config with parameters specified by CREATE
+// INDEX.
+
 // DefaultGeometryIndexConfig returns a default config for a geometry index.
 func DefaultGeometryIndexConfig() *Config {
 	return &Config{
 		S2Geometry: &S2GeometryConfig{
 			// Arbitrary bounding box.
-			// TODO(sumeer): replace with parameters specified by CREATE INDEX.
 			MinX:     -10000,
 			MaxX:     10000,
 			MinY:     -10000,
@@ -72,6 +76,38 @@ func DefaultGeometryIndexConfig() *Config {
 	}
 }
 
+// GeometryIndexConfigForSRID returns a geometry index config for srid.
+func GeometryIndexConfigForSRID(srid geopb.SRID) *Config {
+	p, exists := geoprojbase.Projection(srid)
+	if !exists || p.Bounds == nil {
+		return DefaultGeometryIndexConfig()
+	}
+	b := p.Bounds
+	minX, maxX, minY, maxY := b.MinX, b.MaxX, b.MinY, b.MaxY
+	// There are projections where the min and max are equal e.g. 3571.
+	// We need to have a valid rectangle as the geometry index bounds.
+	if maxX-minX < 1 {
+		maxX++
+	}
+	if maxY-minY < 1 {
+		maxY++
+	}
+	// Expand the bounds by 2x the clippingBoundsDelta, to
+	// ensure that shapes touching the bounds don't get
+	// clipped.
+	boundsExpansion := 2 * clippingBoundsDelta
+	deltaX := (maxX - minX) * boundsExpansion
+	deltaY := (maxY - minY) * boundsExpansion
+	return &Config{
+		S2Geometry: &S2GeometryConfig{
+			MinX:     minX - deltaX,
+			MaxX:     maxX + deltaX,
+			MinY:     minY - deltaY,
+			MaxY:     maxY + deltaY,
+			S2Config: defaultS2Config()},
+	}
+}
+
 // A cell id unused by S2. We use it to index geometries that exceed the
 // configured bounds.
 const exceedsBoundsCellID = s2.CellID(^uint64(0))

pkg/geo/geoindex/s2_geometry_index_test.go

@@ -16,9 +16,11 @@ import (
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/geo"
+	"github.com/cockroachdb/cockroach/pkg/geo/geoprojbase"
 	"github.com/cockroachdb/cockroach/pkg/geo/geos"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/datadriven"
+	"github.com/stretchr/testify/require"
 )
 
 func TestS2GeometryIndexBasic(t *testing.T) {
@@ -113,3 +115,31 @@ func TestClipEWKBByRect(t *testing.T) {
 		}
 	})
 }
+
+func TestNoClippingAtSRIDBounds(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	// Test that indexes that use the SRID bounds don't clip shapes that touch
+	// those bounds. This test uses point shapes representing the four corners
+	// of the bounds.
+	for srid, projInfo := range geoprojbase.Projections {
+		if projInfo.Bounds == nil {
+			continue
+		}
+		b := projInfo.Bounds
+		index := NewS2GeometryIndex(*GeometryIndexConfigForSRID(srid).S2Geometry)
+		// Four corners of the bounds, proceeding clockwise from the lower-left.
+		xCorners := []float64{b.MinX, b.MinX, b.MaxX, b.MaxX}
+		yCorners := []float64{b.MinY, b.MaxY, b.MaxY, b.MinY}
+		for i := range xCorners {
+			g, err := geo.NewGeometryFromPointCoords(xCorners[i], yCorners[i])
+			require.NoError(t, err)
+			keys, err := index.InvertedIndexKeys(context.Background(), g)
+			require.NoError(t, err)
+			require.Equal(t, 1, len(keys))
+			require.NotEqual(t, Key(exceedsBoundsCellID), keys[0],
+				"SRID: %d, Point: %f, %f", srid, xCorners[i], yCorners[i])
+		}
+	}
+
+}

pkg/geo/geoprojbase/geoprojbase.go

@@ -47,7 +47,7 @@ func (p *Proj4Text) Equal(o Proj4Text) bool {
 	return bytes.Equal(p.cStr, o.cStr)
 }
 
-// Bounds represents the lat/lng bounds.
+// Bounds represents the projected or lat/lng bounds.
 type Bounds struct {
 	MinX float64
 	MaxX float64
@@ -70,7 +70,7 @@ type ProjInfo struct {
 
 	// Denormalized fields.
 
-	// Bounds defines the bounds (in lat lng) of the given coordinate system.
+	// Bounds defines the bounds (projected or lat/lng) of the given coordinate system.
 	// If nil, no bounds were defined for the given projection.
 	Bounds *Bounds
 	// IsLatLng stores whether the projection is a LatLng based projection (denormalized from above)

pkg/sql/create_index.go

@@ -162,7 +162,7 @@ func MakeIndexDescriptor(
 			return nil, err
 		}
 		if columnDesc.Type.InternalType.Family == types.GeometryFamily {
-			indexDesc.GeoConfig = *geoindex.DefaultGeometryIndexConfig()
+			indexDesc.GeoConfig = *geoindex.GeometryIndexConfigForSRID(columnDesc.Type.GeoSRIDOrZero())
 			telemetry.Inc(sqltelemetry.GeometryInvertedIndexCounter)
 		}
 		if columnDesc.Type.InternalType.Family == types.GeographyFamily {

pkg/sql/create_table.go

@@ -1409,7 +1409,7 @@ func MakeTableDesc(
 					return desc, err
 				}
 				if columnDesc.Type.InternalType.Family == types.GeometryFamily {
-					idx.GeoConfig = *geoindex.DefaultGeometryIndexConfig()
+					idx.GeoConfig = *geoindex.GeometryIndexConfigForSRID(columnDesc.Type.GeoSRIDOrZero())
 				}
 				if columnDesc.Type.InternalType.Family == types.GeographyFamily {
 					idx.GeoConfig = *geoindex.DefaultGeographyIndexConfig()

pkg/sql/logictest/testdata/logic_test/inverted_index_geospatial

@@ -0,0 +1,81 @@
+# LogicTest: local
+
+# SRID of the geometry column is unspecified, so default index bounds are used.
+statement ok
+CREATE TABLE geo_table(
+  k int primary key,
+  geom geometry,
+  INVERTED INDEX geom_index(geom)
+)
+
+# Shapes with SRID 26918. We've taken small X, Y values and added 400,000 to the X coordinate
+# and 4,000,000 to the Y coordinate to place them inside the bounds of SRID 26918.
+statement ok
+INSERT INTO geo_table VALUES
+  (1, 'SRID=26918;POINT(400001 4000001)'),
+  (2, 'SRID=26918;LINESTRING(400001 4000001, 400002 4000002)'),
+  (3, 'SRID=26918;POINT(400003 4000003)'),
+  (4, 'SRID=26918;LINESTRING(400004 4000004, 400005 4000005)'),
+  (5, 'SRID=26918;LINESTRING(400040 4000040, 400041 4000041)'),
+  (6, 'SRID=26918;POLYGON((400001 4000001, 400005 4000001, 400005 4000005, 400001 4000005, 400001 4000001))')
+
+query I
+SELECT k FROM geo_table WHERE ST_Intersects('SRID=26918;POINT(400003 4000003)'::geometry, geom) ORDER BY k
+----
+3
+6
+
+# The InvertedFilterer stats show "rows read: 6" since all the above shapes overflow
+# the default index bounds.
+query T
+SELECT url FROM [EXPLAIN ANALYZE SELECT k FROM geo_table WHERE ST_Intersects('SRID=26918;POINT(400003 4000003)'::geometry, geom) ORDER BY k]
+----
+https://cockroachdb.github.io/distsqlplan/decode.html#eJykU1Fr20wQfP9-xbIvifku-E4StnOlYCdRWrWOncqGNo1MUKwliEg69-7UOgT_9yIpaWNDREz1InZ3Zm-GYR_R_MhQ4swf-6dzKHUG5-H0Aq79b5fjUTCB0WQ0vvruw-FZMJvPvow78AS9b4B3pG5sfJsRfP3ohz4Ye5MWlrShpTWHB7MwOHvv9I7F4N3lNJjMDz3OOXeh_nG3cyDlB3964c_DK1btyjswDc_8EE6u4H6BDAuV0CTOyaC8RoELhiutlmSM0lXrsQYEyRolZ5gWq9JW7QXDpdKE8hFtajNCifNKZEhxQrrLkWFCNk6zem317k1aJLQe_rGDDGeruDASukfiqGZo9cuApjiR0EOGxsZZBjbNSQI3yPD2wdIzQPQ9OMHFhqEq7V9RxsZ3hFJs2NuFB8VP0paS8zSzpEl3xbb657m_XmlQBQyFBFNJB2NjbWWEUeTwfhS5_X6EQEXStASPEJDhtLQShuINBvN4DTnlSj9AaSiR4HD4nL5u09nH5ieVFk_xONsGVzrNY_2wlc2TaDZ03qC7jhZ2UE1zB_uaE3cfJzOlLemuu-1iKP5Hhk2IcvdMuOCiOgiHO73eMX_5nfZGYuCJphjwgRh4nu-JA_nycoZO59-SFO1Jevv4D8msVGFoy_9rm_lmwZCSO2qu2ahSL-lSq2X9TFNOa17dSMjYZiqaIiiaUSXwJVm0kp12stNKdtvJbivZ2yEvNv_9DgAA__80dbuh
+
+statement ok
+DROP TABLE geo_table
+
+# SRID of the geometry column is specified, so SRID specific bounds are used.
+statement ok
+CREATE TABLE geo_table(
+  k int primary key,
+  geom geometry(geometry, 26918),
+  INVERTED INDEX geom_index(geom)
+)
+
+# Same shapes.
+statement ok
+INSERT INTO geo_table VALUES
+  (1, 'SRID=26918;POINT(400001 4000001)'),
+  (2, 'SRID=26918;LINESTRING(400001 4000001, 400002 4000002)'),
+  (3, 'SRID=26918;POINT(400003 4000003)'),
+  (4, 'SRID=26918;LINESTRING(400004 4000004, 400005 4000005)'),
+  (5, 'SRID=26918;LINESTRING(400040 4000040, 400041 4000041)'),
+  (6, 'SRID=26918;POLYGON((400001 4000001, 400005 4000001, 400005 4000005, 400001 4000005, 400001 4000001))')
+
+
+# Same result.
+query I
+SELECT k FROM geo_table WHERE ST_Intersects('SRID=26918;POINT(400003 4000003)'::geometry, geom) ORDER BY k
+----
+3
+6
+
+# The InvertedFilterer stats show "rows read: 2" since all the above shapes are within the index
+# bounds.
+query T
+SELECT url FROM [EXPLAIN ANALYZE SELECT k FROM geo_table WHERE ST_Intersects('SRID=26918;POINT(400003 4000003)'::geometry, geom) ORDER BY k]
+----
+https://cockroachdb.github.io/distsqlplan/decode.html#eJykVFFP2zAQft-vON0LVMtU2-na4GlSC4QtrLQsrbQxXKHQnFhEGne2uxWh_vcpCWy0iA60vER3933n-853vkX7I0eJo7AfHoxhYXI4iocncB5-Pe33ogH0Br3-2bcQdg-j0Xj0ud-AO-h1DbwifeGSy5zgy8cwDsG6i6xwZCxNnd3dGcXR4XvR3uPBu9NhNBjvthhjzIfqx_zGjpQfwuFJOI7PvDLXrAHD-DCMYf8MrifoYaFTGiQzsijPkePEw7nRU7JWm9J1WwGidImSeZgV84Ur3RMPp9oQylt0mcsJJY7LImNKUjJNhh6m5JIsr9KW515kRUrL7h856OFonhRWQlPwNu-ITiA49zuBH7DgzWPfHiRFCj4D7b6Tseih0b8sGEpSCQI9tC7Jc3DZjCSwMn554-ge0BGwj5OVh3rh_kqwLrkilHzlPV9mVPwk4yg9ynJHhkyTr2u9j4fLuQFdQJdLsKVQsC4xTipUyu-8VYoxXynRZlOljFKCM6UE6xSRQqAi_SfuWCE8aslw4SR0-TOaM0uWMKOZNjewsFSiGHzKnu6ReEmPjnVW3E2CWO_O3GSzxNysjcFd0V5XPKPuaopgA1U7N7BPKfFfomSkjSPT9NdVdPlr9LCeALm5kYwzXu6eYKLd3mMPv4N2jwctXhsBC3jQaoUtviMfLmlXNP7vJvn2m2y9RH9Mdq4LS2v6n8rMVhMPKb2i-uGwemGmdGr0tDqmNocVr3KkZF0d5bURFXWoLPAhmW8li-1ksZXsbyf7W8mtDfJk9ep3AAAA__9EAdjp
+
+# Also works when creating an index.
+statement ok
+DROP INDEX geo_table@geom_index
+
+statement ok
+CREATE INVERTED INDEX geom_index ON geo_table(geom)
+
+query T
+SELECT url FROM [EXPLAIN ANALYZE SELECT k FROM geo_table WHERE ST_Intersects('SRID=26918;POINT(400003 4000003)'::geometry, geom) ORDER BY k]
+----
+https://cockroachdb.github.io/distsqlplan/decode.html#eJykVFFP2zAQft-vON0LVMtU2-na4GlSC4QtrLQsrbQxXKHQnFhEGne2uxWh_vcpCWy0iA60vER3933n-853vkX7I0eJo7AfHoxhYXI4iocncB5-Pe33ogH0Br3-2bcQdg-j0Xj0ud-AO-h1DbwifeGSy5zgy8cwDsG6i6xwZCxNnd3dGcXR4XvR3uPBu9NhNBjvthhjzIfqx_zGjpQfwuFJOI7PvDLXrAHD-DCMYf8MrifoYaFTGiQzsijPkePEw7nRU7JWm9J1WwGidImSeZgV84Ur3RMPp9oQylt0mcsJJY7LImNKUjJNhh6m5JIsr9KW515kRUrL7h856OFonhRWQlPwNu-ITiA49zuBH7DgzWPfHiRFCj4D7b6Tseih0b8sGEpSCQI9tC7Jc3DZjCSwMn554-ge0BGwj5OVh3rh_kqwLrkilHzlPV9mVPwk4yg9ynJHhkyTr2u9j4fLuQFdQJdLsKVQsC4xTipUyu-8VYoxXynRZlOljFKCM6UE6xSRQqAi_SfuWCE8aslw4SR0-TOaM0uWMKOZNjewsFSiGHzKnu6ReEmPjnVW3E2CWO_O3GSzxNysjcFd0V5XPKPuaopgA1U7N7BPKfFfomSkjSPT9NdVdPlr9LCeALm5kYwzXu6eYKLd3mMPv4N2jwctXhsBC3jQaoUtviMfLmlXNP7vJvn2m2y9RH9Mdq4LS2v6n8rMVhMPKb2i-uGwemGmdGr0tDqmNocVr3KkZF0d5bURFXWoLPAhmW8li-1ksZXsbyf7W8mtDfJk9ep3AAAA__9EAdjp

pkg/sql/opt/bench/bench_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/security/securitytest"
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/cockroachdb/cockroach/pkg/sql/opt/exec"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/exec/execbuilder"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/memo"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder"
@@ -571,8 +572,7 @@ func (h *harness) runUsingAPI(tb testing.TB, bmType BenchmarkType, usePrepared b
 	}
 
 	root := execMemo.RootExpr()
-	execFactory := stubFactory{}
-	eb := execbuilder.New(&execFactory, execMemo, nil /* catalog */, root, &h.evalCtx)
+	eb := execbuilder.New(exec.StubFactory{}, execMemo, nil /* catalog */, root, &h.evalCtx)
 	if _, err = eb.Build(); err != nil {
 		tb.Fatalf("%v", err)
 	}