Fix ST_Centroid and ST_Buffer for tiny geometries #13323

jagill · 2019-09-02T11:50:46Z

Version 2.2.3 has two fixes of interest:

correct computation of centroids of very small polygons - centroid fixes Esri/geometry-api-java#227
correct computation of buffers around small polygons - Don't return null buffer polygon Esri/geometry-api-java#243

== RELEASE NOTES ==

General Changes
-----------------
* Fix ST_Buffer for tiny geometries (#13194)
* Fix ST_Centroid for tiny geometries (#10629)

jagill · 2019-09-02T11:51:06Z

cc @mbasmanova

mbasmanova

@jagill Thanks for fixing centroids and buffers for tiny polygons. I'd split this change into 3 commits:

upgrade ESRI
add tests for buffering tiny polygons
replace centroid logic with ESRI's

For better readability I'd update the commit message as follows (but this will change once the change is split into 3 commits).

Version 2.2.3 has two fixes of interest:

correct computation of centroids of very small polygons - Esri/geometry-api-java#227
correct computation of buffers around small polygons - Esri/geometry-api-java#243
Fixes #13194 and #10629

mbasmanova · 2019-09-03T14:46:57Z

presto-geospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestGeoFunctions.java

@@ -195,6 +199,20 @@ public void testSTBuffer()
        assertInvalidFunction("ST_Buffer(ST_Point(0, 0), nan())", "distance is NaN");
    }

+    @Test
+    public void testSTBufferTinyPolygons()


I'd fold this test into testSTBuffer

mbasmanova · 2019-09-03T14:47:17Z

presto-geospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestGeoFunctions.java


+    @Test
+    public void testSTCentroidInvalidGeometry()


I'd fold this test into testSTCentroid

mbasmanova · 2019-09-03T14:47:22Z

presto-geospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestGeoFunctions.java

+    }
+
+    @Test
+    public void testSTCentroidNumericStability()


I'd fold this test into testSTCentroid

mbasmanova · 2019-09-03T14:47:47Z

presto-geospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestGeoFunctions.java

+    @Test
+    public void testSTCentroidNumericStability()
+    {
+        String wkt1 = "MULTIPOLYGON (((153.492818 -28.13729, 153.492821 -28.137291, 153.492816 -28.137289, 153.492818 -28.13729)))";


don't use a1, a2, a3... names for variables; here, I'd inline wkt1 and wkt2.

mbasmanova · 2019-09-03T14:48:19Z

presto-geospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestGeoFunctions.java

    }

    private void assertCentroid(String wkt, Point centroid)
    {
        assertFunction(format("ST_AsText(ST_Centroid(ST_GeometryFromText('%s')))", wkt), VARCHAR, new OGCPoint(centroid, null).asText());
    }

+    private void assertApproximateCentroid(String wkt, Point centroid, double epsilon)
+    {
+        OGCGeometry geometry = OGCGeometry.fromText(wkt);


I'd inline geometry variable

mbasmanova · 2019-09-03T14:48:33Z

presto-geospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestGeoFunctions.java

+    private void assertApproximateCentroid(String wkt, Point centroid, double epsilon)
+    {
+        OGCGeometry geometry = OGCGeometry.fromText(wkt);
+        Slice inputSlice = GeometrySerde.serialize(geometry);


I'd inline inputSlice

mbasmanova · 2019-09-03T14:49:19Z

presto-geospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestGeoFunctions.java

+        OGCGeometry geometry = OGCGeometry.fromText(wkt);
+        Slice inputSlice = GeometrySerde.serialize(geometry);
+        OGCPoint calculatedCentroid = (OGCPoint) GeometrySerde.deserialize(stCentroid(inputSlice));
+        assertEquals(calculatedCentroid.X(), centroid.getX(), epsilon);


I'd rename centroid to expectedCentroid and calculatedCentroid to centroid (or actualCentroid).

Version 2.2.3 has two fixes of interest: correct computation of centroids of very small polygons - Esri/geometry-api-java#227 correct computation of buffers around small polygons - Esri/geometry-api-java#243

jagill · 2019-09-03T19:31:49Z

Addressed comments. cc @mbasmanova

mbasmanova

Test for ST_Buffer NPE fix

mbasmanova · 2019-09-03T20:29:40Z

presto-geospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestGeoFunctions.java

+
+        // For small polygons, there was a bug in ESRI that throw an NPE.  This
+        // was fixed (https://github.com/Esri/geometry-api-java/pull/243) to
+        // return an empty geometry instead. Ideally, these would return


Would you add a note in ST_Buffer's documentation about this behavior?

mbasmanova · 2019-09-03T20:31:01Z

presto-geospatial/src/main/java/com/facebook/presto/plugin/geospatial/GeoFunctions.java

-                default:
-                    throw new PrestoException(INVALID_FUNCTION_ARGUMENT, "Unexpected geometry type: " + geometryType);
-            }
+            return serialize(geometry.centroid());
        }
        catch (RuntimeException e) {
            if (e instanceof PrestoException && ((PrestoException) e).getErrorCode() == INVALID_FUNCTION_ARGUMENT.toErrorCode()) {
                throw e;
            }
            throw new PrestoException(INVALID_FUNCTION_ARGUMENT, format("Cannot compute centroid: %s Use ST_IsValid to confirm that input geometry is valid or compute centroid for a bounding box using ST_Envelope.", e.getMessage()), e);


@jagill Is this logic still needed? I see that you removed the test that triggered it.

I suppose not!

Upgrading to Esri 2.2.3 included the fix in Esri/geometry-api-java#243 Fixes prestodb#13194

In Esri v2.2.3, the centroid calculate includes centroid fixes for degenerate (near dimension 1) polygons (Esri/geometry-api-java#227). This replaces our implemenation (which the original Esri implemenation was based on) with the fixed Esri version. Fixes prestodb#10629

jagill · 2019-09-03T23:09:08Z

Comments address. cc @mbasmanova

mbasmanova

@jagill Looks good to me. Thank you for fixing these longstanding issues.

facebook-github-bot added the CLA Signed label Sep 2, 2019

jagill force-pushed the centroid-numerical-stability branch from 10efddd to 36f027d Compare September 2, 2019 14:29

mbasmanova requested changes Sep 3, 2019

View reviewed changes

mbasmanova requested a review from zhenxiao September 3, 2019 14:49

mbasmanova self-assigned this Sep 3, 2019

Upgrade ESRI geometry to v2.2.3

b004edf

Version 2.2.3 has two fixes of interest: correct computation of centroids of very small polygons - Esri/geometry-api-java#227 correct computation of buffers around small polygons - Esri/geometry-api-java#243

jagill force-pushed the centroid-numerical-stability branch 2 times, most recently from bac0627 to 8e2777d Compare September 3, 2019 18:23

jagill force-pushed the centroid-numerical-stability branch from 8e2777d to 0cd721d Compare September 3, 2019 19:52

mbasmanova reviewed Sep 3, 2019

View reviewed changes

jagill added 2 commits September 3, 2019 19:07

Test for ST_Buffer NPE fix

1e1579f

Upgrading to Esri 2.2.3 included the fix in Esri/geometry-api-java#243 Fixes prestodb#13194

jagill force-pushed the centroid-numerical-stability branch from 0cd721d to a0fead3 Compare September 3, 2019 23:08

mbasmanova approved these changes Sep 3, 2019

View reviewed changes

mbasmanova changed the title ~~Upgrade to ESRI geometry 2.2.3~~ Fix ST_Centroid and ST_Buffer for tiny geometries Sep 4, 2019

mbasmanova merged commit 3cdf680 into prestodb:master Sep 4, 2019

jagill deleted the centroid-numerical-stability branch September 4, 2019 14:47

neeradsomanchi mentioned this pull request Sep 9, 2019

Release notes for 0.226 #13340

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ST_Centroid and ST_Buffer for tiny geometries #13323

Fix ST_Centroid and ST_Buffer for tiny geometries #13323

jagill commented Sep 2, 2019 •

edited by mbasmanova

Loading

jagill commented Sep 2, 2019

mbasmanova left a comment

mbasmanova Sep 3, 2019

mbasmanova Sep 3, 2019

mbasmanova Sep 3, 2019

mbasmanova Sep 3, 2019

mbasmanova Sep 3, 2019

mbasmanova Sep 3, 2019

mbasmanova Sep 3, 2019

jagill commented Sep 3, 2019

mbasmanova left a comment

mbasmanova Sep 3, 2019

mbasmanova Sep 3, 2019

jagill Sep 3, 2019

jagill commented Sep 3, 2019

mbasmanova left a comment

Fix ST_Centroid and ST_Buffer for tiny geometries #13323

Fix ST_Centroid and ST_Buffer for tiny geometries #13323

Conversation

jagill commented Sep 2, 2019 • edited by mbasmanova Loading

jagill commented Sep 2, 2019

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagill commented Sep 3, 2019

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagill commented Sep 3, 2019

mbasmanova left a comment

Choose a reason for hiding this comment

jagill commented Sep 2, 2019 •

edited by mbasmanova

Loading