diff --git a/pkg/geo/geogfn/segmentize.go b/pkg/geo/geogfn/segmentize.go index 1fd3586c1b23..b557c23a6919 100644 --- a/pkg/geo/geogfn/segmentize.go +++ b/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++ { diff --git a/pkg/geo/geogfn/segmentize_test.go b/pkg/geo/geogfn/segmentize_test.go index 36e8fa6fc04b..1a8fcf89648e 100644 --- a/pkg/geo/geogfn/segmentize_test.go +++ b/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) + }) + } } diff --git a/pkg/geo/geomfn/segmentize.go b/pkg/geo/geomfn/segmentize.go index 92a1621ebab3..86720d25b384 100644 --- a/pkg/geo/geomfn/segmentize.go +++ b/pkg/geo/geomfn/segmentize.go @@ -12,8 +12,6 @@ package geomfn import ( "math" - "strconv" - "strings" "github.com/cockroachdb/cockroach/pkg/geo" "github.com/cockroachdb/cockroach/pkg/geo/geosegmentize" @@ -53,18 +51,15 @@ 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 doubleNumPoints > float64(geo.MaxAllowedSplitPoints) { - return nil, errors.Newf( - "attempting to segmentize into too many coordinates; need %s points between %v and %v, max %d", - strings.TrimRight(strconv.FormatFloat(doubleNumPoints, 'f', -1, 64), "."), - a, - b, - geo.MaxAllowedSplitPoints, - ) + if err := geosegmentize.CheckSegmentizeTooManyPoints(doubleNumPoints, a, b); err != nil { + return nil, err } // numberOfSegmentsToCreate represent the total number of segments diff --git a/pkg/geo/geomfn/segmentize_test.go b/pkg/geo/geomfn/segmentize_test.go index a6756552874a..9b31e45b16bb 100644 --- a/pkg/geo/geomfn/segmentize_test.go +++ b/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) diff --git a/pkg/geo/geosegmentize/geosegmentize.go b/pkg/geo/geosegmentize/geosegmentize.go index fb31a6b5259e..28bff8d5549a 100644 --- a/pkg/geo/geosegmentize/geosegmentize.go +++ b/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 +}