Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to esri-geometry-api version 2.2.0 #11157

Merged
merged 1 commit into from
Aug 3, 2018

Conversation

tdcmeehan
Copy link
Contributor

@tdcmeehan tdcmeehan commented Jul 30, 2018

Updating the esri-geometry-api to leverage better support for geometry collections.

@@ -848,10 +848,10 @@ public void testSTUnion()

// within union
assertUnion("MULTIPOINT ((20 20), (25 25))", "POINT (25 25)", "MULTIPOINT ((20 20), (25 25))");
assertUnion("LINESTRING (20 20, 30 30)", "POINT (25 25)", "LINESTRING (20 20, 30 30)");
assertUnion("LINESTRING (20 20, 30 30)", "POINT (25 25)", "LINESTRING (20 20, 25 25, 30 30)");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the addition of point (25, 25) on the output.

@martint
Copy link
Contributor

martint commented Jul 30, 2018

What’s the motivation for updating? Are there specific bug fixes or performance improvements that would benefit presto?

@tdcmeehan
Copy link
Contributor Author

tdcmeehan commented Jul 30, 2018

@martint I would like to add support for the ST_Union aggregation function (there is already a scalar version of this, see #10921). In order for this to be completely useful as an aggregation function, the Esri library must have robust support for geometry collections (for example, to gracefully handle situations where we union disjoint geometries). Unfortunately, as I understand it, prior to version 2.2.0, there were several outstanding issues relating to collections (see Esri/geometry-api-java#177 and Esri/geometry-api-java#176). By upgrading the library to version 2.2.0, it will unblock the work required to add support for the ST_Union aggregation function, and potentially other operations on geometry collections.

As a quick example of why this may be required, in version 2.1.0 of the library, if I union these two geometries:
LINESTRING (5 5, 7 7) and POLYGON ((2 2, 3 1, 1 1, 2 2))
I receive: POLYGON ((2 2, 1 1, 3 1, 2 2)) (i.e. the LINESTRING is silently dropped). In version 2.2.0, it is GEOMETRYCOLLECTION (LINESTRING (5 5, 7 7), POLYGON ((1 1, 3 1, 2 2, 1 1))).

CC: @mbasmanova

@mbasmanova
Copy link
Contributor

@martint 2.2.0 version adds support for geometry collections to multiple operations: Esri/geometry-api-java#176 . It will allow us to fix #10628

@martint
Copy link
Contributor

martint commented Jul 30, 2018

Can we add that to the commit message?

@tdcmeehan tdcmeehan force-pushed the esrigeometry branch 2 times, most recently from 5d78b0b to 1b4e765 Compare July 30, 2018 20:06
Upgrading the Esri geometry library unblocks the fix for prestodb#10628, and adds support for geometry collections to multiple operations.
@tdcmeehan
Copy link
Contributor Author

tdcmeehan commented Aug 1, 2018

My testing has discovered a corner case: Esri/geometry-api-java#192 It's probably best to wait until that is fixed before we update this dependency.

@mbasmanova
Copy link
Contributor

@tdcmeehan Tim, thanks for reporting this issue. It would be nice to fix it, but in the meantime we can workaround by adding an geometry.isEmpty() check before calling estimateMemorySize(). The latest version of the library didn't include any changes to this API, so it is an existing issue and shouldn't block the upgrade which adds support for geometry collections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants