Add ST_Length(SphericalGeography) #12552
Conversation
There was a problem hiding this comment.
@seanbarker Sean, thanks for implementing this function. I noticed that there are build failures. Looks like style check is reporting some issue. Would you take a look?
ST_Length is defined for both linestring and multi-linestring. Would you add multi-linestring support?
Some more comments inline.
presto-geospatial/src/main/java/com/facebook/presto/plugin/geospatial/GeoFunctions.java
Outdated
Show resolved
Hide resolved
...eospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestSphericalGeoFunctions.java
Outdated
Show resolved
Hide resolved
...eospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestSphericalGeoFunctions.java
Outdated
Show resolved
Hide resolved
...eospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestSphericalGeoFunctions.java
Outdated
Show resolved
Hide resolved
...eospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestSphericalGeoFunctions.java
Outdated
Show resolved
Hide resolved
...eospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestSphericalGeoFunctions.java
Outdated
Show resolved
Hide resolved
...eospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestSphericalGeoFunctions.java
Outdated
Show resolved
Hide resolved
...eospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestSphericalGeoFunctions.java
Outdated
Show resolved
Hide resolved
...eospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestSphericalGeoFunctions.java
Show resolved
Hide resolved
There was a problem hiding this comment.
@seanbarker Sean, thanks for the updates. Here are some additional comments.
| @Test | ||
| public void testLength() | ||
| { | ||
| double distance1 = 1365691.096620299; |
There was a problem hiding this comment.
distance1 and distance2 variables are used only once to compute length; it is clearer to use assign length to a pre-computed constant value.
| assertLength(lineString, expectedLength, 1e-4); | ||
| } | ||
|
|
||
| private void assertFunctionImprecise(String function, Double expected, double tolerance) |
There was a problem hiding this comment.
this function is used only once, let's inline it
| assertLength("LINESTRING (-122.41 37.77, -87.62 41.87, -71.05 42.36)", length); | ||
|
|
||
| // Path north pole -> south pole -> north pole should be roughly the circumference of the Earth | ||
| assertLength("LINESTRING (0.0 90.0, 0.0 -90.0, 0.0 90.0)", 4.0075e7, 1e-2); |
There was a problem hiding this comment.
Would it be possible to use the default tolerance here?
There was a problem hiding this comment.
The test fails with the default tolerance, but the difference in the output is only about 44 km. I can either change the estimate of earth's circumference and use the default tolerance or use the lower tolerance.
There was a problem hiding this comment.
Let's change the estimate.
...eospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestSphericalGeoFunctions.java
Show resolved
Hide resolved
There was a problem hiding this comment.
@seanbarker Sean, looks great. Just a couple of minor comments.
CC: @zhenxiao
| assertFunction(function, DOUBLE, expectedLength); | ||
| } | ||
| else { | ||
| // assert the function returns the expected value, within the given error tolerance |
There was a problem hiding this comment.
this comment is redundant, it restates the code in the next line; please, remove
| @Test | ||
| public void testLength() | ||
| { | ||
| double length = 4350866.6362; |
There was a problem hiding this comment.
nit: move this variable closer to its first usage
|
@seanbarker Sean, thank you for the contribution. |
Adds
ST_Length(SphericalGeography)which computes the length of aLINESTRINGon the surface of a spherical model of Earth's surface, i.e. the sum of great-circle distances between adjacent points along the linestring.