Merge pull request cockroachdb#63759 from rafiss/backport20.2-63136-6…

…3263

release-20.2: geomfn;geogfn: fix check of st_segmentize when number of segments overflow

pkg/geo/geogfn/segmentize.go

@@ -57,6 +57,9 @@ func Segmentize(geography geo.Geography, segmentMaxLength float64) (geo.Geograph
 // segments is the power of 2, and all the segments are of the same length.
 // Note: List of points does not consist of end point.
 func segmentizeCoords(a geom.Coord, b geom.Coord, segmentMaxAngle float64) ([]float64, error) {
+	if segmentMaxAngle <= 0 {
+		return nil, errors.Newf("maximum segment angle must be positive")
+	}
 	// Converted geom.Coord into s2.Point so we can segmentize the coordinates.
 	pointA := s2.PointFromLatLng(s2.LatLngFromDegrees(a.Y(), a.X()))
 	pointB := s2.PointFromLatLng(s2.LatLngFromDegrees(b.Y(), b.X()))
@@ -69,24 +72,20 @@ func segmentizeCoords(a geom.Coord, b geom.Coord, segmentMaxAngle float64) ([]fl
 	// 2 coordinates, ensuring that the segments are divided into parts divisible by
 	// a power of 2.
 	//
-	// For that fraction by segment must be less than or equal to
-	// the fraction of max segment length to distance between point, since the
-	// total number of segment must be power of 2 therefore we can write as
-	// 1 / (2^n)[numberOfSegmentsToCreate] <= segmentMaxLength / distanceBetweenPoints < 1 / (2^(n-1))
-	// (2^n)[numberOfSegmentsToCreate] >= distanceBetweenPoints / segmentMaxLength > 2^(n-1)
-	// therefore n = ceil(log2(segmentMaxLength/distanceBetweenPoints)). Hence
-	// numberOfSegmentsToCreate = 2^(ceil(log2(segmentMaxLength/distanceBetweenPoints))).
-	numberOfSegmentsToCreate := int(math.Pow(2, math.Ceil(math.Log2(chordAngleBetweenPoints/segmentMaxAngle))))
-	numPoints := 2 * (1 + numberOfSegmentsToCreate)
-	if numPoints > geo.MaxAllowedSplitPoints {
-		return nil, errors.Newf(
-			"attempting to segmentize into too many coordinates; need %d points between %v and %v, max %d",
-			numPoints,
-			a,
-			b,
-			geo.MaxAllowedSplitPoints,
-		)
+	// We can write that power as 2^n in the following inequality
+	// 2^n >= ceil(chordAngleBetweenPoints/segmentMaxAngle) > 2^(n-1).
+	// We can drop the ceil since 2^n must be an int
+	// 2^n >= chordAngleBetweenPoints/segmentMaxAngle > 2^(n-1).
+	// Then n = ceil(log2(chordAngleBetweenPoints/segmentMaxAngle)).
+	// Hence numberOfSegmentsToCreate = 2^(ceil(log2(chordAngleBetweenPoints/segmentMaxAngle))).
+	doubleNumberOfSegmentsToCreate := math.Pow(2, math.Ceil(math.Log2(chordAngleBetweenPoints/segmentMaxAngle)))
+	doubleNumPoints := float64(len(a)) * (1 + doubleNumberOfSegmentsToCreate)
+	if err := geosegmentize.CheckSegmentizeTooManyPoints(doubleNumPoints, a, b); err != nil {
+		return nil, err
 	}
+	numberOfSegmentsToCreate := int(doubleNumberOfSegmentsToCreate)
+	numPoints := int(doubleNumPoints)
+
 	allSegmentizedCoordinates := make([]float64, 0, numPoints)
 	allSegmentizedCoordinates = append(allSegmentizedCoordinates, a.X(), a.Y())
 	for pointInserted := 1; pointInserted < numberOfSegmentsToCreate; pointInserted++ {

pkg/geo/geogfn/segmentize_test.go

@@ -131,57 +131,79 @@ func TestSegmentizeCoords(t *testing.T) {
 		desc                 string
 		a                    geom.Coord
 		b                    geom.Coord
-		segmentMaxLength     float64
+		segmentMaxAngle      float64
 		resultantCoordinates []float64
 	}{
 		{
 			desc:                 `Coordinate(0, 0) to Coordinate(85, 85), 0.781600222084459`,
 			a:                    geom.Coord{0, 0},
 			b:                    geom.Coord{85, 85},
-			segmentMaxLength:     0.781600222084459,
+			segmentMaxAngle:      0.781600222084459,
 			resultantCoordinates: []float64{0, 0, 4.924985039227315, 44.568038920632105},
 		},
 		{
 			desc:                 `Coordinate(0, 0) to Coordinate(85, 85), 0.7816000651234446`,
 			a:                    geom.Coord{0, 0},
 			b:                    geom.Coord{85, 85},
-			segmentMaxLength:     0.7816000651234446,
+			segmentMaxAngle:      0.7816000651234446,
 			resultantCoordinates: []float64{0, 0, 2.0486953806277866, 22.302074138733936, 4.924985039227315, 44.568038920632105, 11.655816669136822, 66.66485041602017},
 		},
 		{
 			desc:                 `Coordinate(85, 85) to Coordinate(0, 0), 0.29502092024628396`,
 			a:                    geom.Coord{85, 85},
 			b:                    geom.Coord{0, 0},
-			segmentMaxLength:     0.29502092024628396,
+			segmentMaxAngle:      0.29502092024628396,
 			resultantCoordinates: []float64{85, 85, 22.871662720021178, 77.3609628116894, 11.655816669136824, 66.66485041602017, 7.329091976767396, 55.658764687902504, 4.924985039227315, 44.56803892063211, 3.299940624127866, 33.443216802941045, 2.048695380627787, 22.302074138733943, 0.9845421446758968, 11.1527721155093},
 		},
-		{
-			desc:                 `Coordinate(85, 85) to Coordinate(0, 0), -1`,
-			a:                    geom.Coord{85, 85},
-			b:                    geom.Coord{0, 0},
-			segmentMaxLength:     -1,
-			resultantCoordinates: []float64{85, 85},
-		},
 		{
 			desc:                 `Coordinate(0, 0) to Coordinate(0, 0), 0.29`,
 			a:                    geom.Coord{0, 0},
 			b:                    geom.Coord{0, 0},
-			segmentMaxLength:     0.29,
+			segmentMaxAngle:      0.29,
 			resultantCoordinates: []float64{0, 0},
 		},
 		{
 			desc:                 `Coordinate(85, 85) to Coordinate(0, 0), 1.563200444168918`,
 			a:                    geom.Coord{85, 85},
 			b:                    geom.Coord{0, 0},
-			segmentMaxLength:     1.563200444168918,
+			segmentMaxAngle:      1.563200444168918,
 			resultantCoordinates: []float64{85, 85},
 		},
 	}
 	for _, test := range testCases {
 		t.Run(test.desc, func(t *testing.T) {
-			convertedPoints, err := segmentizeCoords(test.a, test.b, test.segmentMaxLength)
+			convertedPoints, err := segmentizeCoords(test.a, test.b, test.segmentMaxAngle)
 			require.NoError(t, err)
 			require.Equal(t, test.resultantCoordinates, convertedPoints)
 		})
 	}
+
+	errorTestCases := []struct {
+		desc            string
+		a               geom.Coord
+		b               geom.Coord
+		segmentMaxAngle float64
+		expectedErr     string
+	}{
+		{
+			desc:            "too many segments required",
+			a:               geom.Coord{0, 100},
+			b:               geom.Coord{1, 1},
+			segmentMaxAngle: 2e-8,
+			expectedErr:     fmt.Sprintf("attempting to segmentize into too many coordinates; need 268435458 points between [0 100] and [1 1], max %d", geo.MaxAllowedSplitPoints),
+		},
+		{
+			desc:            "negative max segment angle",
+			a:               geom.Coord{1, 1},
+			b:               geom.Coord{2, 2},
+			segmentMaxAngle: -1,
+			expectedErr:     "maximum segment angle must be positive",
+		},
+	}
+	for _, test := range errorTestCases {
+		t.Run(test.desc, func(t *testing.T) {
+			_, err := segmentizeCoords(test.a, test.b, test.segmentMaxAngle)
+			require.EqualError(t, err, test.expectedErr)
+		})
+	}
 }

pkg/geo/geomfn/segmentize.go

@@ -51,23 +51,24 @@ func Segmentize(g geo.Geometry, segmentMaxLength float64) (geo.Geometry, error)
 // segment has a length less than or equal to given maximum segment length.
 // Note: List of points does not consist of end point.
 func segmentizeCoords(a geom.Coord, b geom.Coord, maxSegmentLength float64) ([]float64, error) {
+	if maxSegmentLength <= 0 {
+		return nil, errors.Newf("maximum segment length must be positive")
+	}
 	distanceBetweenPoints := math.Sqrt(math.Pow(a.X()-b.X(), 2) + math.Pow(b.Y()-a.Y(), 2))
 
+	doubleNumberOfSegmentsToCreate := math.Ceil(distanceBetweenPoints / maxSegmentLength)
+	doubleNumPoints := float64(len(a)) * (1 + doubleNumberOfSegmentsToCreate)
+	if err := geosegmentize.CheckSegmentizeTooManyPoints(doubleNumPoints, a, b); err != nil {
+		return nil, err
+	}
+
 	// numberOfSegmentsToCreate represent the total number of segments
 	// in which given two coordinates will be divided.
-	numberOfSegmentsToCreate := int(math.Ceil(distanceBetweenPoints / maxSegmentLength))
-	numPoints := 2 * (1 + numberOfSegmentsToCreate)
-	if numPoints > geo.MaxAllowedSplitPoints {
-		return nil, errors.Newf(
-			"attempting to segmentize into too many coordinates; need %d points between %v and %v, max %d",
-			numPoints,
-			a,
-			b,
-			geo.MaxAllowedSplitPoints,
-		)
-	} // segmentFraction represent the fraction of length each segment
+	numberOfSegmentsToCreate := int(doubleNumberOfSegmentsToCreate)
+	numPoints := int(doubleNumPoints)
+	// segmentFraction represent the fraction of length each segment
 	// has with respect to total length between two coordinates.
-	allSegmentizedCoordinates := make([]float64, 0, 2*(1+numberOfSegmentsToCreate))
+	allSegmentizedCoordinates := make([]float64, 0, numPoints)
 	allSegmentizedCoordinates = append(allSegmentizedCoordinates, a.Clone()...)
 	segmentFraction := 1.0 / float64(numberOfSegmentsToCreate)
 	for pointInserted := 1; pointInserted < numberOfSegmentsToCreate; pointInserted++ {

pkg/geo/geomfn/segmentize_test.go

@@ -155,13 +155,6 @@ func TestSegmentizeCoords(t *testing.T) {
 			segmentMaxLength:     0.49999999999999,
 			resultantCoordinates: []float64{0, 0, 0.3333333333333333, 0, 0.6666666666666666, 0},
 		},
-		{
-			desc:                 `Coordinate(1, 1) to Coordinate(0, 0), -1`,
-			a:                    geom.Coord{1, 1},
-			b:                    geom.Coord{0, 0},
-			segmentMaxLength:     -1,
-			resultantCoordinates: []float64{1, 1},
-		},
 		{
 			desc:                 `Coordinate(1, 1) to Coordinate(0, 0), 2`,
 			a:                    geom.Coord{1, 1},
@@ -185,6 +178,35 @@ func TestSegmentizeCoords(t *testing.T) {
 		})
 	}
 
+	errorTestCases := []struct {
+		desc             string
+		a                geom.Coord
+		b                geom.Coord
+		segmentMaxLength float64
+		expectedErr      string
+	}{
+		{
+			desc:             "too many segments required",
+			a:                geom.Coord{0, 0},
+			b:                geom.Coord{100, 100},
+			segmentMaxLength: 0.001,
+			expectedErr:      fmt.Sprintf("attempting to segmentize into too many coordinates; need 282846 points between [0 0] and [100 100], max %d", geo.MaxAllowedSplitPoints),
+		},
+		{
+			desc:             "negative max segment length",
+			a:                geom.Coord{1, 1},
+			b:                geom.Coord{2, 2},
+			segmentMaxLength: -1,
+			expectedErr:      "maximum segment length must be positive",
+		},
+	}
+	for _, test := range errorTestCases {
+		t.Run(test.desc, func(t *testing.T) {
+			_, err := segmentizeCoords(test.a, test.b, test.segmentMaxLength)
+			require.EqualError(t, err, test.expectedErr)
+		})
+	}
+
 	t.Run("many coordinates to segmentize", func(t *testing.T) {
 		g := geo.MustParseGeometry("LINESTRING(0 0, 100 100)")
 		_, err := Segmentize(g, 0.001)

pkg/geo/geosegmentize/geosegmentize.go

@@ -11,6 +11,10 @@
 package geosegmentize
 
 import (
+	"strconv"
+	"strings"
+
+	"github.com/cockroachdb/cockroach/pkg/geo"
 	"github.com/cockroachdb/errors"
 	"github.com/twpayne/go-geom"
 )
@@ -121,3 +125,18 @@ func Segmentize(
 	}
 	return nil, errors.Newf("unknown type: %T", geometry)
 }
+
+// CheckSegmentizeTooManyPoints checks whether segmentize would break down into
+// to many points.
+func CheckSegmentizeTooManyPoints(numPoints float64, a geom.Coord, b geom.Coord) error {
+	if numPoints > float64(geo.MaxAllowedSplitPoints) {
+		return errors.Newf(
+			"attempting to segmentize into too many coordinates; need %s points between %v and %v, max %d",
+			strings.TrimRight(strconv.FormatFloat(numPoints, 'f', -1, 64), "."),
+			a,
+			b,
+			geo.MaxAllowedSplitPoints,
+		)
+	}
+	return nil
+}