From c5abb54e0e367aa8de7fa963ea4e406e9fdf6d4a Mon Sep 17 00:00:00 2001 From: Aditya Maru Date: Wed, 27 Apr 2022 17:26:21 -0400 Subject: [PATCH 1/4] gcp,s3,azure: make the storage client upload chunk size configurable This change adds a `cloudstorage.write_chunk_size` cluster setting that allows us to control the size of the chunks buffered by the cloud storage client when uploading a file to storage. The setting defaults to 8MiB. Prior to this change gcs used a 16MB buffer, s3 a 5MB buffer, and azure a 4MB buffer. A follow up change will add memory monitoring to each external storage writer to account for these buffered chunks during upload. This change was motivated by the fact that in google-cloud-storage SDK versions prior to v1.21.0 every chunk is given a hardcoded timeout of 32s to successfully upload to storage. This includes retries due to transient errors. If any chunk during a backup were to hit this timeout the entire backup would fail. We have additional work to do to make the job more resilient to such failures, but dropping the default chunk size might mean we see fewer chunks hit their timeouts. Release note: None --- pkg/cloud/amazon/s3_storage.go | 4 +++- pkg/cloud/azure/azure_storage.go | 2 +- pkg/cloud/cloud_io.go | 9 +++++++++ pkg/cloud/gcp/gcs_storage.go | 1 + 4 files changed, 14 insertions(+), 2 deletions(-) diff --git a/pkg/cloud/amazon/s3_storage.go b/pkg/cloud/amazon/s3_storage.go index 6f07f1fb4b8d..696cb4f82f70 100644 --- a/pkg/cloud/amazon/s3_storage.go +++ b/pkg/cloud/amazon/s3_storage.go @@ -352,7 +352,9 @@ func newClient( sess.Config.Region = aws.String(region) c := s3.New(sess) - u := s3manager.NewUploader(sess) + u := s3manager.NewUploader(sess, func(uploader *s3manager.Uploader) { + uploader.PartSize = cloud.WriteChunkSize.Get(&settings.SV) + }) return s3Client{client: c, uploader: u}, region, nil } diff --git a/pkg/cloud/azure/azure_storage.go b/pkg/cloud/azure/azure_storage.go index 310124458493..bc659d8ba134 100644 --- a/pkg/cloud/azure/azure_storage.go +++ b/pkg/cloud/azure/azure_storage.go @@ -125,7 +125,7 @@ func (s *azureStorage) Writer(ctx context.Context, basename string) (io.WriteClo defer sp.Finish() _, err := azblob.UploadStreamToBlockBlob( ctx, r, blob, azblob.UploadStreamToBlockBlobOptions{ - BufferSize: 4 << 20, + BufferSize: int(cloud.WriteChunkSize.Get(&s.settings.SV)), }, ) return err diff --git a/pkg/cloud/cloud_io.go b/pkg/cloud/cloud_io.go index e452d89d85bc..191d58c78511 100644 --- a/pkg/cloud/cloud_io.go +++ b/pkg/cloud/cloud_io.go @@ -49,6 +49,15 @@ var httpCustomCA = settings.RegisterStringSetting( "", ).WithPublic() +// WriteChunkSize is used to control the size of each chunk that is buffered and +// uploaded by the cloud storage client. +var WriteChunkSize = settings.RegisterByteSizeSetting( + settings.TenantWritable, + "cloudstorage.write_chunk.size", + "controls the size of each file chunk uploaded by the cloud storage client", + 8<<20, +) + // HTTPRetryOptions defines the tunable settings which control the retry of HTTP // operations. var HTTPRetryOptions = retry.Options{ diff --git a/pkg/cloud/gcp/gcs_storage.go b/pkg/cloud/gcp/gcs_storage.go index 2bd8bf7233ea..3ce1d6c9c83f 100644 --- a/pkg/cloud/gcp/gcs_storage.go +++ b/pkg/cloud/gcp/gcs_storage.go @@ -162,6 +162,7 @@ func (g *gcsStorage) Writer(ctx context.Context, basename string) (io.WriteClose path.Join(g.prefix, basename))}) w := g.bucket.Object(path.Join(g.prefix, basename)).NewWriter(ctx) + w.ChunkSize = int(cloud.WriteChunkSize.Get(&g.settings.SV)) if !gcsChunkingEnabled.Get(&g.settings.SV) { w.ChunkSize = 0 } From b8085a363b1a3bf5450718c80c56c598d79e3758 Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Tue, 3 May 2022 14:17:13 +1000 Subject: [PATCH 2/4] geo: remove dependency on geographiclib geographiclib should only be understood by geogfn, so removing all references there. Release note: None --- pkg/geo/BUILD.bazel | 1 - pkg/geo/geo.go | 10 ---------- pkg/geo/geogfn/azimuth.go | 2 +- pkg/geo/geogfn/distance.go | 2 +- pkg/geo/geogfn/dwithin.go | 2 +- pkg/geo/geogfn/geographiclib.go | 11 +++++++++++ pkg/geo/geogfn/segmentize.go | 2 +- pkg/geo/geogfn/topology_operations.go | 2 +- pkg/geo/geogfn/unary_operators.go | 8 ++++---- 9 files changed, 20 insertions(+), 20 deletions(-) diff --git a/pkg/geo/BUILD.bazel b/pkg/geo/BUILD.bazel index 4b15786640a3..9ef9540a07cf 100644 --- a/pkg/geo/BUILD.bazel +++ b/pkg/geo/BUILD.bazel @@ -17,7 +17,6 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/geo", visibility = ["//visibility:public"], deps = [ - "//pkg/geo/geographiclib", "//pkg/geo/geopb", "//pkg/geo/geoprojbase", "//pkg/sql/pgwire/pgcode", diff --git a/pkg/geo/geo.go b/pkg/geo/geo.go index 98786b6686c4..c3c6227b2be8 100644 --- a/pkg/geo/geo.go +++ b/pkg/geo/geo.go @@ -16,7 +16,6 @@ import ( "encoding/binary" "math" - "github.com/cockroachdb/cockroach/pkg/geo/geographiclib" "github.com/cockroachdb/cockroach/pkg/geo/geopb" "github.com/cockroachdb/cockroach/pkg/geo/geoprojbase" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" @@ -620,15 +619,6 @@ func (g *Geography) ShapeType2D() geopb.ShapeType { return g.ShapeType().To2D() } -// Spheroid returns the spheroid represented by the given Geography. -func (g *Geography) Spheroid() (*geographiclib.Spheroid, error) { - proj, err := geoprojbase.Projection(g.SRID()) - if err != nil { - return nil, err - } - return proj.Spheroid, nil -} - // AsS2 converts a given Geography into it's S2 form. func (g *Geography) AsS2(emptyBehavior EmptyBehavior) ([]s2.Region, error) { geomRepr, err := g.AsGeomT() diff --git a/pkg/geo/geogfn/azimuth.go b/pkg/geo/geogfn/azimuth.go index f758984ded24..4819a1b747a6 100644 --- a/pkg/geo/geogfn/azimuth.go +++ b/pkg/geo/geogfn/azimuth.go @@ -58,7 +58,7 @@ func Azimuth(a geo.Geography, b geo.Geography) (*float64, error) { return nil, nil } - s, err := a.Spheroid() + s, err := spheroidFromGeography(a) if err != nil { return nil, err } diff --git a/pkg/geo/geogfn/distance.go b/pkg/geo/geogfn/distance.go index 6b61ea42db7a..53b883629dc3 100644 --- a/pkg/geo/geogfn/distance.go +++ b/pkg/geo/geogfn/distance.go @@ -45,7 +45,7 @@ func Distance( if err != nil { return 0, err } - spheroid, err := a.Spheroid() + spheroid, err := spheroidFromGeography(a) if err != nil { return 0, err } diff --git a/pkg/geo/geogfn/dwithin.go b/pkg/geo/geogfn/dwithin.go index b3c523399ca6..13f1677c6fc7 100644 --- a/pkg/geo/geogfn/dwithin.go +++ b/pkg/geo/geogfn/dwithin.go @@ -34,7 +34,7 @@ func DWithin( if distance < 0 { return false, pgerror.Newf(pgcode.InvalidParameterValue, "dwithin distance cannot be less than zero") } - spheroid, err := a.Spheroid() + spheroid, err := spheroidFromGeography(a) if err != nil { return false, err } diff --git a/pkg/geo/geogfn/geographiclib.go b/pkg/geo/geogfn/geographiclib.go index a81e61f3c6a6..9a7cd5ee6ed3 100644 --- a/pkg/geo/geogfn/geographiclib.go +++ b/pkg/geo/geogfn/geographiclib.go @@ -11,7 +11,9 @@ package geogfn import ( + "github.com/cockroachdb/cockroach/pkg/geo" "github.com/cockroachdb/cockroach/pkg/geo/geographiclib" + "github.com/cockroachdb/cockroach/pkg/geo/geoprojbase" "github.com/golang/geo/s2" ) @@ -20,3 +22,12 @@ func spheroidDistance(s *geographiclib.Spheroid, a s2.Point, b s2.Point) float64 inv, _, _ := s.Inverse(s2.LatLngFromPoint(a), s2.LatLngFromPoint(b)) return inv } + +// spheroid returns the spheroid represented by the given Geography. +func spheroidFromGeography(g geo.Geography) (*geographiclib.Spheroid, error) { + proj, err := geoprojbase.Projection(g.SRID()) + if err != nil { + return nil, err + } + return proj.Spheroid, nil +} diff --git a/pkg/geo/geogfn/segmentize.go b/pkg/geo/geogfn/segmentize.go index e14c031c7018..b84308253a5e 100644 --- a/pkg/geo/geogfn/segmentize.go +++ b/pkg/geo/geogfn/segmentize.go @@ -40,7 +40,7 @@ func Segmentize(geography geo.Geography, segmentMaxLength float64) (geo.Geograph if segmentMaxLength <= 0 { return geo.Geography{}, pgerror.Newf(pgcode.InvalidParameterValue, "maximum segment length must be positive") } - spheroid, err := geography.Spheroid() + spheroid, err := spheroidFromGeography(geography) if err != nil { return geo.Geography{}, err } diff --git a/pkg/geo/geogfn/topology_operations.go b/pkg/geo/geogfn/topology_operations.go index 135ac896e774..b3bf34fc1947 100644 --- a/pkg/geo/geogfn/topology_operations.go +++ b/pkg/geo/geogfn/topology_operations.go @@ -54,7 +54,7 @@ func Centroid(g geo.Geography, useSphereOrSpheroid UseSphereOrSpheroid) (geo.Geo if err != nil { return geo.Geography{}, err } - spheroid, err := g.Spheroid() + spheroid, err := spheroidFromGeography(g) if err != nil { return geo.Geography{}, err } diff --git a/pkg/geo/geogfn/unary_operators.go b/pkg/geo/geogfn/unary_operators.go index b657ef6f8ad7..4205fdaa47ef 100644 --- a/pkg/geo/geogfn/unary_operators.go +++ b/pkg/geo/geogfn/unary_operators.go @@ -28,7 +28,7 @@ func Area(g geo.Geography, useSphereOrSpheroid UseSphereOrSpheroid) (float64, er if err != nil { return 0, err } - spheroid, err := g.Spheroid() + spheroid, err := spheroidFromGeography(g) if err != nil { return 0, err } @@ -74,7 +74,7 @@ func Perimeter(g geo.Geography, useSphereOrSpheroid UseSphereOrSpheroid) (float6 if err != nil { return 0, err } - spheroid, err := g.Spheroid() + spheroid, err := spheroidFromGeography(g) if err != nil { return 0, err } @@ -98,7 +98,7 @@ func Length(g geo.Geography, useSphereOrSpheroid UseSphereOrSpheroid) (float64, if err != nil { return 0, err } - spheroid, err := g.Spheroid() + spheroid, err := spheroidFromGeography(g) if err != nil { return 0, err } @@ -117,7 +117,7 @@ func Project(g geo.Geography, distance float64, azimuth s1.Angle) (geo.Geography return geo.Geography{}, pgerror.Newf(pgcode.InvalidParameterValue, "ST_Project(geography) is only valid for point inputs") } - spheroid, err := g.Spheroid() + spheroid, err := spheroidFromGeography(g) if err != nil { return geo.Geography{}, err } From d86d0f796d3ce9ea088ef0e73854ffddd287c2e3 Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Tue, 3 May 2022 14:38:59 +1000 Subject: [PATCH 3/4] geoprojbase: remove dependency on geographiclib This prevents an additional C dependency on geo that is not required. Release note: None --- pkg/cmd/generate-spatial-ref-sys/main.go | 8 ++--- pkg/geo/BUILD.bazel | 1 + pkg/geo/geo_test.go | 2 ++ pkg/geo/geogfn/BUILD.bazel | 2 +- pkg/geo/geogfn/distance.go | 14 ++++---- pkg/geo/geogfn/dwithin.go | 2 +- pkg/geo/geogfn/segmentize.go | 2 +- .../geogfn/{geographiclib.go => spheroid.go} | 7 ++-- pkg/geo/geogfn/unary_operators.go | 12 +++---- pkg/geo/geographiclib/BUILD.bazel | 1 + pkg/geo/geographiclib/geographiclib.go | 34 +++++++++++++++---- pkg/geo/geoindex/BUILD.bazel | 1 + pkg/geo/geoindex/geoindex.go | 2 ++ pkg/geo/geoindex/s2_geography_index.go | 2 +- pkg/geo/geomfn/BUILD.bazel | 1 + pkg/geo/geomfn/geomfn.go | 6 +++- pkg/geo/geoprojbase/BUILD.bazel | 10 ++++-- pkg/geo/geoprojbase/geoprojbase.go | 16 +++++++-- pkg/geo/geoprojbase/projections.go | 15 ++++++-- pkg/geo/geoprojbase/projections_test.go | 10 +++--- pkg/sql/opt/invertedidx/BUILD.bazel | 1 + pkg/sql/opt/invertedidx/geo.go | 4 ++- pkg/util/encoding/BUILD.bazel | 1 + pkg/util/encoding/encoding_test.go | 2 ++ 24 files changed, 112 insertions(+), 44 deletions(-) rename pkg/geo/geogfn/{geographiclib.go => spheroid.go} (76%) diff --git a/pkg/cmd/generate-spatial-ref-sys/main.go b/pkg/cmd/generate-spatial-ref-sys/main.go index 3481e5eb7218..01cd595c376e 100644 --- a/pkg/cmd/generate-spatial-ref-sys/main.go +++ b/pkg/cmd/generate-spatial-ref-sys/main.go @@ -101,12 +101,12 @@ func buildData() embeddedproj.Data { continue } - key := spheroidKey{s.Radius, s.Flattening} + key := spheroidKey{s.Radius(), s.Flattening()} mu.Lock() spheroidHash, ok := foundSpheroids[key] if !ok { shaBytes := sha256.Sum256([]byte( - strconv.FormatFloat(s.Radius, 'f', -1, 64) + "," + strconv.FormatFloat(s.Flattening, 'f', -1, 64), + strconv.FormatFloat(s.Radius(), 'f', -1, 64) + "," + strconv.FormatFloat(s.Flattening(), 'f', -1, 64), )) spheroidHash = 0 for _, b := range shaBytes[:6] { @@ -117,8 +117,8 @@ func buildData() embeddedproj.Data { d.Spheroids, embeddedproj.Spheroid{ Hash: spheroidHash, - Radius: s.Radius, - Flattening: s.Flattening, + Radius: s.Radius(), + Flattening: s.Flattening(), }, ) } diff --git a/pkg/geo/BUILD.bazel b/pkg/geo/BUILD.bazel index 9ef9540a07cf..ff48d6f7f9d0 100644 --- a/pkg/geo/BUILD.bazel +++ b/pkg/geo/BUILD.bazel @@ -53,6 +53,7 @@ go_test( ], embed = [":geo"], deps = [ + "//pkg/geo/geographiclib", "//pkg/geo/geopb", "@com_github_cockroachdb_errors//:errors", "@com_github_golang_geo//s2", diff --git a/pkg/geo/geo_test.go b/pkg/geo/geo_test.go index cbbbfd6c79b7..701102d6cf1a 100644 --- a/pkg/geo/geo_test.go +++ b/pkg/geo/geo_test.go @@ -17,6 +17,8 @@ import ( "strconv" "testing" + // Blank import so projections are initialized correctly. + _ "github.com/cockroachdb/cockroach/pkg/geo/geographiclib" "github.com/cockroachdb/cockroach/pkg/geo/geopb" "github.com/cockroachdb/errors" "github.com/golang/geo/s2" diff --git a/pkg/geo/geogfn/BUILD.bazel b/pkg/geo/geogfn/BUILD.bazel index 0d7283339d51..f9998a923f4e 100644 --- a/pkg/geo/geogfn/BUILD.bazel +++ b/pkg/geo/geogfn/BUILD.bazel @@ -9,9 +9,9 @@ go_library( "distance.go", "dwithin.go", "geogfn.go", - "geographiclib.go", "intersects.go", "segmentize.go", + "spheroid.go", "topology_operations.go", "unary_operators.go", ], diff --git a/pkg/geo/geogfn/distance.go b/pkg/geo/geogfn/distance.go index 53b883629dc3..3b8019ec1cba 100644 --- a/pkg/geo/geogfn/distance.go +++ b/pkg/geo/geogfn/distance.go @@ -15,7 +15,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/geo" "github.com/cockroachdb/cockroach/pkg/geo/geodist" - "github.com/cockroachdb/cockroach/pkg/geo/geographiclib" + "github.com/cockroachdb/cockroach/pkg/geo/geoprojbase" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/golang/geo/s1" @@ -24,7 +24,7 @@ import ( // SpheroidErrorFraction is an error fraction to compensate for using a sphere // to calculate the distance for what is actually a spheroid. The distance -// calculation has an error that is bounded by (2 * spheroid.Flattening)%. +// calculation has an error that is bounded by (2 * spheroid.flattening)%. // This 5% margin is pretty safe. const SpheroidErrorFraction = 0.05 @@ -196,7 +196,7 @@ func (c *s2GeodistEdgeCrosser) ChainCrossing(p geodist.Point) (bool, geodist.Poi // See distance_test.go for examples of the "truer" distance values. // Since we aim to be compatible with PostGIS, we adopt the same approach. func distanceGeographyRegions( - spheroid *geographiclib.Spheroid, + spheroid geoprojbase.Spheroid, useSphereOrSpheroid UseSphereOrSpheroid, aRegions []s2.Region, bRegions []s2.Region, @@ -244,7 +244,7 @@ func distanceGeographyRegions( // geographyMinDistanceUpdater finds the minimum distance using a sphere. // Methods will return early if it finds a minimum distance <= stopAfterLE. type geographyMinDistanceUpdater struct { - spheroid *geographiclib.Spheroid + spheroid geoprojbase.Spheroid useSphereOrSpheroid UseSphereOrSpheroid minEdge s2.Edge minD s1.ChordAngle @@ -257,7 +257,7 @@ var _ geodist.DistanceUpdater = (*geographyMinDistanceUpdater)(nil) // newGeographyMinDistanceUpdater returns a new geographyMinDistanceUpdater with the // correct arguments set up. func newGeographyMinDistanceUpdater( - spheroid *geographiclib.Spheroid, + spheroid geoprojbase.Spheroid, useSphereOrSpheroid UseSphereOrSpheroid, stopAfter float64, exclusivity geo.FnExclusivity, @@ -269,7 +269,7 @@ func newGeographyMinDistanceUpdater( // buffer for spheroid distances being slightly off. multiplier -= SpheroidErrorFraction } - stopAfterChordAngle := s1.ChordAngleFromAngle(s1.Angle(stopAfter * multiplier / spheroid.SphereRadius)) + stopAfterChordAngle := s1.ChordAngleFromAngle(s1.Angle(stopAfter * multiplier / spheroid.SphereRadius())) return &geographyMinDistanceUpdater{ spheroid: spheroid, minD: math.MaxFloat64, @@ -288,7 +288,7 @@ func (u *geographyMinDistanceUpdater) Distance() float64 { if u.useSphereOrSpheroid == UseSpheroid { return spheroidDistance(u.spheroid, u.minEdge.V0, u.minEdge.V1) } - return u.minD.Angle().Radians() * u.spheroid.SphereRadius + return u.minD.Angle().Radians() * u.spheroid.SphereRadius() } // Update implements the geodist.DistanceUpdater interface. diff --git a/pkg/geo/geogfn/dwithin.go b/pkg/geo/geogfn/dwithin.go index 13f1677c6fc7..26639f594923 100644 --- a/pkg/geo/geogfn/dwithin.go +++ b/pkg/geo/geogfn/dwithin.go @@ -39,7 +39,7 @@ func DWithin( return false, err } - angleToExpand := s1.Angle(distance / spheroid.SphereRadius) + angleToExpand := s1.Angle(distance / spheroid.SphereRadius()) if useSphereOrSpheroid == UseSpheroid { angleToExpand *= (1 + SpheroidErrorFraction) } diff --git a/pkg/geo/geogfn/segmentize.go b/pkg/geo/geogfn/segmentize.go index b84308253a5e..d956e51b83eb 100644 --- a/pkg/geo/geogfn/segmentize.go +++ b/pkg/geo/geogfn/segmentize.go @@ -46,7 +46,7 @@ func Segmentize(geography geo.Geography, segmentMaxLength float64) (geo.Geograph } // Convert segmentMaxLength to Angle with respect to earth sphere as // further calculation is done considering segmentMaxLength as Angle. - segmentMaxAngle := segmentMaxLength / spheroid.SphereRadius + segmentMaxAngle := segmentMaxLength / spheroid.SphereRadius() ret, err := geosegmentize.Segmentize(geometry, segmentMaxAngle, segmentizeCoords) if err != nil { return geo.Geography{}, err diff --git a/pkg/geo/geogfn/geographiclib.go b/pkg/geo/geogfn/spheroid.go similarity index 76% rename from pkg/geo/geogfn/geographiclib.go rename to pkg/geo/geogfn/spheroid.go index 9a7cd5ee6ed3..348190b43541 100644 --- a/pkg/geo/geogfn/geographiclib.go +++ b/pkg/geo/geogfn/spheroid.go @@ -12,19 +12,20 @@ package geogfn import ( "github.com/cockroachdb/cockroach/pkg/geo" - "github.com/cockroachdb/cockroach/pkg/geo/geographiclib" + // Blank import so projections are initialized correctly. + _ "github.com/cockroachdb/cockroach/pkg/geo/geographiclib" "github.com/cockroachdb/cockroach/pkg/geo/geoprojbase" "github.com/golang/geo/s2" ) // spheroidDistance returns the s12 (meter) component of spheroid.Inverse from s2 Points. -func spheroidDistance(s *geographiclib.Spheroid, a s2.Point, b s2.Point) float64 { +func spheroidDistance(s geoprojbase.Spheroid, a s2.Point, b s2.Point) float64 { inv, _, _ := s.Inverse(s2.LatLngFromPoint(a), s2.LatLngFromPoint(b)) return inv } // spheroid returns the spheroid represented by the given Geography. -func spheroidFromGeography(g geo.Geography) (*geographiclib.Spheroid, error) { +func spheroidFromGeography(g geo.Geography) (geoprojbase.Spheroid, error) { proj, err := geoprojbase.Projection(g.SRID()) if err != nil { return nil, err diff --git a/pkg/geo/geogfn/unary_operators.go b/pkg/geo/geogfn/unary_operators.go index 4205fdaa47ef..0faf5e8cfd63 100644 --- a/pkg/geo/geogfn/unary_operators.go +++ b/pkg/geo/geogfn/unary_operators.go @@ -14,7 +14,7 @@ import ( "math" "github.com/cockroachdb/cockroach/pkg/geo" - "github.com/cockroachdb/cockroach/pkg/geo/geographiclib" + "github.com/cockroachdb/cockroach/pkg/geo/geoprojbase" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/golang/geo/s1" @@ -52,7 +52,7 @@ func Area(g geo.Geography, useSphereOrSpheroid UseSphereOrSpheroid) (float64, er } } if useSphereOrSpheroid == UseSphere { - totalArea *= spheroid.SphereRadius * spheroid.SphereRadius + totalArea *= spheroid.SphereRadius() * spheroid.SphereRadius() } return totalArea, nil } @@ -132,8 +132,8 @@ func Project(g geo.Geography, distance float64, azimuth s1.Angle) (geo.Geography azimuth = azimuth.Normalized() // Check the distance validity. - if distance > (math.Pi * spheroid.Radius) { - return geo.Geography{}, pgerror.Newf(pgcode.InvalidParameterValue, "distance must not be greater than %f", math.Pi*spheroid.Radius) + if distance > (math.Pi * spheroid.Radius()) { + return geo.Geography{}, pgerror.Newf(pgcode.InvalidParameterValue, "distance must not be greater than %f", math.Pi*spheroid.Radius()) } if point.Empty() { @@ -163,7 +163,7 @@ func Project(g geo.Geography, distance float64, azimuth s1.Angle) (geo.Geography // length returns the sum of the lengths and perimeters in the shapes of the Geography. // In OGC parlance, length returns both LineString lengths _and_ Polygon perimeters. func length( - regions []s2.Region, spheroid *geographiclib.Spheroid, useSphereOrSpheroid UseSphereOrSpheroid, + regions []s2.Region, spheroid geoprojbase.Spheroid, useSphereOrSpheroid UseSphereOrSpheroid, ) (float64, error) { var totalLength float64 for _, region := range regions { @@ -194,7 +194,7 @@ func length( } } if useSphereOrSpheroid == UseSphere { - totalLength *= spheroid.SphereRadius + totalLength *= spheroid.SphereRadius() } return totalLength, nil } diff --git a/pkg/geo/geographiclib/BUILD.bazel b/pkg/geo/geographiclib/BUILD.bazel index a0d9cd241e1a..3cbc36218bef 100644 --- a/pkg/geo/geographiclib/BUILD.bazel +++ b/pkg/geo/geographiclib/BUILD.bazel @@ -15,6 +15,7 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/geo/geographiclib", visibility = ["//visibility:public"], deps = [ + "//pkg/geo/geoprojbase", "@com_github_golang_geo//s1", "@com_github_golang_geo//s2", ], diff --git a/pkg/geo/geographiclib/geographiclib.go b/pkg/geo/geographiclib/geographiclib.go index efc0a973454e..304b5b9a380f 100644 --- a/pkg/geo/geographiclib/geographiclib.go +++ b/pkg/geo/geographiclib/geographiclib.go @@ -21,10 +21,17 @@ import "C" import ( "math" + "github.com/cockroachdb/cockroach/pkg/geo/geoprojbase" "github.com/golang/geo/s1" "github.com/golang/geo/s2" ) +func init() { + geoprojbase.MakeSpheroid = func(radius, flattening float64) (geoprojbase.Spheroid, error) { + return NewSpheroid(radius, flattening), nil + } +} + var ( // WGS84Spheroid represents the default WGS84 ellipsoid. WGS84Spheroid = NewSpheroid(6378137, 1/298.257223563) @@ -34,23 +41,38 @@ var ( // on a given spheroid. type Spheroid struct { cRepr C.struct_geod_geodesic - Radius float64 - Flattening float64 - SphereRadius float64 + radius float64 + flattening float64 + sphereRadius float64 } // NewSpheroid creates a spheroid from a radius and flattening. func NewSpheroid(radius float64, flattening float64) *Spheroid { minorAxis := radius - radius*flattening s := &Spheroid{ - Radius: radius, - Flattening: flattening, - SphereRadius: (radius*2 + minorAxis) / 3, + radius: radius, + flattening: flattening, + sphereRadius: (radius*2 + minorAxis) / 3, } C.geod_init(&s.cRepr, C.double(radius), C.double(flattening)) return s } +// Radius returns the radius of the spheroid. +func (s *Spheroid) Radius() float64 { + return s.radius +} + +// Flattening returns the flattening factor of the spheroid. +func (s *Spheroid) Flattening() float64 { + return s.flattening +} + +// SphereRadius returns the radius of a sphere that fits inside the spheroid. +func (s *Spheroid) SphereRadius() float64 { + return s.sphereRadius +} + // Inverse solves the geodetic inverse problem on the given spheroid // (https://en.wikipedia.org/wiki/Geodesy#Geodetic_problems). // Returns s12 (distance in meters), az1 (azimuth at point 1) and az2 (azimuth at point 2). diff --git a/pkg/geo/geoindex/BUILD.bazel b/pkg/geo/geoindex/BUILD.bazel index 74909d01c2bd..8773517411a0 100644 --- a/pkg/geo/geoindex/BUILD.bazel +++ b/pkg/geo/geoindex/BUILD.bazel @@ -16,6 +16,7 @@ go_library( deps = [ "//pkg/geo", "//pkg/geo/geogfn", + "//pkg/geo/geographiclib", "//pkg/geo/geomfn", "//pkg/geo/geopb", "//pkg/geo/geoprojbase", diff --git a/pkg/geo/geoindex/geoindex.go b/pkg/geo/geoindex/geoindex.go index 044f41ee833b..eb61ab4edea8 100644 --- a/pkg/geo/geoindex/geoindex.go +++ b/pkg/geo/geoindex/geoindex.go @@ -19,6 +19,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/geo" "github.com/cockroachdb/cockroach/pkg/geo/geogfn" + // Blank import so projections are initialized correctly. + _ "github.com/cockroachdb/cockroach/pkg/geo/geographiclib" "github.com/cockroachdb/cockroach/pkg/geo/geopb" "github.com/golang/geo/s2" "github.com/twpayne/go-geom" diff --git a/pkg/geo/geoindex/s2_geography_index.go b/pkg/geo/geoindex/s2_geography_index.go index f76487937358..4b2552c62256 100644 --- a/pkg/geo/geoindex/s2_geography_index.go +++ b/pkg/geo/geoindex/s2_geography_index.go @@ -186,7 +186,7 @@ func (i *s2GeographyIndex) DWithin( // error. multiplier += geogfn.SpheroidErrorFraction } - angle := s1.Angle(multiplier * distanceMeters / projInfo.Spheroid.SphereRadius) + angle := s1.Angle(multiplier * distanceMeters / projInfo.Spheroid.SphereRadius()) // maxLevelDiff puts a bound on the number of cells used after the expansion. // For example, we do not want expanding a large country by 1km to generate too // many cells. diff --git a/pkg/geo/geomfn/BUILD.bazel b/pkg/geo/geomfn/BUILD.bazel index 3088bef5b851..1ec5c3a5e6ad 100644 --- a/pkg/geo/geomfn/BUILD.bazel +++ b/pkg/geo/geomfn/BUILD.bazel @@ -45,6 +45,7 @@ go_library( deps = [ "//pkg/geo", "//pkg/geo/geodist", + "//pkg/geo/geographiclib", "//pkg/geo/geopb", "//pkg/geo/geos", "//pkg/geo/geosegmentize", diff --git a/pkg/geo/geomfn/geomfn.go b/pkg/geo/geomfn/geomfn.go index 29afdb348e51..26e2af786ce8 100644 --- a/pkg/geo/geomfn/geomfn.go +++ b/pkg/geo/geomfn/geomfn.go @@ -11,7 +11,11 @@ // Package geomfn contains functions that are used for geometry-based builtins. package geomfn -import "github.com/twpayne/go-geom" +import ( + // Blank import so projections are initialized correctly. + _ "github.com/cockroachdb/cockroach/pkg/geo/geographiclib" + "github.com/twpayne/go-geom" +) // applyCoordFunc applies a function on src to copy onto dst. // Both slices represent a single Coord within the FlatCoord array. diff --git a/pkg/geo/geoprojbase/BUILD.bazel b/pkg/geo/geoprojbase/BUILD.bazel index 8aa986769620..01cc26213c96 100644 --- a/pkg/geo/geoprojbase/BUILD.bazel +++ b/pkg/geo/geoprojbase/BUILD.bazel @@ -10,12 +10,13 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/geo/geoprojbase", visibility = ["//visibility:public"], deps = [ - "//pkg/geo/geographiclib", "//pkg/geo/geopb", "//pkg/geo/geoprojbase/embeddedproj", "//pkg/sql/pgwire/pgcode", "//pkg/sql/pgwire/pgerror", "@com_github_cockroachdb_errors//:errors", + "@com_github_golang_geo//s1", + "@com_github_golang_geo//s2", ], ) @@ -23,6 +24,9 @@ go_test( name = "geoprojbase_test", size = "small", srcs = ["projections_test.go"], - embed = [":geoprojbase"], - deps = ["@com_github_stretchr_testify//require"], + deps = [ + ":geoprojbase", + "//pkg/geo/geographiclib", + "@com_github_stretchr_testify//require", + ], ) diff --git a/pkg/geo/geoprojbase/geoprojbase.go b/pkg/geo/geoprojbase/geoprojbase.go index 0300ef2932bc..2a5e769852ec 100644 --- a/pkg/geo/geoprojbase/geoprojbase.go +++ b/pkg/geo/geoprojbase/geoprojbase.go @@ -17,11 +17,12 @@ import ( "bytes" "sort" - "github.com/cockroachdb/cockroach/pkg/geo/geographiclib" "github.com/cockroachdb/cockroach/pkg/geo/geopb" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/errors" + "github.com/golang/geo/s1" + "github.com/golang/geo/s2" ) // Proj4Text is the text representation of a PROJ4 transformation. @@ -79,7 +80,18 @@ type ProjInfo struct { // IsLatLng stores whether the projection is a LatLng based projection (denormalized from above) IsLatLng bool // The spheroid represented by the SRID. - Spheroid *geographiclib.Spheroid + Spheroid Spheroid +} + +// Spheroid represents a spheroid object. +type Spheroid interface { + Inverse(a, b s2.LatLng) (s12, az1, az2 float64) + InverseBatch(points []s2.Point) float64 + AreaAndPerimeter(points []s2.Point) (area float64, perimeter float64) + Project(point s2.LatLng, distance float64, azimuth s1.Angle) s2.LatLng + Radius() float64 + Flattening() float64 + SphereRadius() float64 } // ErrProjectionNotFound indicates a project was not found. diff --git a/pkg/geo/geoprojbase/projections.go b/pkg/geo/geoprojbase/projections.go index 39a92cd9abce..e8995a5097e4 100644 --- a/pkg/geo/geoprojbase/projections.go +++ b/pkg/geo/geoprojbase/projections.go @@ -17,7 +17,6 @@ import ( _ "embed" // required for go:embed "sync" - "github.com/cockroachdb/cockroach/pkg/geo/geographiclib" "github.com/cockroachdb/cockroach/pkg/geo/geopb" "github.com/cockroachdb/cockroach/pkg/geo/geoprojbase/embeddedproj" "github.com/cockroachdb/errors" @@ -29,6 +28,13 @@ var projData []byte var once sync.Once var projectionsInternal map[geopb.SRID]ProjInfo +// MakeSpheroid is an injectable function which creates a spheroid. +// If you hit the assertion here, you may want to blank import geographic lib, e.g. +// _ "github.com/cockroachdb/cockroach/pkg/geo/geographiclib". +var MakeSpheroid = func(radius, flattening float64) (Spheroid, error) { + return nil, errors.AssertionFailedf("MakeSpheroid not initialised") +} + // getProjections returns the mapping of SRID to projections. // Use the `Projection` function to obtain one. func getProjections() map[geopb.SRID]ProjInfo { @@ -39,9 +45,12 @@ func getProjections() map[geopb.SRID]ProjInfo { } // Build a temporary map of spheroids so we can look them up by hash. - spheroids := make(map[int64]*geographiclib.Spheroid, len(d.Spheroids)) + spheroids := make(map[int64]Spheroid, len(d.Spheroids)) for _, s := range d.Spheroids { - spheroids[s.Hash] = geographiclib.NewSpheroid(s.Radius, s.Flattening) + spheroids[s.Hash], err = MakeSpheroid(s.Radius, s.Flattening) + if err != nil { + panic(err) + } } projectionsInternal = make(map[geopb.SRID]ProjInfo, len(d.Projections)) diff --git a/pkg/geo/geoprojbase/projections_test.go b/pkg/geo/geoprojbase/projections_test.go index 25db77f1c2cf..52d60f82c1eb 100644 --- a/pkg/geo/geoprojbase/projections_test.go +++ b/pkg/geo/geoprojbase/projections_test.go @@ -8,19 +8,21 @@ // by the Apache License, Version 2.0, included in the file // licenses/APL.txt. -package geoprojbase +package geoprojbase_test import ( "strconv" "testing" + _ "github.com/cockroachdb/cockroach/pkg/geo/geographiclib" + "github.com/cockroachdb/cockroach/pkg/geo/geoprojbase" "github.com/stretchr/testify/require" ) func TestProjections(t *testing.T) { - for srid, proj := range getProjections() { - t.Run(strconv.Itoa(int(srid)), func(t *testing.T) { - require.NotEqual(t, Bounds{}, proj.Bounds) + for _, proj := range geoprojbase.AllProjections() { + t.Run(strconv.Itoa(int(proj.SRID)), func(t *testing.T) { + require.NotEqual(t, geoprojbase.Bounds{}, proj.Bounds) require.GreaterOrEqual(t, proj.Bounds.MaxX, proj.Bounds.MinX) require.GreaterOrEqual(t, proj.Bounds.MaxY, proj.Bounds.MinY) }) diff --git a/pkg/sql/opt/invertedidx/BUILD.bazel b/pkg/sql/opt/invertedidx/BUILD.bazel index 252452a2da11..faeabb48150b 100644 --- a/pkg/sql/opt/invertedidx/BUILD.bazel +++ b/pkg/sql/opt/invertedidx/BUILD.bazel @@ -12,6 +12,7 @@ go_library( deps = [ "//pkg/geo", "//pkg/geo/geogfn", + "//pkg/geo/geographiclib", "//pkg/geo/geoindex", "//pkg/geo/geopb", "//pkg/geo/geoprojbase", diff --git a/pkg/sql/opt/invertedidx/geo.go b/pkg/sql/opt/invertedidx/geo.go index aaf9ed02b5ce..d920d16007e8 100644 --- a/pkg/sql/opt/invertedidx/geo.go +++ b/pkg/sql/opt/invertedidx/geo.go @@ -16,6 +16,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/geo" "github.com/cockroachdb/cockroach/pkg/geo/geogfn" + // Blank import so projections are initialized correctly. + _ "github.com/cockroachdb/cockroach/pkg/geo/geographiclib" "github.com/cockroachdb/cockroach/pkg/geo/geoindex" "github.com/cockroachdb/cockroach/pkg/geo/geopb" "github.com/cockroachdb/cockroach/pkg/geo/geoprojbase" @@ -686,7 +688,7 @@ func (p *PreFilterer) PreFilter( if err != nil { return false, err } - angleToExpand := s1.Angle(distance / proj.Spheroid.SphereRadius) + angleToExpand := s1.Angle(distance / proj.Spheroid.SphereRadius()) if useSphereOrSpheroid == geogfn.UseSpheroid { angleToExpand *= (1 + geogfn.SpheroidErrorFraction) } diff --git a/pkg/util/encoding/BUILD.bazel b/pkg/util/encoding/BUILD.bazel index 713ab47efa2d..99edf4f85ed0 100644 --- a/pkg/util/encoding/BUILD.bazel +++ b/pkg/util/encoding/BUILD.bazel @@ -41,6 +41,7 @@ go_test( embed = [":encoding"], deps = [ "//pkg/geo", + "//pkg/geo/geographiclib", "//pkg/geo/geopb", "//pkg/roachpb", "//pkg/util/bitarray", diff --git a/pkg/util/encoding/encoding_test.go b/pkg/util/encoding/encoding_test.go index 723db2838a85..c91c39878734 100644 --- a/pkg/util/encoding/encoding_test.go +++ b/pkg/util/encoding/encoding_test.go @@ -22,6 +22,8 @@ import ( "github.com/cockroachdb/apd/v3" "github.com/cockroachdb/cockroach/pkg/geo" + // Blank import so projections are initialized correctly. + _ "github.com/cockroachdb/cockroach/pkg/geo/geographiclib" "github.com/cockroachdb/cockroach/pkg/geo/geopb" "github.com/cockroachdb/cockroach/pkg/util/bitarray" "github.com/cockroachdb/cockroach/pkg/util/duration" From a6f6d3843c1493562e432fb46b4daab1ae690d39 Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Tue, 3 May 2022 14:42:24 +1000 Subject: [PATCH 4/4] geo: disallow imports from geoproj / geographiclib Release note: None --- pkg/BUILD.bazel | 1 + pkg/geo/BUILD.bazel | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index 7aac01eea2af..f333f8f6a96c 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -118,6 +118,7 @@ ALL_TESTS = [ "//pkg/geo/geos:geos_test", "//pkg/geo/geotransform:geotransform_test", "//pkg/geo/twkb:twkb_test", + "//pkg/geo:geo_disallowed_imports_test", "//pkg/geo:geo_test", "//pkg/gossip:gossip_test", "//pkg/internal/client/requestbatcher:requestbatcher_test", diff --git a/pkg/geo/BUILD.bazel b/pkg/geo/BUILD.bazel index ff48d6f7f9d0..010d0b21834f 100644 --- a/pkg/geo/BUILD.bazel +++ b/pkg/geo/BUILD.bazel @@ -1,4 +1,5 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") +load("//pkg/testutils/buildutil:buildutil.bzl", "disallowed_imports_test") go_library( name = "geo", @@ -63,3 +64,11 @@ go_test( "@com_github_twpayne_go_geom//:go-geom", ], ) + +disallowed_imports_test( + "geo", + [ + "//pkg/geo/geographiclib", + "//pkg/geo/geoproj", + ], +)