Add ST_Union geospatial function #10921

yaoxin226 · 2018-06-26T19:15:04Z

ST_Union function takes two geometries and returns a new geometry that represents the point set union of the input geometries.

yaoxin226 · 2018-06-26T19:16:33Z

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

+    {
+        OGCGeometry leftGeometry = deserialize(left);
+        OGCGeometry rightGeometry = deserialize(right);
+        OGCGeometry ogcGeometry = leftGeometry.union(rightGeometry);


It returns null when unions GEOMETRYCOLLECTION EMPTY with GEOMETRYCOLLECTION EMPTY

@yaoxin226 Xin, see Esri/geometry-api-java#176 and Esri/geometry-api-java#177 for limitations of the library's union implementation. See also the implementation of ST_ConvexHull in GeoFunctions.java.

Until the next release of the library, let's restrict the input of ST_Union to geometries other than geometry collection. Let's also add explicit handing of empty geometries.

if (left.isEmpty) return right; if (right.isEmpty) return left;

Thanks for your prompt reply.
So this function only supports POINT and LINE_STRING and POLYGON as the input of the function? Support of Multi-geometries also waits?

@yaoxin226 It supports multi-geometries, but not geometry-collection.

mbasmanova

@yaoxin226 Xin, this is looking very nice. Thank you very much for adding lots of tests.

mbasmanova · 2018-06-28T00:53:20Z

presto-docs/src/main/sphinx/functions/geospatial.rst

@@ -126,6 +126,10 @@ Operations

    Returns the geometry value that represents the point set symmetric difference of two geometries.

+.. function:: ST_Union(Geometry, Geometry) -> Geometry
+
+    Returns a geometry that represents the point set union of the Geometries.


s/of the Geometries/of the input geometries

Could you mention that GEOMETRYCOLLECTION is not supported?

This function doesn't support geometry collections.

mbasmanova · 2018-06-28T00:57:30Z

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

+        assertFunction(format("ST_ASText(ST_Union(ST_GeometryFromText('%s'), ST_GeometryFromText('%s')))", rightWkt, leftWkt), VARCHAR, expectWkt);
+    }
+
+    private void assertSTUnionInvalidFunction(String leftWkt, String rightWkt, String errorMessage)


Since all callers pass the same errorMessage, let's remove errorMessage argument and hard-coded the error message. Let's also rename this method to assertInvalidUnion.

Hardcoded both GeometryCollection and errorMessage, since all callers pass the same parameters for them. Also renamed the function name to reflect that.

mbasmanova · 2018-06-28T00:57:57Z

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

+        assertSTUnion("MULTIPOLYGON (((1 1, 3 1, 3 3, 1 3, 1 1)))", "MULTIPOLYGON (((2 2, 4 2, 4 4, 2 4, 2 2)))", "POLYGON ((1 1, 3 1, 3 2, 4 2, 4 4, 2 4, 2 3, 1 3, 1 1))");
+    }
+
+    private void assertSTUnion(String leftWkt, String rightWkt, String expectWkt)


For clarity, let's remove ST: assertUnion.

mbasmanova · 2018-06-28T00:58:37Z

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

+    private void assertSTUnion(String leftWkt, String rightWkt, String expectWkt)
+    {
+        assertFunction(format("ST_ASText(ST_Union(ST_GeometryFromText('%s'), ST_GeometryFromText('%s')))", leftWkt, rightWkt), VARCHAR, expectWkt);
+        assertFunction(format("ST_ASText(ST_Union(ST_GeometryFromText('%s'), ST_GeometryFromText('%s')))", rightWkt, leftWkt), VARCHAR, expectWkt);


Thanks for testing union(a, b) as well as union(b, a).

mbasmanova · 2018-06-28T00:59:40Z

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

+    {
+        // invalid type GEOMETRYCOLLECTION
+        String invalidGeometryCollectionErrorMessage = "ST_Union only applies to POINT or MULTI_POINT or LINE_STRING or MULTI_LINE_STRING or POLYGON or MULTI_POLYGON. Input type is: GEOMETRY_COLLECTION";
+        assertSTUnionInvalidFunction("GEOMETRYCOLLECTION(POINT(2 3))", "POINT (1 2)", invalidGeometryCollectionErrorMessage);


perhaps, write this as a loop over simpleWkts

mbasmanova · 2018-06-28T01:01:25Z

presto-main/src/test/java/com/facebook/presto/operator/scalar/FunctionAssertions.java

@@ -331,7 +331,7 @@ public void assertInvalidFunction(String projection, StandardErrorCode errorCode
        catch (PrestoException e) {
            try {
                assertEquals(e.getErrorCode(), errorCode.toErrorCode());
-                assertTrue(e.getMessage().equals(messagePattern) || e.getMessage().matches(messagePattern));
+                assertTrue(e.getMessage().equals(messagePattern) || e.getMessage().matches(messagePattern), format("Exception message[%s] should either equals or matches messagePattern[%s]", e.getMessage(), messagePattern));


To match L329, let's update to Error message [%s] doesn't match [%s]

mbasmanova · 2018-06-28T01:02:09Z

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

+        OGCGeometry rightGeometry = deserialize(right);
+        validateType("ST_Union", rightGeometry, EnumSet.of(POINT, MULTI_POINT, LINE_STRING, MULTI_LINE_STRING, POLYGON, MULTI_POLYGON));
+
+        if (leftGeometry.isEmpty()) {


perhaps, move this check above deserialize(right) to avoid unnecessary deserialization

mbasmanova · 2018-06-28T01:02:29Z

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

+            return left;
+        }
+
+        OGCGeometry ogcGeometry = leftGeometry.union(rightGeometry);


perhaps, inline this variable

mbasmanova · 2018-06-28T01:03:33Z

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

@@ -496,6 +496,31 @@ public static long stNumGeometries(@SqlType(GEOMETRY_TYPE_NAME) Slice input)
        return ((OGCGeometryCollection) geometry).numGeometries();
    }

+    @SqlNullable


this function doesn't return nulls, does it?

yaoxin226

Thanks @mbasmanova for the thorough review. I have addressed all the comments.

yaoxin226 · 2018-06-28T18:07:04Z

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

+        assertFunction(format("ST_ASText(ST_Union(ST_GeometryFromText('%s'), ST_GeometryFromText('%s')))", rightWkt, leftWkt), VARCHAR, expectWkt);
+    }
+
+    private void assertSTUnionInvalidFunction(String leftWkt, String rightWkt, String errorMessage)


Hardcoded both GeometryCollection and errorMessage, since all callers pass the same parameters for them. Also renamed the function name to reflect that.

mbasmanova

@yaoxin226 Looks great!

Please, squash commits and update commit message to remove reference to PostGIS. Geospatial functions in Presto follow SQL/MM Part 3 standard.

Add ST_Union geospatial function

ST_Union function takes two geometries and returns a new geometry that represents the point set union of the input geometries.

mbasmanova · 2018-06-28T18:35:47Z

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

+
+    private void assertInvalidGeometryCollectionUnion(String validWkt)
+    {
+        assertInvalidFunction(format("ST_ASText(ST_Union(ST_GeometryFromText('%s'), ST_GeometryFromText('%s')))", validWkt, "GEOMETRYCOLLECTION(POINT(2 3))"), "ST_Union only applies to POINT or MULTI_POINT or LINE_STRING or MULTI_LINE_STRING or POLYGON or MULTI_POLYGON. Input type is: GEOMETRY_COLLECTION");


ST_Text and placeholder for geometry collection are not necessary; let's simplify to

assertInvalidFunction(format("ST_Union(ST_GeometryFromText('%s'), ST_GeometryFromText('GEOMETRYCOLLECTION (POINT(2 3))'))", validWkt), "ST_Union only applies to POINT or MULTI_POINT or LINE_STRING or MULTI_LINE_STRING or POLYGON or MULTI_POLYGON. Input type is: GEOMETRY_COLLECTION");

yaoxin226 · 2018-06-28T22:08:21Z

Addressed all comments and squashed into one commit

ST_Union function takes two geometries and returns a new geometry that represents the point set union of the input geometries.

mbasmanova · 2018-06-28T23:03:33Z

@yaoxin226 Thank you, Xin.

yaoxin226 · 2018-06-28T23:15:16Z

Thanks @mbasmanova for your prompt and thorough review!

facebook-github-bot added the CLA Signed label Jun 26, 2018

yaoxin226 requested a review from mbasmanova June 26, 2018 19:15

yaoxin226 self-assigned this Jun 26, 2018

yaoxin226 commented Jun 26, 2018

View reviewed changes

mbasmanova requested changes Jun 28, 2018

View reviewed changes

yaoxin226 commented Jun 28, 2018

View reviewed changes

mbasmanova approved these changes Jun 28, 2018

View reviewed changes

yaoxin226 force-pushed the ST_Union branch from 915fd7c to 8c70cc0 Compare June 28, 2018 22:06

Add ST_Union geospatial function

88514f6

ST_Union function takes two geometries and returns a new geometry that represents the point set union of the input geometries.

yaoxin226 force-pushed the ST_Union branch from 8c70cc0 to 88514f6 Compare June 28, 2018 22:11

mbasmanova changed the title ~~Add geometry function ST_Union~~ Add ST_Union geospatial function Jun 28, 2018

mbasmanova merged commit f5f2a3b into prestodb:master Jun 28, 2018

yaoxin226 deleted the ST_Union branch June 28, 2018 23:15

tdcmeehan mentioned this pull request Jul 30, 2018

Upgrade to esri-geometry-api version 2.2.0 #11157

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ST_Union geospatial function #10921

Add ST_Union geospatial function #10921

yaoxin226 commented Jun 26, 2018 •

edited by mbasmanova

Loading

yaoxin226 Jun 26, 2018

mbasmanova Jun 26, 2018

yaoxin226 Jun 27, 2018

mbasmanova Jun 27, 2018

mbasmanova left a comment

mbasmanova Jun 28, 2018

mbasmanova Jun 28, 2018

mbasmanova Jun 28, 2018

yaoxin226 Jun 28, 2018

mbasmanova Jun 28, 2018

mbasmanova Jun 28, 2018

mbasmanova Jun 28, 2018

mbasmanova Jun 28, 2018

mbasmanova Jun 28, 2018

mbasmanova Jun 28, 2018

mbasmanova Jun 28, 2018

yaoxin226 left a comment

yaoxin226 Jun 28, 2018

mbasmanova left a comment

mbasmanova Jun 28, 2018

yaoxin226 commented Jun 28, 2018

mbasmanova commented Jun 28, 2018

yaoxin226 commented Jun 28, 2018

Add ST_Union geospatial function #10921

Add ST_Union geospatial function #10921

Conversation

yaoxin226 commented Jun 26, 2018 • edited by mbasmanova Loading

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

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

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

yaoxin226 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaoxin226 commented Jun 28, 2018

mbasmanova commented Jun 28, 2018

yaoxin226 commented Jun 28, 2018

yaoxin226 commented Jun 26, 2018 •

edited by mbasmanova

Loading