Merge #107515

107515: builtins: fix st_asmvtgeom to return NULL for empty geometry r=yuzefovich a=yuzefovich Previously, when `st_asmvtgeom` resulted in an empty geometry, we would create an empty `DGeometry` object, which is different from `DNull`. This is incorrect (different from postgres) and could lead to internal errors in distributed plans. This is now fixed. No release note given that this is a new feature. Fixes: #106884. Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]>
cockroachdb · Jul 25, 2023 · d9c0e34 · d9c0e34
2 parents 7dea772 + e41129c
commit d9c0e34
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 38 deletions.
diff --git a/pkg/geo/geomfn/mvtgeom.go b/pkg/geo/geomfn/mvtgeom.go
@@ -19,7 +19,8 @@ import (
 	"github.com/twpayne/go-geom"
 )
 
-// AsMVTGeometry returns a geometry converted into Mapbox Vector Tile coordinate space.
+// AsMVTGeometry returns a geometry converted into Mapbox Vector Tile coordinate
+// space.
 func AsMVTGeometry(
 	g geo.Geometry, bounds geo.CartesianBoundingBox, extent int, buffer int, clipGeometry bool,
 ) (geo.Geometry, error) {
@@ -36,11 +37,7 @@ func AsMVTGeometry(
 		return geo.Geometry{}, nil
 	}
 
-	out, err := transformToMVTGeom(gt, g, bbox, extent, buffer, clipGeometry)
-	if err != nil {
-		return geo.Geometry{}, err
-	}
-	return *out, nil
+	return transformToMVTGeom(gt, g, bbox, extent, buffer, clipGeometry)
 }
 
 func validateInputParams(bbox geopb.BoundingBox, extent int, buffer int) error {
@@ -76,50 +73,50 @@ func geometrySmallerThanHalfBoundingBox(gt geom.T, bbox geopb.BoundingBox, exten
 // transformToMVTGeom transforms a geometry into vector tile coordinate space.
 func transformToMVTGeom(
 	gt geom.T, g geo.Geometry, bbox geopb.BoundingBox, extent int, buffer int, clipGeometry bool,
-) (*geo.Geometry, error) {
+) (geo.Geometry, error) {
 	basicType, err := getBasicType(gt)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to get geom.T basic type")
+		return geo.Geometry{}, errors.Wrap(err, "failed to get geom.T basic type")
 	}
 	basicTypeGeometry, err := convertToBasicType(g, basicType)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to convert geometry to basic type")
+		return geo.Geometry{}, errors.Wrap(err, "failed to convert geometry to basic type")
 	}
 	if basicTypeGeometry.Empty() {
-		return &geo.Geometry{}, nil
+		return geo.Geometry{}, nil
 	}
 
 	removeRepeatedPointsGeometry, err := RemoveRepeatedPoints(basicTypeGeometry, 0)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to remove repeated points")
+		return geo.Geometry{}, errors.Wrap(err, "failed to remove repeated points")
 	}
 	// Remove points on straight lines
 	simplifiedGeometry, empty, err := Simplify(removeRepeatedPointsGeometry, 0, false)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to simplify geometry")
+		return geo.Geometry{}, errors.Wrap(err, "failed to simplify geometry")
 	}
 	if empty {
-		return &geo.Geometry{}, nil
+		return geo.Geometry{}, nil
 	}
 
 	affinedGeometry, err := transformToTileCoordinateSpace(simplifiedGeometry, bbox, extent)
 	if err != nil {
-		return nil, err
+		return geo.Geometry{}, err
 	}
 
 	snappedGeometry, err := snapToIntegersGrid(affinedGeometry)
 	if err != nil {
-		return nil, err
+		return geo.Geometry{}, err
 	}
 
 	out, err := clipAndValidateMVTOutput(snappedGeometry, basicType, extent, buffer, clipGeometry)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to clip and validate")
+		return geo.Geometry{}, errors.Wrap(err, "failed to clip and validate")
 	}
 	if out.Empty() {
-		return &geo.Geometry{}, nil
+		return geo.Geometry{}, nil
 	}
-	return &out, nil
+	return out, nil
 }
 
 func transformToTileCoordinateSpace(

diff --git a/pkg/sql/logictest/testdata/logic_test/geospatial_zm b/pkg/sql/logictest/testdata/logic_test/geospatial_zm
@@ -397,3 +397,14 @@ FROM ( VALUES
 a10080b518c09a0c                                                a10080b518c09a0c                                                      a10080b518c09a0c
 a208030280b518c09a0c0e04c0cf24c0843d070a                        a2080f0280b518c09a0cb06d04c0cf24c0843dbf3e0a                          a2088f0280b518c09a0cb06dc0b802c0cf24c0843dbf3ea08d06
 a30801010480b518c0cf2402c09a0cc09a0c00ffb418bf9a0c0ac09a0c0009  a3080d010480b518c0cf24d00fc09a0cc09a0c00ffb418bf9a0c904ec09a0c008f4e  a3080d010480b518c0cf24d00fc09a0cc09a0c00ffb418bf9a0c904ec09a0c008f4e
+
+# Regression test for incorrectly not using DNull represenation for empty
+# geometry which then led to an internal error on the remote node (#106884).
+statement ok
+CREATE TABLE t106884 AS SELECT 1;
+SELECT st_asmvtgeom(
+          '01060000C000000000'::GEOMETRY,
+          'BOX(-2.4310452547766257 -0.7340617188515679,1.4606149586106913 1.509111744681483)'::BOX2D,
+          1::INT4
+        )::GEOMETRY
+        FROM t106884;
diff --git a/pkg/sql/sem/builtins/geo_builtins.go b/pkg/sql/sem/builtins/geo_builtins.go
@@ -4588,11 +4588,7 @@ The paths themselves are given in the direction of the first geometry.`,
 			Fn: func(_ context.Context, _ *eval.Context, args tree.Datums) (tree.Datum, error) {
 				g := tree.MustBeDGeometry(args[0]).Geometry
 				bbox := tree.MustBeDBox2D(args[1]).CartesianBoundingBox
-				newGeom, err := geomfn.AsMVTGeometry(g, bbox, 4096, 256, true)
-				if err != nil {
-					return nil, err
-				}
-				return &tree.DGeometry{Geometry: newGeom}, nil
+				return asMVTGeometry(g, bbox, 4096, 256, true)
 			},
 			Info: infoBuilder{
 				info: `Transforms a geometry into the coordinate space of a MVT (Mapbox Vector Tile) tile, clipping it to the tile bounds.
@@ -4616,11 +4612,7 @@ The rectangular bounds of the tile in the target map coordinate space must be pr
 				g := tree.MustBeDGeometry(args[0]).Geometry
 				bbox := tree.MustBeDBox2D(args[1]).CartesianBoundingBox
 				extent := int(tree.MustBeDInt(args[2]))
-				newGeom, err := geomfn.AsMVTGeometry(g, bbox, extent, 256, true)
-				if err != nil {
-					return nil, err
-				}
-				return &tree.DGeometry{Geometry: newGeom}, nil
+				return asMVTGeometry(g, bbox, extent, 256, true)
 			},
 			Info: infoBuilder{
 				info: `Transforms a geometry into the coordinate space of a MVT (Mapbox Vector Tile) tile, clipping it to the tile bounds.
@@ -4645,11 +4637,7 @@ The rectangular bounds of the tile in the target map coordinate space must be pr
 				bbox := tree.MustBeDBox2D(args[1]).CartesianBoundingBox
 				extent := int(tree.MustBeDInt(args[2]))
 				buffer := int(tree.MustBeDInt(args[3]))
-				newGeom, err := geomfn.AsMVTGeometry(g, bbox, extent, buffer, true)
-				if err != nil {
-					return nil, err
-				}
-				return &tree.DGeometry{Geometry: newGeom}, nil
+				return asMVTGeometry(g, bbox, extent, buffer, true)
 			},
 			Info: infoBuilder{
 				info: `Transforms a geometry into the coordinate space of a MVT (Mapbox Vector Tile) tile, clipping it to the tile bounds.
@@ -4675,11 +4663,7 @@ The rectangular bounds of the tile in the target map coordinate space must be pr
 				extent := int(tree.MustBeDInt(args[2]))
 				buffer := int(tree.MustBeDInt(args[3]))
 				clip := bool(tree.MustBeDBool(args[4]))
-				newGeom, err := geomfn.AsMVTGeometry(g, bbox, extent, buffer, clip)
-				if err != nil {
-					return nil, err
-				}
-				return &tree.DGeometry{Geometry: newGeom}, nil
+				return asMVTGeometry(g, bbox, extent, buffer, clip)
 			},
 			Info: infoBuilder{
 				info: `Transforms a geometry into the coordinate space of a MVT (Mapbox Vector Tile) tile, clipping it to the tile bounds if required.
@@ -7898,3 +7882,16 @@ func geosVersion() string {
 	}
 	return geosV
 }
+
+func asMVTGeometry(
+	g geo.Geometry, bounds geo.CartesianBoundingBox, extent int, buffer int, clipGeometry bool,
+) (tree.Datum, error) {
+	newGeom, err := geomfn.AsMVTGeometry(g, bounds, extent, buffer, clipGeometry)
+	if err != nil {
+		return nil, err
+	}
+	if newGeom.Empty() {
+		return tree.DNull, nil
+	}
+	return &tree.DGeometry{Geometry: newGeom}, nil
+}