builtins: implement ST_Intersection and ST_Buffer for geography types #51537
Conversation
|
This change is |
otan
force-pushed
the
best_srid
branch
7 times, most recently
from
a595035 to
5afc11e
Compare
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @otan)
pkg/geo/geogfn/best_projection.go, line 31 at r1 (raw file):
if geography objects are within one half zone UTM but not same UTM will pick one of those
I didn't understand this phrasing.
pkg/geo/geogfn/best_projection.go, line 41 at r1 (raw file):
// Check if these fit either the North Pole or South Pole areas. // If the center of each is greater than 70, and it is within 45 degrees, we can use
would be clearer as:
// If the center has latitude greater than 70 degrees, and length within 45 degrees, we can use ...
Are these 45 and 70 constants used to match the behavior of PostGIS or is there a deeper reason?
pkg/geo/geogfn/best_projection.go, line 54 at r1 (raw file):
} // Attempt to fit the geometries into one UTM zone, which is 6 degrees wide each.
How about:
// Each UTM zone is 6 degrees wide and distortion is low for geometries that fit within the zone. We use
// UTM if the width is lower than the UTM zone width, even though the geometry may span 2 zones -- using
// the geometry center to pick the UTM zone should result in most of the geometry being in the picked zone.
pkg/geo/geogfn/best_projection.go, line 59 at r1 (raw file):
// Offset longitude -180 to 0 and divide by 6 to get the zone. // Note that we treat 180 degree longtitudes as offset 59. sridOffset := geopb.SRID(math.Min(math.Floor((center.Lng.Degrees()+180)/6), 59))
What about the exceptions https://en.wikipedia.org/wiki/Universal_Transverse_Mercator_coordinate_system#Exceptions ?
pkg/geo/geogfn/best_projection.go, line 64 at r1 (raw file):
return getGeomProjection(32601 + sridOffset) } // Start at the south UTM SRID.
Hmm, so the geometry could span 4 zones.
Though the purpose of the S and N zone distinction is unclear to me -- it doesn't seem to be motivated by distortion. Do you know why they are distinct?
pkg/sql/sem/builtins/geo_builtins.go, line 342 at r1 (raw file):
return nil, err } return retransformedGeom.AsGeography()
naming nit: how about inLatLonGeom, inProjectedGeom, outProjectedGeom, outLatLonGeom instead of geom, transformedGeom, appliedGeom, retransformedGeom
There was a problem hiding this comment.
Reviewable status:
pkg/geo/geogfn/best_projection.go, line 31 at r1 (raw file):
Previously, sumeerbhola wrote…
if geography objects are within one half zone UTM but not same UTM will pick one of thoseI didn't understand this phrasing.
Fixed it up.
pkg/geo/geogfn/best_projection.go, line 41 at r1 (raw file):
Previously, sumeerbhola wrote…
would be clearer as:
// If the center has latitude greater than 70 degrees, and length within 45 degrees, we can use ...Are these 45 and 70 constants used to match the behavior of PostGIS or is there a deeper reason?
I was roughly following PostGIS, but also modified this logic such that 45 makes a lot more sense.
pkg/geo/geogfn/best_projection.go, line 54 at r1 (raw file):
Previously, sumeerbhola wrote…
How about:
// Each UTM zone is 6 degrees wide and distortion is low for geometries that fit within the zone. We use
// UTM if the width is lower than the UTM zone width, even though the geometry may span 2 zones -- using
// the geometry center to pick the UTM zone should result in most of the geometry being in the picked zone.
Done.
pkg/geo/geogfn/best_projection.go, line 59 at r1 (raw file):
Previously, sumeerbhola wrote…
What about the exceptions https://en.wikipedia.org/wiki/Universal_Transverse_Mercator_coordinate_system#Exceptions ?
I don't think we care too much about it, so I've ignored it but added a comment.
pkg/geo/geogfn/best_projection.go, line 64 at r1 (raw file):
Previously, sumeerbhola wrote…
Hmm, so the geometry could span 4 zones.
Though the purpose of the S and N zone distinction is unclear to me -- it doesn't seem to be motivated by distortion. Do you know why they are distinct?
Usage of false northings -- see http://help.arcgis.com/en/geodatabase/10.0/sdk/arcsde/concepts/geometry/coordref/coordsys/projected/mapprojections.htm.
I'm not sure it makes a difference in practice though.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status:
pkg/geo/geogfn/best_projection.go, line 30 at r2 (raw file):
// The algorithm is described by ST_Buffer documentation: // It first determines the best SRID that fits the bounding box of the 2 // geography objects ... if Geography objects span multiple zones, it will pick one
do you mean UTM zones? But the rest of the sentence talks about both UTM and LAEA. This would be easier to understand if the sentence didn't mix the two.
pkg/geo/geogfn/best_projection.go, line 41 at r2 (raw file):
// Check if these fit either the North Pole or South Pole areas. // If the center has latitude than 70 (an arbitrary polar threshold), and it is
... has latitude greater than 70 ...
pkg/geo/geogfn/best_projection.go, line 49 at r2 (raw file):
https://epsg.io/3574.
this says [-90, 0] for latitude and doesn't mention LAEA -- did you mean to add a different link? I couldn't find a "South Pole LAEA" -- perhaps it does not exist. Which is fine -- we can just say in a comment here that we reused the same idea as the northern hemisphere.
There was a problem hiding this comment.
Reviewable status:
pkg/geo/geogfn/best_projection.go, line 30 at r2 (raw file):
Previously, sumeerbhola wrote…
do you mean UTM zones? But the rest of the sentence talks about both UTM and LAEA. This would be easier to understand if the sentence didn't mix the two.
Done.
pkg/geo/geogfn/best_projection.go, line 41 at r2 (raw file):
Previously, sumeerbhola wrote…
... has latitude greater than 70 ...
Done.
pkg/geo/geogfn/best_projection.go, line 49 at r2 (raw file):
Previously, sumeerbhola wrote…
https://epsg.io/3574.this says [-90, 0] for latitude and doesn't mention LAEA -- did you mean to add a different link? I couldn't find a "South Pole LAEA" -- perhaps it does not exist. Which is fine -- we can just say in a comment here that we reused the same idea as the northern hemisphere.
Done.
pkg/sql/sem/builtins/geo_builtins.go, line 342 at r1 (raw file):
Previously, sumeerbhola wrote…
naming nit: how about
inLatLonGeom, inProjectedGeom, outProjectedGeom, outLatLonGeominstead ofgeom, transformedGeom, appliedGeom, retransformedGeom
Done.
PostGIS uses _ST_BestSRID to approximate a geography as a geometry to perform ST_Buffer and ST_Intersection. We're obliged to do the same - comments are inline. Release note (sql change): Implements the ST_Buffer and ST_Intersection functions for Geography types. Release note (sql change): Implement ST_Intersection for string types.
|
bors r=sumeerbhola |
|
Build succeeded: |
PostGIS uses _ST_BestSRID to approximate a geography as a geometry to
perform ST_Buffer and ST_Intersection. We're obliged to do the same -
comments are inline.
Resolves #48812
Resolves #48389
Resolves #48390
Resolves #48391
Resolves #48398
Release note (sql change): Implements the ST_Buffer and ST_Intersection
functions for Geography types.
Release note (sql change): Implement ST_Intersection for string types.