Use JTS instead of ESRI for geometries, Parts 1 and 2 #13604

jagill · 2019-10-25T02:46:37Z

I am in the process of refactoring the geospatial functions to use JTS instead of Esri. Why?

JTS is a more standard library, that is compatible with PostGIS and GEOS and most geometry libraries.
It conforms to the ISO standards better.
Serialization is about 80% of the CPU for many geometrical functions, and
JTS serializes about 20% faster than ESRI, and has the potential of serializing 10-20x faster.

Since geometries are always converted to/from slices, it's fine for some functions to use Esri and some to use JTS (this is the case already). This PR converts most of the "easy" functions (those that don't involve much in the way of logical or numerical changes). However, JTS uses the standard order for polygon rings (counter-clockwise) instead of Esri's clockwise. This means that many of the tests need to have the order of the vertices reversed. I've isolated all of those changes into the commit that uses JTS for conversion from/to WKT.

== RELEASE NOTES ==

General Changes
* Use JTS instead of Esri for many geometrical operations.
  + Polygon WKTs must have closed loops.  Previously Esri would close the loops for you.
  + Certain other invalid geometries will fail to be created from WKTs, such as
    `LINESTRING(0 0, 0 0, 0 0)`.
  + Returned WKTs may have a different point order.
  + Fixes incorrect calculation of extreme points in certain cases.

jagill · 2019-10-25T19:47:34Z

cc @arhimondr

arhimondr · 2019-10-25T20:09:38Z

...to-geospatial/src/test/java/com/facebook/presto/plugin/geospatial/BenchmarkSTIntersects.java

@@ -121,7 +121,7 @@ public void validateBenchmarkData()
        public void setup()
                throws IOException
        {
-            simpleGeometry = stGeometryFromText(utf8Slice("POLYGON ((16.5 54, 16.5 54.1, 16.51 54.1, 16.8 54))"));
+            simpleGeometry = stGeometryFromText(utf8Slice("POLYGON ((16.5 54, 16.5 54.1, 16.51 54.1, 16.8 54, 16.5 54))"));


Make Polygon WKTs valid in test cases

Although it is by the standard - it is a breaking change. Do we care that if somebody had existing WKT stored in the database that were compatible with ESRI it will stop working?

If we are fine with that, should we have a better release note?

arhimondr · 2019-10-25T20:11:03Z

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

 public final class GeometryUtils
 {
+    public static final CoordinateSequenceFactory COORDINATE_SEQUENCE_FACTORY = new PackedCoordinateSequenceFactory();


Are this objects (CoordinateSequenceFactory, GeometryFactory) thread safe?

upd: those are threadsafe, but mutable. Let's make them private

arhimondr · 2019-10-25T20:12:42Z

...eospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestSphericalGeoFunctions.java

@@ -158,9 +158,6 @@ public void testArea()
        // Empty polygon
        assertFunction("ST_Area(to_spherical_geography(ST_GeometryFromText('POLYGON EMPTY')))", DOUBLE, null);

-        // Invalid polygon (too few vertices)


Why are we removing this test case?

Previously, certain types of invalid geometries could be created, but would have to be checked for explicitly in functions such as these. Now, those geometries fail on construction (I've updated the release notes to mention this).

arhimondr · 2019-10-25T20:12:53Z

...eospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestSphericalGeoFunctions.java

@@ -172,7 +169,7 @@ public void testArea()

        assertArea("POLYGON((0 0, 0 1, 1 1, 1 0, 0 0))", 123.64E8);

-        assertArea("POLYGON((-122.150124 37.486095, -122.149201 37.486606,  -122.145725 37.486580, -122.145923 37.483961 , -122.149324 37.482480 ,  -122.150837 37.483238,  -122.150901 37.485392))", 163290.93943446054);
+        assertArea("POLYGON((-122.150124 37.486095, -122.149201 37.486606,  -122.145725 37.486580, -122.145923 37.483961, -122.149324 37.482480,  -122.150837 37.483238,  -122.150901 37.485392, -122.150124 37.486095))", 163290.93943446054);


This seems to belong to the previous commit

arhimondr · 2019-10-25T20:13:03Z

...eospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestSphericalGeoFunctions.java

@@ -210,12 +207,6 @@ public void testLength()
        // Empty linestring returns null
        assertLength("LINESTRING EMPTY", null);

-        // Linestring with one point has length 0


Why are we removing these two?

arhimondr · 2019-10-25T20:14:15Z

...al-toolkit/src/main/java/com/facebook/presto/geospatial/serde/GeometrySerializationType.java

@@ -68,4 +68,28 @@ public static GeometrySerializationType getForCode(int code)
                throw new IllegalArgumentException("Invalid type code: " + code);
        }
    }
+
+    public String toString()


Why do we need this? Why the standard toString from the enum is not good enough? If this is something that we use for actual serialization / deserialization, should we have a separate method and not reuse the toString interface?

This isn't used for serde: it is used for a user-facing error message. It's a bit nicer and conforms to the previous error message. However, I have no strong feelings and I'm happy for users to read MULTI_LINE_STRING.

I've removed it: it's probably better to change the test error message than add a new method that must be maintained.

arhimondr · 2019-10-25T20:22:08Z

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

    }

    @Description("Returns a Geometry type object from Well-Known Binary representation (WKB)")
    @ScalarFunction("ST_GeomFromBinary")
    @SqlType(GEOMETRY_TYPE_NAME)
    public static Slice stGeomFromBinary(@SqlType(VARBINARY) Slice input)
    {
-        return serialize(geomFromBinary(input));
+        return GeometrySerde.serialize(geomFromBinary(input));


The GeometrySerde.serialize and GeometrySerde.deserialize are confusing. Should we rename GeometrySerde to EsriGeometrySerde?

arhimondr · 2019-10-25T20:32:36Z

...al-toolkit/src/main/java/com/facebook/presto/geospatial/serde/GeometrySerializationType.java

+            case ENVELOPE:
+                return "Envelope";
+            default:
+                return this.name();


throw IllegalArgument

arhimondr

The change looks good to me.

The only concern i have - is that we are braking backward compatibility by disallowing "invalid" polygons that don't have the last point. But as we discussed - that should be fine. As such polygons are "invalid".

@mbasmanova Would you like to have a look before we merge this?

arhimondr · 2019-10-30T20:17:13Z

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

 public final class GeometryUtils
 {
+    public static final CoordinateSequenceFactory COORDINATE_SEQUENCE_FACTORY = new PackedCoordinateSequenceFactory();


You are not using them outside anymore, could you please make them private?

arhimondr · 2019-10-30T20:17:41Z

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

@@ -38,8 +40,8 @@

 public final class GeometryUtils
 {
-    public static final CoordinateSequenceFactory COORDINATE_SEQUENCE_FACTORY = new PackedCoordinateSequenceFactory();
-    public static final GeometryFactory GEOMETRY_FACTORY = new GeometryFactory(COORDINATE_SEQUENCE_FACTORY);
+    private static final CoordinateSequenceFactory COORDINATE_SEQUENCE_FACTORY = new PackedCoordinateSequenceFactory();


Oh, i see, you are making them private in the next commit. Could you please move it to the commit before?

linux-foundation-easycla · 2019-11-08T02:06:47Z

The committers are authorized under a signed CLA.

✅ James Gill (7aa067b9283e4f310426ce00c913c3921b295a3a, 1417e52084d2f414e47f44cd747b86863c9bbef4, 3ddf604ba86da57076a4ad67771b322d7988a641, eb2793f3e1aa871c91d03262bffd69ab34323f92)

jagill · 2019-11-08T21:08:11Z

Running the verifier suite, I found only two discrepancies:

The order of polygon WKTs changed in the expected way,
I found some unexpected changes to ST_XMax (and Y and Min) that -- when investigated -- showed the new values matched manual testing. So I'll consider this an unintentional bug fix.

As a first step to converting to JTS, convert the WKT serde functions to JTS. While the code change here is small, JTS uses the standard order for polygon rings (counter-clockwise), while Esri uses the reverse order (clockwise). This means many of the test cases need to have their coordinates reversed. Also, a couple tests which tested invalid geometries will actually be caught in WKT parsing; so they were removed from the other function tests.

Converting most unary properties and operators to JTS. Binary relations and operators were not touched, as well as unary operators that would require significant logic changes.

bhhari · 2020-01-29T22:50:59Z

fixes #14031

facebook-github-bot added the CLA Signed label Oct 25, 2019

arhimondr reviewed Oct 25, 2019

View reviewed changes

jagill force-pushed the use-jts branch 3 times, most recently from 2e9e7d6 to 05bebdf Compare October 30, 2019 18:06

arhimondr approved these changes Oct 30, 2019

View reviewed changes

jagill force-pushed the use-jts branch from 05bebdf to 346e477 Compare October 30, 2019 20:34

jagill force-pushed the use-jts branch 2 times, most recently from dc3f559 to f6149ab Compare November 8, 2019 02:06

jagill force-pushed the use-jts branch 2 times, most recently from d0b4060 to 820ea91 Compare November 8, 2019 20:59

jagill force-pushed the use-jts branch 3 times, most recently from eb2793f to 8d1df35 Compare November 9, 2019 19:38

jagill force-pushed the use-jts branch from 8d1df35 to 1aefa38 Compare November 20, 2019 21:28

jagill force-pushed the use-jts branch from 1aefa38 to 510a2ad Compare December 4, 2019 01:00

Upgrade JTS to 1.16.1

02c79af

jagill force-pushed the use-jts branch from 510a2ad to a0f548d Compare December 6, 2019 16:50

jagill added 3 commits December 6, 2019 13:49

Convert GeoFunctions to JTS: Part 2

9b3a540

Converting most unary properties and operators to JTS. Binary relations and operators were not touched, as well as unary operators that would require significant logic changes.

Rename GeometrySerde to EsriGeometrySerde

d38b153

jagill force-pushed the use-jts branch from a0f548d to d38b153 Compare December 6, 2019 18:50

arhimondr merged commit 46ae108 into prestodb:master Dec 9, 2019

jagill deleted the use-jts branch December 9, 2019 19:10

This was referenced Jan 17, 2020

Add release notes for 0.231 #13980

Closed

Support Release Notes Generation #13964

Closed

caithagoras mentioned this pull request Jan 22, 2020

Add release notes for 0.231 #13997

Merged

6 tasks

yupeng9 mentioned this pull request Jul 9, 2020

[Part 1] Add geo support apache/pinot#5654

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use JTS instead of ESRI for geometries, Parts 1 and 2 #13604

Use JTS instead of ESRI for geometries, Parts 1 and 2 #13604

jagill commented Oct 25, 2019 •

edited

Loading

jagill commented Oct 25, 2019

arhimondr Oct 25, 2019

arhimondr Oct 25, 2019

arhimondr Oct 25, 2019

arhimondr Oct 25, 2019

jagill Oct 25, 2019

arhimondr Oct 25, 2019

arhimondr Oct 25, 2019

jagill Oct 25, 2019

arhimondr Oct 25, 2019

jagill Oct 25, 2019 •

edited

Loading

jagill Oct 25, 2019

arhimondr Oct 25, 2019

arhimondr Oct 25, 2019

arhimondr left a comment

arhimondr Oct 30, 2019

arhimondr Oct 30, 2019

linux-foundation-easycla bot commented Nov 8, 2019 •

edited

Loading

jagill commented Nov 8, 2019

bhhari commented Jan 29, 2020

Use JTS instead of ESRI for geometries, Parts 1 and 2 #13604

Use JTS instead of ESRI for geometries, Parts 1 and 2 #13604

Conversation

jagill commented Oct 25, 2019 • edited Loading

jagill commented Oct 25, 2019

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

jagill Oct 25, 2019 • 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

arhimondr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Nov 8, 2019 • edited Loading

jagill commented Nov 8, 2019

bhhari commented Jan 29, 2020

jagill commented Oct 25, 2019 •

edited

Loading

jagill Oct 25, 2019 •

edited

Loading

linux-foundation-easycla bot commented Nov 8, 2019 •

edited

Loading