[SEDONA-298] Implement ST_ClosestPoint #877
Conversation
| assertEquals(expected2, actual2); | ||
| Point expectedPoint3 = GEOMETRY_FACTORY.createPoint(new Coordinate(125.75342465753425, 115.34246575342466)); | ||
| Double expected3 = Functions.closestPoint(lineString2, point2).distance(expectedPoint3); | ||
| assertEquals(expected3, 0, 1e-3); |
There was a problem hiding this comment.
Why is the tolerance level 1e-3 here? please standardize to 1e-6 or we can even go upto 1e-9.
| @Test | ||
| public void closestPoint() { | ||
| Point point1 = GEOMETRY_FACTORY.createPoint(new Coordinate(1, 1)); | ||
| LineString lineString1 = GEOMETRY_FACTORY.createLineString(coordArray(1, 0, 1, 1, 2, 1, 2, 0, 1, 0)); |
There was a problem hiding this comment.
What happens if one or both the geometries are empty? Please add unit tests testing that.
| @@ -447,6 +448,12 @@ public static Geometry lineFromMultiPoint(Geometry geometry) { | |||
| return GEOMETRY_FACTORY.createLineString(coordinates.toArray(new Coordinate[0])); | |||
| } | |||
|
|
|||
| public static Geometry closestPoint(Geometry left, Geometry right) { | |||
| DistanceOp distanceOp = new DistanceOp(left, right); | |||
| Coordinate[] closestPoints = distanceOp.nearestPoints(); | |||
There was a problem hiding this comment.
Since we're using JTS DistanceOp, what happens if the one or both the input geometries are 3D? Is the z ordinate ignored, or it is supported?
We will have to update documentation and tests accordingly.
| Point expectedPoint4 = GEOMETRY_FACTORY.createPoint(new Coordinate(131.59149149528952, 101.89887534906197)); | ||
| Double expected4 = Functions.closestPoint(polygonA, polygonB).distance(expectedPoint4); | ||
| assertEquals(expected4, 0, 1e-6); | ||
| } |
There was a problem hiding this comment.
Please add a unit test case testing geometry collections
There was a problem hiding this comment.
OK, but I'm a little confused. Though it could work. I search it but can't find any usage about geometry collections. XD
There was a problem hiding this comment.
There could be many possible use cases. Like given a geometry collection representing a neighborhood (polygon/multipolygon for houses), find closest point to a given external point.
|
|
||
| Example1: | ||
| ```sql | ||
| SELECT ST_AsText( ST_ClosestPoint(g1, g2)) As ptwkt; |
There was a problem hiding this comment.
This documentation needs to be updated about 3D geometries, based on testing JTS DistanceOp
There was a problem hiding this comment.
Update documentation here to include the thrown exception if any of the geometry is empty
| return GEOMETRY_FACTORY.createPoint(closestPoints[0]); | ||
| } | ||
| catch (Exception e) { | ||
| return null; |
There was a problem hiding this comment.
Do not throw null here, throw a meaningful exception. If this is thrown if the geometry is empty, throw an IllegalArgumentException with an appropriate message
There was a problem hiding this comment.
Should we throw an exception or just return a null value? I think postgis return an empty value:
postgres=# select ST_AsText(ST_ClosestPoint(ST_GeomFromText('POINT EMPTY'),
ST_GeomFromText('LINESTRING (0 0, 1 0, 2 0, 3 0, 4 0, 5 0)')));
st_astext
-----------
(1 row)
There was a problem hiding this comment.
It is better to throw an exception, in a case where multiple functions are stringed together, which is what happens usually, throwing null means user would see an unexplainable NullPointerException eventually.
By throwing an IllegalArgumentException with a proper message, we will end up helping the user.
There was a problem hiding this comment.
Based on this, I tried to resolve all your comments! You can review again. :) Thanks! 😁
| Point expectedPoint4 = GEOMETRY_FACTORY.createPoint(new Coordinate(131.59149149528952, 101.89887534906197)); | ||
| Double expected4 = Functions.closestPoint(polygonA, polygonB).distance(expectedPoint4); | ||
| assertEquals(expected4, 0, 1e-6); | ||
| } |
There was a problem hiding this comment.
There could be many possible use cases. Like given a geometry collection representing a neighborhood (polygon/multipolygon for houses), find closest point to a given external point.
| assertNull(actual); | ||
|
|
||
| // Both objects are empty | ||
| Polygon emptyPolygon = GEOMETRY_FACTORY.createPolygon(); |
There was a problem hiding this comment.
refactor these test cases to catch the meaningful exception.
There would be 2 asserts, 1 for the type of exception thrown, 1 for the message in the exception.
Check implementation and tests of nRings for reference
|
|
||
| Example1: | ||
| ```sql | ||
| SELECT ST_AsText( ST_ClosestPoint(g1, g2)) As ptwkt; |
There was a problem hiding this comment.
Update documentation here to include the thrown exception if any of the geometry is empty
docs/api/sql/Function.md
Outdated
| ## ST_ClosestPoint | ||
|
|
||
| Introduction: Returns the 2-dimensional point on geom1 that is closest to geom2. This is the first point of the shortest line between the geometries. If using 3D geometries, the Z coordinates will be ignored. If you have a 3D Geometry, you may prefer to use ST_3DClosestPoint. | ||
|
|
There was a problem hiding this comment.
Update documentation here to include the thrown exception if any of the geometry is empty
Did you read the Contributor Guide?
Is this PR related to a JIRA ticket?
[SEDONA-XXX] my subject.What changes were proposed in this PR?
Add ST_ClosestPointfunction to sedona-common, sedona-sql, sedona-flink, and the dataframe/python API.
How was this patch tested?
Did this PR include necessary documentation updates?