Support GeoSpatial functions in Presto #8878

zhenxiao · 2017-08-30T02:03:58Z

Extract Geometry Type and functions in this commit from:
#5435

mbasmanova

@zhenxiao , thank you for extracting spatial functions into a separate PR and thanks for addressing comments. The code looks good. All functions seem to have tests for happy paths. It would be helpful to add some negative test cases to the test suite as well. I have a question about handling nulls returned by deserialize function below.

mbasmanova · 2017-08-30T15:20:06Z

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

+
+public final class GeoFunctions
+{
+    private static final String LINE = "LineString";


GeometryUtils.LINE_STRING to avoid copy-pasting hard-coded strings

Ditto other constants.

mbasmanova · 2017-08-30T15:20:24Z

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

+    private static final String MULTIPOINT = "MultiPoint";
+    private static final String POLYGON = "Polygon";
+    private static final String POINT = "Point";
+    private static final String GEOMETRY = "Geometry";


GeometryType.NAME to avoid copy-pasting hard-coded strings

mbasmanova · 2017-08-30T15:22:12Z

presto-geospatial/src/main/java/com/facebook/presto/geospatial/GeometryUtils.java

+        return getOGCType(s);
+    }
+
+    public static int getWkid(Slice slice)


This method doesn't seem to be used. Did you mean to make it private and call it from deserialize line 71?

mbasmanova · 2017-08-30T15:22:48Z

presto-geospatial/src/main/java/com/facebook/presto/geospatial/GeometryUtils.java

+        return result;
+    }
+
+    public static OGCType getGeomType(Slice slice)


This method doesn't seem to be used.

mbasmanova · 2017-08-30T15:24:54Z

presto-geospatial/src/main/java/com/facebook/presto/geospatial/GeometryUtils.java

+        }
+    }
+
+    public static final OGCType getOGCType(int value)


This method is used only by getGeomType which itself it not used.

mbasmanova · 2017-08-30T15:35:52Z

presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java

+    public void testSTLineString()
+            throws Exception
+    {
+        assertQuery("select st_as_text(st_line_string('linestring(1 1, 2 2, 1 3)'))", "select 'LINESTRING (1 1, 2 2, 1 3)'");


nit: perhaps, test 'linestring empty' as well

mbasmanova · 2017-08-30T15:37:13Z

presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java

+    public void testSTCentroid()
+            throws Exception
+    {
+        assertQuery("select st_as_text(st_centroid(st_geometry_from_text('polygon ((1 1, 1 4, 4 4, 4 1))')))", "select 'POINT (2.5 2.5)'");


nit: consider testing st_centroid on a non-polygon shape

ditto other methods which apply to a restricted set of shape types

mbasmanova · 2017-08-30T15:37:40Z

presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java

+    public void testSTIsClosed()
+            throws Exception
+    {
+        assertQuery("select st_is_closed(st_geometry_from_text('linestring(1 1, 2 2, 1 3, 1 1)'))", "select true");


nit: consider testing linestring which is not closed

mbasmanova · 2017-08-30T15:37:53Z

presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java

+    public void testSTIsEmpty()
+            throws Exception
+    {
+        assertQuery("select st_is_empty(st_geometry_from_text('POINT (1.5 2.5)'))", "select false");


nit: consider testing empty shape

mbasmanova · 2017-08-30T15:39:15Z

presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java

+            throws Exception
+    {
+        assertQuery("select st_max_x(st_geometry_from_text('linestring(8 4, 4 8)'))", "select 8.0");
+        assertQuery("select st_max_y(st_geometry_from_text('linestring(8 4, 4 8)'))", "select 8.0");


nit: consider testing using a linestring with different max_x and max_y; otherwise a typo in the implementation which makes max_x and max_y always return same value will go unnoticed

martint

Some initial comments

martint · 2017-08-30T22:09:08Z

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

+    @SqlType(GEOMETRY)
+    public static Slice stLineString(@SqlType(StandardTypes.VARCHAR) Slice input)
+    {
+        OGCGeometry geometry = getGeometry(OGCGeometry.fromText(input.toStringUtf8()), "st_line_string", LINE);


Are there well-known constants for things like "st_line_string"?

martint · 2017-08-30T22:24:31Z

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

+    @Description("Construct Point Geometry")
+    @ScalarFunction
+    @SqlType(GEOMETRY)
+    public static Slice stPoint(@SqlType(StandardTypes.DOUBLE) double longitude, @SqlType(StandardTypes.DOUBLE) double latitude)


Why "latitude" and "longitude" vs just "x" and "y"? This is mixing concepts of euclidean geometry with geospatial systems, which include reference systems and geodetic datums.

This distinction is important when performing computations like calculating areas and lengths. E.g. the area of a "polygon" whose vertices are defined by latitude/longitude pairs (which is not really a polygon, but a slice of curved surface) is not the same as the area of a 2-d polygon.

yes, will fix it

martint · 2017-08-30T22:34:31Z

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

+    }
+
+    @Description("Returns Geometry of input WKT text")
+    @ScalarFunction("st_geometry_from_text")


Maybe call this from_wkt / to_wkt (similar to from_iso8601/to_iso8601)

will fix it

@martint, ST_GeometryFromText and ST_AsText are standard names for conversion functions to/from WKT. The way I think about it is "text" in the context of "spatial" (provided by ST_ prefix) implies Well-Known-Text. Similarly, ST_GeometryFromBinary and ST_AsBinary names are used for conversions to/from Well-Known-Binary format.

PostGIS: http://www.postgis.org/docs/ST_AsText.html
ESRI-for-Hadoop: https://github.com/Esri/spatial-framework-for-hadoop/wiki/UDF-Constructors

Speaking of function names, I noticed that standard names don't match Presto's convention: ST_GeometryFromText vs. st_geometry_from_text. Do you think it is better to keep standard names or change them to match Presto's style?

In that case we should keep the name. We should stay as close to the standard as possible.

BTW, are all the functions in this PR defined in the standard? Have we looked at what's their expected behavior and whether our implementation matches? Can you comment on the question I had above whether the library is meant to operate on euclidean geometry or with respect to a reference system and datum (WGS84, etc)?

@zhenxiao , @martint , re: euclidean geometry - it is my understanding is that all computations in this PR are performed on a 2D plane. @zhenxiao , would you confirm?

The new Geometry type uses ESRI shape file format for serialization. That format allows to specify spatial reference, but also accepts null. Constructor functions ST_Point, ST_Polygon, ST_GeometryFromText accept WKT which doesn't include SRID, hence, all the instances of Geometry type have null spatial reference. Computations accept Geometry arguments, but strip spatial references via calls to OGCGeometry.getEsriGeometry() before invoking underlying library functions to perform the computation. For example,

public static double stArea(@SqlType(NAME) Slice input)
{
// Convert ESRI shape file to OGCGeometry
OGCGeometry geometry = getGeometry(deserialize(input), "ST_Area", POLYGON);
// Strip spatial reference and call calculateArea2D
return geometry.getEsriGeometry().calculateArea2D();
}

@martint I confirm all the implementations are 2D and euclidean geometry.
Yes, as @mbasmanova said, this PR is using DSRI shape for serialization/deserialization, and calling underlying library function calls to perform the computation

martint · 2017-08-30T23:07:37Z

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

+    }
+
+    @SqlNullable
+    @Description("Returns true if and only if the line is closed")


What does it mean the line is closed? Does it mean that if it's made of multiple segments all segments are connected? Or that it's a completely closed loop (i.e., the last segment connects to the first)? If the latter, this implementation seems incorrect.

martint · 2017-08-30T23:08:50Z

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

+        boolean result = true;
+        for (int i = 0; result && i < pathCount; i++) {
+            Point start = lines.getPoint(lines.getPathStart(i));
+            Point end = lines.getPoint(lines.getPathEnd(i) - 1);


I don't know much about the getPoint/getPathEnd APIs, but shouldn't this be getPathEnd(i - 1) ?

getPathEnd: Returns the index immediately following the last index of the path. So use MultiPath.getPathEnd(i) -1 to get the last index of the path

martint · 2017-08-30T23:12:51Z

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

+            return polygon.getPointCount() + polygon.getPathCount();
+        }
+        else if (geometry.geometryType().equals(POINT)) {
+            return geometry.getEsriGeometry().isEmpty() ? 0 : 1;


This seems strange. If I have an st_point, shouldn't count always be 1? What does it mean for the point to be "empty"?

yes, will fix it

martint · 2017-08-30T23:13:34Z

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

+    }
+
+    @SqlNullable
+    @Description("Returns true if and only if the line is closed and simple")


What does closed and simple mean?

simple means:
Each figure of the instance must not intersect itself, except at its endpoints.
No two figures of the instance can intersect each other at a point that is not in both of their boundaries.

martint · 2017-08-30T23:13:55Z

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

+        return line.isClosed() && line.isSimple();
+    }
+
+    @Description("Returns the start point of an line")


will update

martint · 2017-08-30T23:16:58Z

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

+        OGCGeometry geometry = getGeometry(deserialize(input), "st_start_point", LINE);
+        MultiPath lines = (MultiPath) geometry.getEsriGeometry();
+        SpatialReference reference = geometry.getEsriSpatialReference();
+        return serialize(OGCGeometry.createFromEsriGeometry(lines.getPoint(0), reference));


Why do we need the SpatialReference? shouldn't this just return lines.getPoint(0)? (See my comment above regarding euclidean geometry vs geospatial)

need to serialize an OGCGeometry, Point is not OGCGeometry

martint · 2017-08-30T23:18:06Z

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

+        OGCGeometry geometry = getGeometry(deserialize(input), "st_end_point", LINE);
+        MultiPath lines = (MultiPath) geometry.getEsriGeometry();
+        SpatialReference reference = geometry.getEsriSpatialReference();
+        return serialize(OGCGeometry.createFromEsriGeometry(lines.getPoint(lines.getPointCount() - 1), reference));


Same comment here

zhenxiao · 2017-08-31T01:03:24Z

thank you @mbasmanova @martint
Get your comments addressed with updated PR

electrum · 2017-08-31T16:08:27Z

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

+
+    Returns the Y coordinate of point.
+
+.. function:: st_point_number(geometry) -> bigint


I think this was renamed to st_point_count

@electrum , FWIW, PostGIS has this function under ST_NumPoints name: http://postgis.org/docs/ST_NumPoints.html

OpenGIS standard also mentions ST_NumPoints and ST_NumInteriorRings

electrum · 2017-08-31T16:09:43Z

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

+    @Description("Returns the number of interior rings in the polygon")
+    @ScalarFunction
+    @SqlType(StandardTypes.BIGINT)
+    public static long stInteriorRingNumber(@SqlType(NAME) Slice input)


Should this also be count instead of number?

FWIW, PostGIS version is named ST_NumInteriorRings: http://postgis.org/docs/ST_NumInteriorRings.html

electrum · 2017-08-31T16:19:26Z

I didn't realize until now that SQL/MM is ISO standard 13249-3. I could only find a draft from 2004, and it seems the latest edition was published in 2016, so we should buy a copy for review.

Which of these functions are covered by the standard? Does the behavior and naming match?

mbasmanova · 2017-08-31T18:40:09Z

@electrum , I found that PostGIS documentation conveniently marks functions which conform to SQL/MM, or OpenGIS, or both.

Here is an example of ST_IsSimple from PostGIS: https://postgis.net/docs/ST_IsSimple.html
And here is a list of all ST_* functions supported by PostGIS: http://postgis.org/docs/reference.html I've been referring to this documentation for checking whether functions in this PR are conforming to the standard or not. (One function which seems non-standard is st_envelope_intersect. It can be replaced with a composition of standard functions: st_intersect(st_envelope(g1), st_envelope(g2)).)

Also, I found an overview of SQL/MM which I suspect is easier to digest than the actual standard document, but it is not as detailed: http://doesen0.informatik.uni-leipzig.de/proceedings/paper/68.pdf

zhenxiao · 2017-08-31T18:48:34Z

Thank you @electrum @mbasmanova @martint
I will review postGIS and updated any function names that do not match

zhenxiao · 2017-08-31T23:14:52Z

@martint @mbasmanova @electrum updated with a review of all functions matches name and logic in:
http://doesen0.informatik.uni-leipzig.de/proceedings/paper/68.pdf

mbasmanova

@zhenxiao , @martint , I've been walking through the functions defined in GeoFunctions.java and checking the naming and implementation with the standard document. I've checked functions up to ST_IsClosed and have some comments. I'll check the rest of the functions and post further comments in a separate review.

mbasmanova · 2017-09-05T19:39:42Z

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

+{
+    private GeoFunctions() {}
+
+    @Description("Return a geometry type Line object from Well-Known Text representation (WKT)")


In the standard ST_LineFromText function is defined as a constructor for ST_LineString which is a more generic type then line. Would you replace "Line" with "LineString" in this documentation?

In addition, ST_LineFromText doesn't allow constructing multi-line strings and therefore should not allow MULTILINESTRING WKT input. Current implementation does. Would you restrict the implementation to just LINESTRING ? (The standard defines ST_MLineFromText as a constructor for multi-line-strings.)

mbasmanova · 2017-09-05T19:43:40Z

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

+    @SqlType(NAME)
+    public static Slice stPoint(@SqlType(StandardTypes.DOUBLE) double x, @SqlType(StandardTypes.DOUBLE) double y)
+    {
+        OGCGeometry geometry = getGeometry(OGCGeometry.createFromEsriGeometry(new Point(x, y), null), "ST_Point", POINT);


getGeometry call here is redundant, because OGCGeometry.createFromEsriGeometry(new Point(x, y), null) can't return anything other than point. Perhaps, remove it.

mbasmanova · 2017-09-05T19:45:12Z

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

+        return serialize(geometry);
+    }
+
+    @Description("Returns a polygon built from Well-Known Text representation (WKT)")


nit: inconsistent documentation: "a polygon built" vs. "a geometry type Line object"

mbasmanova · 2017-09-05T19:50:35Z

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

+        return serialize(geometry);
+    }
+
+    @Description("Returns the area of the surface if it is a polygon")


The reference to "surface" here might be confusing because it may suggest the measurement around the sphere or ellipsoid as opposed to simple Euclidean measurement on a 2D plane. @zhenxiao , it might be useful to clarify how exactly the measurement is taken.

mbasmanova · 2017-09-05T19:52:20Z

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

+        return geometry.getEsriGeometry().calculateArea2D();
+    }
+
+    @Description("Return a Geometry from Well-Known Text representation (WKT)")


nit: for consistency sake, consider replacing "Return a Geometry" with "Returns a geometry type object"

mbasmanova · 2017-09-05T19:53:49Z

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

+    }
+
+    @SqlNullable
+    @Description("Return the Well-Known Text (WKT) representation of the geometry")


nit: Return -> Returns

mbasmanova · 2017-09-05T19:55:59Z

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

+        return Slices.utf8Slice(deserialize(input).asText());
+    }
+
+    @Description("Returns the geometric center of a geometry")


nit: perhaps, borrow the documentation from the standard and clarify that this method applies to polygons only?

"Returns the Point value that is the mathematical centroid of the Polygon"

However, the implementation doesn't seem to match the definition of centroid [1]: "the arithmetic mean ("average") position of all the points in the shape". Instead, the implementation calculates the center of the bounding box. Is this intended? The standard calls for "mathematical centroid" and PostGIS implements it that way [2]. I also noticed that ESRI-for-Hadoop implemented ST_Centroid as in this PR [3]: "ST_Centroid(polygon) returns the point that is the center of the polygon's envelope"

[1] https://en.wikipedia.org/wiki/Centroid
[2] https://postgis.net/docs/ST_Centroid.html
[3] https://github.com/Esri/spatial-framework-for-hadoop/wiki/UDF-Accessors#st_centroid

mbasmanova · 2017-09-05T20:45:56Z

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

+
+    @Description("Return the coordinate dimension of the Geometry")
+    @ScalarFunction("ST_CoordDim")
+    @SqlType(StandardTypes.BIGINT)


nit: since dimension values are limited to 2, 3 or 4 would it make sense to use TINYINT instead of BIGINT?

mbasmanova · 2017-09-05T20:49:03Z

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

+
+    @Description("Returns the inherent dimension of this Geometry object, which must be less than or equal to the coordinate dimension")
+    @ScalarFunction("ST_Dimension")
+    @SqlType(StandardTypes.BIGINT)


nit: since dimension values are limited to 2, 3 or 4 would it make sense to use TINYINT instead of BIGINT?

mbasmanova

Here are some further comments on functions up to but not including ST_NPoints.

mbasmanova · 2017-09-05T21:02:05Z

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

+        for (int i = 0; result && i < pathCount; i++) {
+            Point start = lines.getPoint(lines.getPathStart(i));
+            Point end = lines.getPoint(lines.getPathEnd(i) - 1);
+            result = result && end.equals(start);


nit: looks like this method can return false early if it detects a non-closed line

mbasmanova · 2017-09-05T21:03:22Z

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

+    }
+
+    @SqlNullable
+    @Description("Returns TRUE if the LINESTRING's start and end points are coincident")


nit: for consistency sake, consider replacing LINESTRING with LineString and also mention Multi-LineString as the implementation allows both types.

mbasmanova · 2017-09-05T21:06:16Z

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

+    }
+
+    @SqlNullable
+    @Description("Returns true if this Geometry is an empty geometrycollection, polygon, point etc")


nit: perhaps, borrow the documentation from the standard

"Returns TRUE if a geometry value corresponds to the empty set"

It would be helpful to decide how to refer to TRUE value in the docs. Some comments use all-upper-case TRUE while others use lower-case true.

mbasmanova · 2017-09-05T21:12:48Z

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

+        return deserialize(input).isEmpty();
+    }
+
+    @Description("Returns the 2d length of the geometry if it is a linestring or multilinestring")


perhaps, like for ST_Area, clarify what kind of computation is provided? Planar or geodesic?

mbasmanova · 2017-09-05T21:16:14Z

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

+    }
+
+    @Description("Returns X maxima of a bounding box 2d or 3d or a geometry")
+    @ScalarFunction("ST_XMax")


I don't see this function in the SQL/MM standard, but I do see it available in PostGIS. What's the use case for this function? The documentation refers to 2d and 3d box objects, but I don't think this PR supports them. Ditto ST_YMax, ST_YMin, ST_XMin.

mbasmanova · 2017-09-05T21:17:27Z

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

+    }
+
+    @Description("Return the number of interior rings of the first polygon in the geometry")
+    @ScalarFunction("ST_NumInteriorRings")


the standard name doesn't have 's' at the end: ST_NumInteriorRing

mbasmanova · 2017-09-05T21:18:17Z

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

+        return envelope.getYMin();
+    }
+
+    @Description("Return the number of interior rings of the first polygon in the geometry")


nit: perhaps, borrow the documentation from the standard and clarify that this function only applies to polygons

"Returns the cardinality of the collection of interior rings of a polygon"

zhenxiao · 2017-09-05T23:53:03Z

thank you @mbasmanova
I get your comments addressed.

mbasmanova

@zhenxiao , I made a full pass other the functions in GeoFunctions.java . Here are remaining comments.

mbasmanova · 2017-09-06T00:52:00Z

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

+    }
+
+    @Description("Returns the number of points in the geometry")
+    @ScalarFunction("ST_NPoints")


SQL/MM only defines ST_NumPoints which "returns the cardinality of the ST_Point collection in the ST_LineString value". ST_NPoints function comes from PostGIS and is non-standard extension of ST_NumPoints which applies to any geometry type.

Would it make sense to rename this function to ST_NumPoints and document is as a natural extension of the standard one?

mbasmanova · 2017-09-06T01:23:37Z

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

+        else if (geometry.geometryType().equals(POINT)) {
+            return 1;
+        }
+        else if (geometry.geometryType().equals(POLYGON)) {


Polygon, MultiPoint and MultiPath have common base class MultiVertexGeometry which has a getPointCount method. Therefore, all three types can be handled with a single cast to MultiVertexGeometry.

return ((MultiVertexGeometry) geometry.getEsriGeometry()).getPointCount();

mbasmanova · 2017-09-06T02:07:40Z

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

+    {
+        OGCGeometry geometry = deserialize(input);
+        OGCGeometry bound = geometry.boundary();
+        if (bound.geometryType().equals(MULTI_LINE_STRING) && ((OGCMultiLineString) bound).numGeometries() == 1) {


This logic seems unnecessary. I noticed that serialize function outputs 3 pieces of data: (1) SRID, (2) geometry type, (3) ESRI Shape File. However, deserialize function uses only (1) and (3) and implicitly derives (2) from (3). serialize-deserialize cycle applied to a MultiLineString of one LineString produces the same result as converting MultiLineString to a LineString first. Hence, two suggestions:

(1) remove this logic
(2) update serialize to not output shape type

mbasmanova · 2017-09-06T02:16:19Z

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

+        return serialize(bound);
+    }
+
+    @Description("Returns a geometry representing the double precision (float8) bounding box of the supplied geometry")


nit: perhaps, borrow from the standard to clarify that return value is a polygon

"Returns the bounding rectangular polygon of a geometry"

mbasmanova · 2017-09-06T02:18:46Z

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

+        return serialize(OGCGeometry.createFromEsriGeometry(envelope, reference));
+    }
+
+    @Description("Returns a geometry that represents that part of geometry A that does not intersect with geometry B")


nit: references to A and B don't match parameter names left and right

perhaps, copy from the SQL/MM: "Returns the Geometry value that represents the point set difference of two geometries"

mbasmanova · 2017-09-06T02:36:18Z

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

+        OGCGeometry leftGeometry = deserialize(left);
+        OGCGeometry rightGeometry = deserialize(right);
+        OperatorSimpleRelation containsOperator = OperatorContains.local();
+        return containsOperator.execute(leftGeometry.getEsriGeometry(), rightGeometry.getEsriGeometry(), leftGeometry.getEsriSpatialReference(), null);


why not leftGeometry.contains(rightGeometry) ?

mbasmanova · 2017-09-06T02:38:57Z

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

+    {
+        OGCGeometry leftGeometry = deserialize(left);
+        OGCGeometry rightGeometry = deserialize(right);
+        OperatorSimpleRelation crossesOperator = OperatorCrosses.local();


why not leftGeometry.crosses(rightGeometry) ?

mbasmanova · 2017-09-06T02:45:37Z

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

+    }
+
+    @SqlNullable
+    @Description("Returns TRUE if the supplied geometries have some, but not all, interior points in common")


This definition seems correct. However, when I called this function with a line and a point on the line, I got FALSE. I believe this function should return TRUE in such a case. Does it work like this for you?

This is what I tried:

assertQuery("select st_crosses(st_line('linestring(0 0, 1 1)'), st_point(0, 0))", "select true");

https://postgis.net/docs/ST_Crosses.html

Hi @mbasmanova I think:
st_crosses(st_line('linestring(0 0, 1 1)'), st_point(0, 0))

should return FALSE

as Point(0 0) is one point in linestring(0 0, 1 1), it is ALL in the LineString. ST_Crosses requires Not All in.

What do you think?

@zhenxiao , I think you are right. Thanks for clarifying.

mbasmanova · 2017-09-06T02:46:36Z

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

+    {
+        OGCGeometry leftGeometry = deserialize(left);
+        OGCGeometry rightGeometry = deserialize(right);
+        OperatorSimpleRelation disjointOperator = OperatorDisjoint.local();


why not leftGeometry.disjoint(rightGeometry) ? ditto ST_Intersects, ST_Overlaps, ST_Within, etc.

mbasmanova · 2017-09-06T02:47:50Z

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

+    {
+        OGCGeometry leftGeometry = deserialize(left);
+        OGCGeometry rightGeometry = deserialize(right);
+        OperatorSimpleRelation equalsOperator = OperatorEquals.local();


why not call OGCGeometry.equals method?

mbasmanova

@zhenxiao , thanks for addressing my comments. I'm wondering why I can't see the changes you made in isolation. The only guess I have is that you are using force-push to update the PR (git push -f origin geoSpatial). If that's the case, could you omit '-f' in the next pushes? This will generate a commit per change and allow me to see changes between revisions. The PR will automatically combine all commits together and allow the final version to be merged as a single commit.

mbasmanova · 2017-09-06T03:06:03Z

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

+            xCoordinate += polygon.getPoint(i).getX();
+            yCoordinate += polygon.getPoint(i).getY();
+        }
+        Point centroid = new Point(xCoordinate / pointCount, yCoordinate / pointCount);


According to https://en.wikipedia.org/wiki/Centroid this implementation calculates centroid of a set of points and not centroid of a polygon. The formula for the latter is more complex and includes an area of the polygon.

Also, for an polygon "POLYGON EMPTY", will pointCount be 0 and will it cause a division-by-zero error?

mbasmanova · 2017-09-06T03:23:02Z

@martint , @electrum , re: "are all the functions in this PR defined in the standard?" - Yes, with the exception of ST_XMin, ST_XMax, ST_YMin, ST_YMax. These functions are missing from SQL/MM, but are available in PostGIS. They return the coordinates of the bounding box. How would you like to proceed with these? Would it be sufficient to document that these are non-standard?

re: "Have we looked at what's their expected behavior and whether our implementation matches?" - I looked at all of the functions in GeoFunctions.java and cross-checked their names and implementation with SQL/MM. With a couple exceptions, things are looking good. We need to work through the implementation of ST_Centroid and I have a question about ST_Crosses.

Overall, I'm wondering what are your thoughts on documentation for the functions. Given that SQL/MM standard is not freely available, should we provide more comprehensive explanation of the functions and include examples? Or should we refer folks to PostGIS documentation? Personally, I found PostGIS docs very clear. Should we also mention somewhere that while these functions follow SQL/MM standard they do not implement it in full?

zhenxiao · 2017-09-06T07:19:59Z

thank you @mbasmanova
get your comments addressed. I also put a comment for ST_Crosses
Added additional commits to previous ones

mbasmanova · 2017-09-06T14:58:51Z

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

+        return Slices.utf8Slice(deserialize(input).asText());
+    }
+
+    @Description("Returns the Point value that is the mathematical centroid of the set of points in Polygon")


SQL/MM defines ST_Centroid as "returns the ST_Point value that is the mathematical centroid of the ST_Surface value" which is different from the implementation of this function. PostGIS implements ST_Centroid according to the standard [1].

Why do you want a different implementation for this function?

https://postgis.net/docs/ST_Centroid.html

mbasmanova · 2017-09-06T15:01:05Z

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

+    }
+
+    @SqlNullable
+    @Description("Returns TRUE if the supplied geometries have some, but not all, interior points in common")


@zhenxiao , I think you are right. Thanks for clarifying.

mbasmanova

@zhenxiao , @martint , the functions look good to me, except for ST_Centroid (see comments below).

mbasmanova · 2017-09-06T15:02:34Z

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

+    public static Slice stBoundary(@SqlType(NAME) Slice input)
+    {
+        OGCGeometry geometry = deserialize(input);
+        OGCGeometry bound = geometry.boundary();


nit: this variable seems not needed; if you like to keep it, consider renaming to 'boundary'

mbasmanova · 2017-09-06T15:04:32Z

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

+    {
+        OGCGeometry leftGeometry = deserialize(left);
+        OGCGeometry rightGeometry = deserialize(right);
+        checkArgument(leftGeometry.getEsriSpatialReference().equals(rightGeometry.getEsriSpatialReference()), "Input Geometries Spatial Reference do not match");


nit: this check is copy-pasted in multiple places; consider moving it into a helper method?

verifySameSR(leftGeometry, rightGeometry);

mbasmanova · 2017-09-06T15:08:47Z

presto-geospatial/src/main/java/com/facebook/presto/geospatial/GeometryUtils.java

+        int wkid = input.SRID();
+        Geometry geometry = input.getEsriGeometry();
+
+        if (geometry == null) {


When input is an instance of GeometryCollection, the geometry will be null. Therefore serializing GeometryCollection will always produce an empty slice. Is this intentional?

--- from com.esri.core.geometry.ogc.OGCConcreteGeometryCollection

public Geometry getEsriGeometry() {
return null;
}

public GeometryCursor getEsriGeometryCursor() { return new OGCConcreteGeometryCollection.GeometryCursorOGC(this.geometries); }

yes, I did not find a good way to serialize GeometryCollection. What do you think?

@zhenxiao , at first I thought that it might be OK to not support GeometryCollection by disallowing GeometryCollection in GeometryFromText. However, the problem is that GeometryCollection may be created as a result of some of the operations. For example, ST_Intersection may return a collection:

ST_Intersection(ST_GeometryFromText('POLYGON ((0 0, 0 1, 1 1, 1 0, 0 0))'), ST_GeometryFromText('LINESTRING (-1 1, 1 -1, 1 2)'))

returns a point and a linestring: GEOMETRYCOLLECTION (POINT (0 0), LINESTRING (1 0, 1 1))

GeometryCollection can be serialized as an array of geometries. ArrayBlockBuilder may be handy. One option is for serialize to always produce an array of geometries and for deserialize to create GEOMETRYCOLLECTION when array size > 1 and GEOMETRYCOLLECTION EMPTY when array is empty. What do you think?

mbasmanova · 2017-09-06T15:10:41Z

presto-geospatial/src/main/java/com/facebook/presto/geospatial/GeometryUtils.java

+        }
+        byte[] shape = GeometryEngine.geometryToEsriShape(geometry);
+
+        if (shape == null) {


Is this possible? Under what circumstances GeometryEngine.geometryToEsriShape(geometry) returns null for non-null input? Is this the case for empty geometries? Wouldn't it be clearer to check that before calling geometryToEsriShape?

shape will never be null, will update

mbasmanova · 2017-09-06T15:14:52Z

presto-geospatial/src/main/java/com/facebook/presto/geospatial/GeometryUtils.java

+    public static OGCGeometry deserialize(Slice shape)
+    {
+        requireNonNull(shape, "shape is null");
+        int wkid = shape.getInt(0);


Will this throw an exception If shape is an EMPTY_SLICE?

It might be helpful to add tests cases for serialize and deserialize functions to make sure all the cases work properly.

mbasmanova · 2017-09-06T15:16:17Z

presto-geospatial/src/main/java/com/facebook/presto/geospatial/GeometryUtils.java

+        requireNonNull(shape, "shape is null");
+        int wkid = shape.getInt(0);
+        ByteBuffer shapeBuffer = getShapeBuffer(shape);
+        if (shapeBuffer.limit() < SIZE_WKID || shapeBuffer.getInt(0) == Geometry.Type.Unknown.value()) {


The 'shape' slice has 2 fileds: (1) wkid, (2) shape buffer. It seems strange to check the size of the shape buffer against the size of the WKID field. Would you clarify?

zhenxiao · 2017-09-07T03:42:06Z

@mbasmanova get comments addressed
Added implementation for ST_Centroid

mbasmanova

@zhenxiao , thanks for updating the implementation of ST_Centroid. I think it is now correctly computing centroid for a point, a set of points, a line and a set of lines. The computation for polygons doesn't seem correct though. See details below.

mbasmanova · 2017-09-07T17:33:35Z

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

+        double ySum = 0;
+        for (int i = 0; i < pointCount; i++) {
+            Point point = null;
+            if (type.equals(MULTI_POINT)) {


This logic can be simplified by capturing the result of (MultiVertexGeometry) geometry.getEsriGeometry() in a variable in the beginning of the method and using MultiVertexGeometry.getPoint(i) method.

mbasmanova · 2017-09-07T17:42:41Z

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

+            weightSum += weight;
+            double polygonXSum = 0;
+            double polygonYSum = 0;
+            for (int j = 0; j < polygon.getPointCount(); j++) {


Why do you compute the centroid of a polygon as a mean of its vertexes? Based on [1] and [2] the formula is different.

[1] http://www.seas.upenn.edu/~sys502/extra_materials/Polygon%20Area%20and%20Centroid.pdf
[2] https://en.wikipedia.org/wiki/Centroid#Properties

zhenxiao · 2017-09-07T20:30:01Z

@mbasmanova thanks for your review
Get ST_Centroid fixed for Polygon
let me think about GeometryCollection

mbasmanova

@zhenxiao , thanks for updating the implementation of ST_Centroid. It looks correct to me now.

mbasmanova · 2017-09-07T21:51:51Z

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

+        for (int i = 0; i < polygon.getPointCount(); i++) {
+            Point current = polygon.getPoint(i);
+            Point next = (i + 1 == polygon.getPointCount()) ? polygon.getPoint(0) : polygon.getPoint(i + 1);
+            xSum += (current.getX() + next.getX()) * (current.getX() * next.getY() - next.getX() * current.getY());


nit: (current.getX() * next.getY() - next.getX() * current.getY()) computation repeats 3 times; consider saving the results in a variable for efficiently and readability

mbasmanova · 2017-09-07T21:52:35Z

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

+    // Points centroid is arithmetic mean of the input points
+    private static Point getPointsCentroid(OGCGeometry geometry)
+    {
+        String type = geometry.geometryType();


'type' variable doesn't seem to be used

mbasmanova · 2017-09-07T23:20:29Z

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

+    // Polygon centroid:
+    // c[x] = (Sigma(x[i] + x[i + 1]) * (x[i] * y[i + 1] - x[i + 1] * y[i]), for i = 0 to N - 1) / (6 * signedArea)
+    // c[y] = (Sigma(y[i] + y[i + 1]) * (x[i] * y[i + 1] - x[i + 1] * y[i]), for i = 0 to N - 1) / (6 * signedArea)
+    private static Point getPolygonCentroid(OGCGeometry geometry)


@zhenxiao , I just realized that this formula applies only to polygons without holes. The general case of polygon with one or more holes requires a bit more computation. I haven't found an official formula yet, but the following Stack Overflow answer may be correct. They compute the centroid as a weighted diff between outer inner polygons.

https://math.stackexchange.com/questions/623841/finding-centroid-of-polygon-with-holes-polygons/623849#623849

Seems that is more computation for it. How about we add a comment, and explicitly not support polygons with holes?

mbasmanova · 2017-09-07T23:22:34Z

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

+        double signedArea = 0;
+        for (int i = 0; i < polygon.getPointCount(); i++) {
+            Point current = polygon.getPoint(i);
+            Point next = (i + 1 == polygon.getPointCount()) ? polygon.getPoint(0) : polygon.getPoint(i + 1);


nit: WKT format for polygon requires that first and last points be the same. Hence, it may be sufficient to loop from 0 to polygon.getPointCount() - 1.

zhenxiao · 2017-09-08T02:59:10Z

with @mbasmanova 's help, added serialization/deserialization for GeometryCollection
comments addressed

mbasmanova

@zhenxiao , thanks for updating the tests.

mbasmanova · 2017-09-14T15:29:21Z

presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java

+    public void testSTLineFromText()
+            throws Exception
+    {
+        assertQuery("select ST_AsText(ST_LineFromText('LINESTRING EMPTY'))", "select 'MULTILINESTRING EMPTY'");


It looks like ST_AsText(ST_Point(input)) doesn't preserve the type of the input geometry when geometry is empty. Here LineString got converted to MultiLineString. Further down in this file I also see that Polygon gets converted to MultiPolygon. Is this expected? Perhaps, worth documenting.

yes, this is expected. Since when ST_AsText trying to deserialize, it only knows it is a polyline, not clear whether it is a line or multiline. I will document it.

mbasmanova · 2017-09-14T15:31:27Z

presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java

+    {
+        assertQuery("select ST_AsText(ST_LineFromText('LINESTRING EMPTY'))", "select 'MULTILINESTRING EMPTY'");
+        assertQuery("select ST_AsText(ST_LineFromText('LineString(1 1, 2 2, 1 3)'))", "select 'LINESTRING (1 1, 2 2, 1 3)'");
+        assertQueryFails("select ST_AsText(ST_LineFromText('MULTILineString EMPTY'))", "ST_LineFromText only applies to LineString. Input type is: MultiLineString");


nit: throughout this test suite, the WKT strings are not consistent. For example, there are LineString, MULTILineString, MULTILINESTRING, etc. Would you take a pass and make them all align. Perhaps, uppercase all the WKT type prefixes?

'POLYGON...', 'POINT...', 'LINESTRING...'

mbasmanova · 2017-09-14T15:33:51Z

presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java

+    {
+        assertQuery("select ST_Area(ST_GeometryFromText('Polygon ((2 2, 2 6, 6 6, 6 2))'))", "select 16.0");
+        assertQueryFails("select ST_Area(ST_GeometryFromText('Point (1 4)'))", "ST_Area only applies to Polygon. Input type is: Point");
+        assertQueryFails("select ST_Area(ST_GeometryFromText('POLYGON EMPTY'))", "ST_Area only applies to Polygon. Input type is: MultiPolygon");


ST_Area for an empty polygon should not fail. According to SQL/MM spec it should return null. Also, I noticed that ST_Length for empty line-string returns 0. Let's make these two method consistent: both return 0 or null.

8.1.2 ST_Area Methods ...If SELF is an empty set, then return the null value.

BTW, why does ST_Area not allow multi-polygons?

mbasmanova · 2017-09-14T15:40:22Z

presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java

+        assertQuery("select ST_NumInteriorRing(ST_GeometryFromText('POLYGON ((0 0, 0 5, 5 5, 5 0, 0 0))'))", "select 0");
+        assertQuery("select ST_NumInteriorRing(ST_GeometryFromText('POLYGON ((0 0, 8 0, 0 8, 0 0), (1 1, 1 5, 5 1, 1 1))'))", "select 1");
+        assertQueryFails("select ST_NumInteriorRing(ST_GeometryFromText('LineString(8 4, 5 7)'))", "ST_NumInteriorRing only applies to Polygon. Input type is: LineString");
+        assertQueryFails("select ST_NumInteriorRing(ST_GeometryFromText('POLYGON EMPTY'))", "ST_NumInteriorRing only applies to Polygon. Input type is: MultiPolygon");


According to the spec, ST_NumInteriorRing of an empty Polygon should return null.

mbasmanova · 2017-09-14T15:41:23Z

presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java

+        assertQuery("select ST_AsText(ST_EndPoint(ST_GeometryFromText('LineString(8 4, 4 8, 5 6)')))", "select 'POINT (5 6)'");
+        assertQueryFails("select ST_AsText(ST_StartPoint(ST_GeometryFromText('Polygon ((2 0, 2 1, 3 1))')))", "ST_StartPoint only applies to LineString. Input type is: Polygon");
+        assertQueryFails("select ST_AsText(ST_EndPoint(ST_GeometryFromText('Polygon ((2 0, 2 1, 3 1))')))", "ST_EndPoint only applies to LineString. Input type is: Polygon");
+        assertQueryFails("select ST_AsText(ST_StartPoint(ST_GeometryFromText('LINESTRING EMPTY')))", "ST_StartPoint only applies to LineString. Input type is: MultiLineString");


ST_StartPoint of a empty line-string should return null

mbasmanova · 2017-09-14T15:42:21Z

presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java

+        assertQuery("select ST_X(ST_GeometryFromText('Point (1 2)'))", "select 1.0");
+        assertQuery("select ST_Y(ST_GeometryFromText('Point (1 2)'))", "select 2.0");
+        assertQueryFails("select ST_Y(ST_GeometryFromText('Polygon ((2 0, 2 1, 3 1))'))", "ST_Y only applies to Point. Input type is: Polygon");
+        assertQueryFails("select ST_Y(ST_GeometryFromText('POINT EMPTY'))", "This operation should not be performed on an empty geometry.");


according to the spec, ST_Y of empty point should return null

mbasmanova · 2017-09-14T15:43:20Z

presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java

+        assertQuery("select ST_AsText(ST_ExteriorRing(ST_GeometryFromText('POLYGON ((1 1, 1 4, 4 1))')))", "select 'LINESTRING (1 1, 4 1, 1 4, 1 1)'");
+        assertQuery("select ST_AsText(ST_ExteriorRing(ST_GeometryFromText('POLYGON ((0 0, 0 5, 5 5, 5 0, 0 0), (1 1, 1 2, 2 2, 2 1, 1 1))')))", "select 'LINESTRING (0 0, 5 0, 5 5, 0 5, 0 0)'");
+        assertQueryFails("select ST_AsText(ST_ExteriorRing(ST_GeometryFromText('LineString(1 1, 2 2, 1 3)')))", "ST_ExteriorRing only applies to Polygon. Input type is: LineString");
+        assertQueryFails("select ST_AsText(ST_ExteriorRing(ST_GeometryFromText('POLYGON EMPTY')))", "ST_ExteriorRing only applies to Polygon. Input type is: MultiPolygon");


according to the spec, ST_ExteriorRing of empty polygon should return null

zhenxiao · 2017-09-15T03:23:44Z

@mbasmanova get comments addressed

martint · 2017-09-18T17:53:51Z

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

+
+Presto GeoSpatial functions are compliant with the Open Geospatial Consortium’s (OGC) OpenGIS Specifications. As such, many Presto GeoSpatial functions require, or more accurately, assume that geometries that are operated on are both simple and valid. For example, it does not make sense to calculate the area of a polygon that has a hole defined outside of the polygon, or to construct a polygon from a non-simple boundary line.
+
+Presto GeoSpatial fucntions support the Well-Known Text (WKT) form of spatial objects. Here are the examples:


typo: fucntions

martint · 2017-09-18T19:04:55Z

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

+    @Description("Returns a Geometry type LineString object from Well-Known Text representation (WKT)")
+    @ScalarFunction("ST_LineFromText")
+    @SqlType(NAME)
+    public static Slice stLineString(@SqlType(StandardTypes.VARCHAR) Slice input)


Call this method parseLine.

martint · 2017-09-18T19:08:38Z

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

+import io.airlift.slice.Slices;
+import org.apache.commons.lang.StringUtils;
+
+import static com.facebook.presto.geospatial.GeometryType.NAME;


NAME is too generic to import it statically.

martint · 2017-09-18T19:11:25Z

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

+
+    @Description("Returns the area of a polygon using Euclidean measurement on a 2D plane (based on spatial ref) in projected units")
+    @ScalarFunction("ST_Area")
+    @SqlType(StandardTypes.DOUBLE)


Are these functions defined as returning double in the spec? (vs Decimal, for instance)

It is defined as float.

@martint , @zhenxiao , here is a snippet from the SQL/MM

METHOD ST_Area()
RETURNS DOUBLE PRECISION
LANGUAGE SQL
DETERMINISTIC
CONTAINS SQL
RETURNS NULL ON NULL INPUT,

martint · 2017-09-18T19:28:03Z

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

+        return leftGeometry.within(rightGeometry);
+    }
+
+    private static OGCGeometry getGeometry(OGCGeometry geometry, String function, String...validTypes)


The name of this method is misleading. It's just validating that the type is one of validTypes.

BTW, the cost of this function could be non-trivial (due to having to create an array for the varargs argument and the loop/equality check against multiple types). A better approach might be to create an enum of well-known types and rewrite this as:

private static void validateType(String function, OGCGeometry geometry, Set<GeometryTypeName> validTypes) { GeometryTypeName actual = GeometryTypeName.valueOf(geometry.getType()); if (!validTypes.contains(actual)) { throw ... } }

and call it like:

validateType("ST_xyz", geometry, EnumSet.of(ST_POINT, ST_LINE));

(also, note the slight change in order of arguments. I think that makes a little more logical sense)

martint · 2017-09-18T23:01:28Z

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

+            ySum += (current.getY() + next.getY()) * ladder;
+            signedArea += ladder;
+        }
+        return new Point(xSum / (signedArea * 3), ySum / (signedArea * 3));


The comments before the method say "6 * signedArea", but this multiplies by 3. Which one is it?

it is 6. sum of ladder actually is 2 * signedArea, I merged computation there. Will update

martint · 2017-09-18T23:10:57Z

presto-geospatial/src/main/java/com/facebook/presto/geospatial/GeometryType.java

+            return null;
+        }
+
+        return format("<%s>", getTypeSignature()).getBytes();


While there isn't a strict contract for what getObjectValue should return, it's a good idea to return a json-serializable object that represents the value. This is currently used by the client protocol and can be handy for tests.

I am not very clear about it, is it good to return typeSignature?

It might be better to return the WKT for the value, if possible. This is what's going to be rendered by the CLI

will update

martint · 2017-09-18T23:28:15Z

presto-geospatial/src/main/java/com/facebook/presto/geospatial/GeometryUtils.java

+        }
+        final BasicSliceInput input = shape.getInput();
+
+        int wkid = input.readInt();


From looking at the rest of the code, this is the ID of the coordinate system (or spatial reference), so I'd call it just that (i.e., spatialReferenceId, coordinateSystemId, etc). wkid is not descriptive enough.

martint · 2017-09-18T23:36:46Z

presto-geospatial/src/main/java/com/facebook/presto/geospatial/GeometryUtils.java

+        List<OGCGeometry> geometries = new ArrayList<>();
+        while (input.available() > 0) {
+            int length = input.readInt();
+            byte[] geometryBytes = new byte[length];


You could just do:

ByteBuffer buffer = input.readSlice(length).toByteBuffer()

Alternatively, you could keep track of the position you're reading at and just call shape.toByteBuffer(offset, length) to avoid having to create the intermediate geometryBytes

get it. Will fix it

martint · 2017-09-18T23:45:43Z

presto-server/src/main/provisio/presto.xml

@@ -86,6 +86,12 @@
        </artifact>
    </artifactSet>

+    <artifactSet to="plugin/geo">


geospatial?

mbasmanova

@zhenxiao , thanks for addressing the comments. This PR is looking good to me.

mbasmanova · 2017-09-19T02:49:24Z

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

    {
-        OGCGeometry geometry = getGeometry(deserialize(input), "ST_NumInteriorRing", POLYGON);
-        return ((OGCPolygon) geometry).numInteriorRing();
+        OGCGeometry geometry = getGeometry(deserialize(input), "ST_NumInteriorRing", POLYGON, MULTI_POLYGON);


This must be a type. ST_NumInteriorRing applies only to polygons. It doesn't apply to multi-polygon. Also, the implementation casts geometry to OGCPolygon. This cast will fail if geometry is multi-polygon. Right?

mbasmanova · 2017-09-19T02:51:31Z

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

    @Description("Returns the first point of a LINESTRING geometry as a Point")
    @ScalarFunction("ST_StartPoint")
    @SqlType(NAME)
    public static Slice stStartPoint(@SqlType(NAME) Slice input)
    {
-        OGCGeometry geometry = getGeometry(deserialize(input), "ST_StartPoint", LINE_STRING);
+        OGCGeometry geometry = getGeometry(deserialize(input), "ST_StartPoint", LINE_STRING, MULTI_LINE_STRING);


My reading of the SQL/MM suggests that ST_StartPoint and ST_EndPoint are defined only for line strings. These are not defined for multi-line strings.

zhenxiao · 2017-09-19T05:02:47Z

@mbasmanova @martint thanks for your comments
get comments addressed

martint · 2017-09-19T18:57:15Z

Thanks @zhenxiao. It's getting very close.

You seem to have missed this comment in your latest batch of changes: 501e33c#r139566928

zhenxiao · 2017-09-20T01:05:28Z

thank you, @martint just addressed missing comments

martint · 2017-09-20T15:44:15Z

presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java

+import static com.facebook.presto.tpch.TpchMetadata.TINY_SCHEMA_NAME;
+
+public class TestGeoQueries
+        extends AbstractTestQueryFramework


Any reason these test run full queries instead of using AbstractTestFunctions (see, for instance, TestJsonFunctions)

zhenxiao · 2017-09-21T00:22:44Z

Thank you, @martint I get comments addressed

martint

Looks good, but there seem to be some build failures. Can you take a look?

zhenxiao · 2017-09-22T18:58:58Z

@martint seems when running presto-product-tests, it does not build main test module, which I added getTypeRegistry() method.
Do you know how to run product-tests locally? I think if it first build main test module, should be OK

highker · 2017-09-22T19:01:46Z

@zhenxiao probably it's a testbed issue; I have restarted

zhenxiao · 2017-09-22T19:03:03Z

nice of you, @highker

nezihyigitbasi

Sorry for the last minute comments everyone. @zhenxiao please also squash the commits before we merge this.

nezihyigitbasi · 2017-09-22T19:19:27Z

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

+GeoSpatial Functions
+=====================
+
+Presto GeoSpatial functions support SQL/MM specification


The first sentence is somewhat isolated from the rest of the doc.

btw the SQL/MM specification.

nezihyigitbasi · 2017-09-22T19:22:11Z

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

+
+.. function:: ST_Contains(Geometry, Geometry) -> boolean
+
+    Returns TRUE if and only if no points of right lie in the exterior of left, and at least one point of the interior of left lies in the interior of right.


left, right meaning the first and second argument I guess?

nezihyigitbasi · 2017-09-22T19:22:38Z

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

+
+.. function:: ST_Intersects(Geometry, Geometry) -> boolean
+
+    Returns TRUE if the Geometries spatially intersect in 2D - (share any portion of space) and FALSE if they don't (they are Disjoint)


Disjoint -> disjoint.

nezihyigitbasi · 2017-09-22T19:23:19Z

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

+
+.. function:: ST_Disjoint(Geometry, Geometry) -> boolean
+
+    Returns TRUE if the Geometries do not "spatially intersect" - if they do not share any space together.


given geometries (as you used in the function below, also doesn't make sense to capitalize Geometries).

nezihyigitbasi · 2017-09-22T19:23:26Z

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

+
+.. function:: ST_Intersects(Geometry, Geometry) -> boolean
+
+    Returns TRUE if the Geometries spatially intersect in 2D - (share any portion of space) and FALSE if they don't (they are Disjoint)


given geometries

nezihyigitbasi · 2017-09-22T19:24:59Z

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

+
+.. function:: ST_Distance(Geometry, Geometry) -> double
+
+    Returns the 2-dimensional cartesian minimum distance (based on spatial ref) between two Geometries in projected units.


you use 2D above but 2-dimensional here, intentional?

nezihyigitbasi · 2017-09-22T19:26:36Z

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

+
+    Returns the last point of a LineString Geometry as a Point.
+
+.. function:: ST_X(point) -> double


any reason why you don't capitalize as Point as you do for other functions?

nezihyigitbasi · 2017-09-22T19:27:13Z

presto-geospatial/pom.xml

+            <version>2.4</version>
+        </dependency>
+
+        <!-- Presto SPI -->


unnecessary comment

nezihyigitbasi · 2017-09-22T19:29:45Z

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

+    @SqlType(GEOMETRY_TYPE_NAME)
+    public static Slice stPoint(@SqlType(StandardTypes.DOUBLE) double x, @SqlType(StandardTypes.DOUBLE) double y)
+    {
+        OGCGeometry geometry = OGCGeometry.createFromEsriGeometry(new Point(x, y), null);


static import createFromEsriGeometry

nezihyigitbasi · 2017-09-22T19:32:52Z

presto-geospatial/src/main/java/com/facebook/presto/geospatial/GeometryUtils.java

+    public static Slice serialize(OGCGeometry input)
+    {
+        int spatialReferenceId = input.SRID();
+        final DynamicSliceOutput sliceOutput = new DynamicSliceOutput(100);


we don't usually declare local variables as final.

nezihyigitbasi · 2017-09-22T19:35:43Z

there is a compilation problem @zhenxiao

[ERROR] /home/travis/build/prestodb/presto/presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java:[36,31] cannot find symbol
  symbol:   method getTypeRegistry()
  location: variable functionAssertions of type com.facebook.presto.operator.scalar.FunctionAssertions
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.0:testCompile (default-testCompile) on project presto-geospatial: Compilation failure
[ERROR] /home/travis/build/prestodb/presto/presto-geospatial/src/test/java/com/facebook/presto/geospatial/TestGeoQueries.java:[36,31] cannot find symbol
[ERROR]   symbol:   method getTypeRegistry()
[ERROR]   location: variable functionAssertions of type com.facebook.presto.operator.scalar.FunctionAssertions
[ERROR] -> [Help 1]

zhenxiao · 2017-09-22T20:51:44Z

Thank you @nezihyigitbasi I will work on it
@highker do you have any hints how to run the product tests locally? I am trying to reproduce it
Seems my main-test updates did not get compiled for this test

nezihyigitbasi · 2017-09-22T21:46:10Z

@zhenxiao why did you add <type>test-jar</type> for the presto-main module to the presto-geospatial/pom.xml file?

zhenxiao · 2017-09-22T21:58:07Z

@nezihyigitbasi getting complain about using undeclared dependencies without adding test-jar for presto-main

nezihyigitbasi · 2017-09-22T22:09:03Z

please update the version in presto-geospatial/pom.xml to 0.186-SNAPSHOT. we have just released a new version.

zhenxiao · 2017-09-22T22:22:04Z

@nezihyigitbasi get your comments addressed
Updated to the latest version
not sure about the product tests

zhenxiao · 2017-09-22T23:27:37Z

aha, product tests running good now
The only failure is a timeout, not related to this PR
@nezihyigitbasi may be let it run one more time?

nezihyigitbasi · 2017-09-22T23:29:14Z

The tests look good (except a known flaky one) so I merged the PR. Thanks @zhenxiao!

facebook-github-bot added the CLA Signed label Aug 30, 2017

zhenxiao mentioned this pull request Aug 30, 2017

GeoSpatial functions and optimization in Presto #5435

Closed

zhenxiao force-pushed the geoSpatial branch from 748aef1 to e0098df Compare August 30, 2017 05:07

mbasmanova requested changes Aug 30, 2017

View reviewed changes

martint suggested changes Aug 30, 2017

View reviewed changes

zhenxiao force-pushed the geoSpatial branch from e0098df to 4ef3660 Compare August 31, 2017 01:02

electrum reviewed Aug 31, 2017

View reviewed changes

zhenxiao force-pushed the geoSpatial branch from 4ef3660 to e0c6d2b Compare August 31, 2017 23:13

mbasmanova requested changes Sep 5, 2017

View reviewed changes

mbasmanova reviewed Sep 5, 2017

View reviewed changes

zhenxiao force-pushed the geoSpatial branch from e0c6d2b to 4ea2f91 Compare September 5, 2017 23:52

mbasmanova reviewed Sep 6, 2017

View reviewed changes

mbasmanova reviewed Sep 7, 2017

View reviewed changes

mbasmanova reviewed Sep 14, 2017

View reviewed changes

martint suggested changes Sep 18, 2017

View reviewed changes

martint assigned zhenxiao Sep 19, 2017

mbasmanova reviewed Sep 19, 2017

View reviewed changes

zhenxiao force-pushed the geoSpatial branch from 4da8a5f to 501e33c Compare September 19, 2017 05:03

martint reviewed Sep 20, 2017

View reviewed changes

martint approved these changes Sep 22, 2017

View reviewed changes

nezihyigitbasi reviewed Sep 22, 2017

View reviewed changes

zhenxiao force-pushed the geoSpatial branch from 6ff8b89 to 6d1492d Compare September 22, 2017 21:34

zhenxiao force-pushed the geoSpatial branch from 6d1492d to dd1dc72 Compare September 22, 2017 21:57

Support GeoSpatial functions in Presto

1aa3e84

zhenxiao force-pushed the geoSpatial branch from dd1dc72 to 1aa3e84 Compare September 22, 2017 22:18

nezihyigitbasi closed this Sep 22, 2017

zhenxiao deleted the geoSpatial branch December 9, 2017 02:25


		Returns the Y coordinate of point.

		.. function:: st_point_number(geometry) -> bigint


		Presto GeoSpatial functions are compliant with the Open Geospatial Consortium’s (OGC) OpenGIS Specifications. As such, many Presto GeoSpatial functions require, or more accurately, assume that geometries that are operated on are both simple and valid. For example, it does not make sense to calculate the area of a polygon that has a hole defined outside of the polygon, or to construct a polygon from a non-simple boundary line.

		Presto GeoSpatial fucntions support the Well-Known Text (WKT) form of spatial objects. Here are the examples:


		.. function:: ST_Contains(Geometry, Geometry) -> boolean

		Returns TRUE if and only if no points of right lie in the exterior of left, and at least one point of the interior of left lies in the interior of right.


		.. function:: ST_Intersects(Geometry, Geometry) -> boolean

		Returns TRUE if the Geometries spatially intersect in 2D - (share any portion of space) and FALSE if they don't (they are Disjoint)


		.. function:: ST_Disjoint(Geometry, Geometry) -> boolean

		Returns TRUE if the Geometries do not "spatially intersect" - if they do not share any space together.


		.. function:: ST_Distance(Geometry, Geometry) -> double

		Returns the 2-dimensional cartesian minimum distance (based on spatial ref) between two Geometries in projected units.


		Returns the last point of a LineString Geometry as a Point.

		.. function:: ST_X(point) -> double

Support GeoSpatial functions in Presto #8878

Support GeoSpatial functions in Presto #8878

Conversation

zhenxiao commented Aug 30, 2017

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

martint 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

mbasmanova Aug 31, 2017 • edited 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

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

zhenxiao commented Aug 31, 2017

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

electrum commented Aug 31, 2017

mbasmanova commented Aug 31, 2017

zhenxiao commented Aug 31, 2017

zhenxiao commented Aug 31, 2017

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

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

zhenxiao commented Sep 5, 2017

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

mbasmanova Aug 31, 2017 •

edited

Loading