Implement ST_Area for SphericalGeography

[ochalouhi]

See https://www.movable-type.co.uk/scripts/latlong.html
and http://osgeo-org.1560.x6.nabble.com/Area-of-a-spherical-polygon-td3841625.html
and https://www.element84.com/blog/determining-if-a-spherical-polygon-contains-a-pole
for the underlying Maths

[mbasmanova]

@ochalouhi Olivier, thank you for adding this function. I have some initial comments below.

[mbasmanova]

@ochalouhi Olivier, this is pretty cool. I took a first pass over the function implementation and test and have some comments. I haven't looked at the benchmark and documentation yet.

[mbasmanova]

CC: @jwass @jagill

[mbasmanova]

Some more comments.

[mbasmanova]

In general, in the Presto codebase it is preferable to not add future looking functionality until that future arrives.

[mbasmanova]

I'm not convinced that this check makes sense. Also, why hasProcessedPoint is set to true when this check fails? Valid geometries don't have identical vertexes, hence, this check should not matter as long as input is valid.

[ochalouhi]

We have 3 options :

1. Throw an exception because we assume that this is an invalid polygon
2. Return the wrong result because it's an invalid polygon
3. Filter it out, and return the right answer
   I've picked Add basic UncompressedColumnInput #3 but I'm open to any other strategy, I just want to make sure that it's a conscious decision

[ochalouhi]

Postgis does not seem to care about duplicate points (it does not report an error)

[mbasmanova]

@ochalouhi #1 is the preferred option because it is consistent with other functions and the SQL specification.

[mbasmanova]

Indeed, PostGIS doesn't seem to comply with the SQL specification which calls for errors to be raised on invalid input. Unlike PostGIS, Presto aims for full compliance.

[mbasmanova]

I think this check should be removed, but if it stays, the code would be easier to read if it was rearranged like so:

if (<same-point>) {
   return;
}

<process the point>

[ochalouhi]

I will get rid of it.
Wouldn't it be better to check at polygon creation time ? This way, each method doing computations on a polygon could assume it is valid

[mbasmanova]

@ochalouhi Validity checks are very expensive, hence, we don't include them by default.

[mbasmanova]

@ochalouhi Thanks for fixing the OH and PA polygons. Would you add a note about this to the commit message? This would allow future maintainers to understand this change without looking too closely. The commit message also needs to include the results of the benchmark. Having these will allow for determining whether there is a performance diff later without reverting to this state and re-running the benchmark.

[mbasmanova]

Math.abs call seems unnecessary here.

[mbasmanova]

Indeed, PostGIS doesn't seem to comply with the SQL specification which calls for errors to be raised on invalid input. Unlike PostGIS, Presto aims for full compliance.

[mbasmanova]

I think this check should be removed, but if it stays, the code would be easier to read if it was rearranged like so:

if (<same-point>) {
   return;
}

<process the point>

[mbasmanova]

This logic mutates internal state and makes it illegal to invoke getSphericalExcess multiple times. Perhaps, add a boolean to enforce that getSphericalExcess is called only once and add is never called after getSphericalExcess? It might be clearer to rename getSphericalExcess to computeSphericalExcess.

[mbasmanova]

@ochalouhi Looks good to me % one comment.

[mbasmanova]

@ochalouhi Olivier, thank you for the contribution.