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

[SEDONA-113] Add ST_PointN to Apache Sedona #621

Merged
merged 10 commits into from
Apr 24, 2022

Conversation

Pooja444
Copy link
Contributor

Did you read the Contributor Guide?

Is this PR related to a JIRA ticket?

What changes were proposed in this PR?

Added ST_PointN function to Flink and SQL

How was this patch tested?

Added unit tests in Java, Scala and Python

Did this PR include necessary documentation updates?

  • Yes, I have updated the documentation update.

}

public static Geometry getExteriorRing(Geometry geometry) {
return geometry.getFactory().createLinearRing(geometry.getCoordinates());
Copy link
Member

Choose a reason for hiding this comment

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

Use this function in JTS Polygon. Dont create LinearRing by coordinates as they might NOT be the rings.https://locationtech.github.io/jts/javadoc/org/locationtech/jts/geom/Polygon.html#getExteriorRing--

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I updated this

docs/api/flink/Function.md Show resolved Hide resolved
@Pooja444 Pooja444 requested a review from jiayuasu April 23, 2022 04:59
FROM df
```

Input: `POLYGON((0 0, 1 1, 2 1, 0 1, 1 -1))`
Copy link
Member

Choose a reason for hiding this comment

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

The example here is NOT a valid WKT format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it

assert point != null;
Assert.assertEquals("POINT (0.5 0.5)", point.toString());
}

Copy link
Member

Choose a reason for hiding this comment

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

Please add a test for exterior ring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@jiayuasu jiayuasu changed the title [Sedona-113] Add ST_PointN to Apache Sedona [SEDONA-113] Add ST_PointN to Apache Sedona Apr 23, 2022
@Pooja444 Pooja444 requested a review from jiayuasu April 23, 2022 06:43
override def nullable: Boolean = true

lazy val GeometryFactory = new GeometryFactory()
lazy val emptyGeometry: GeometryCollection = GeometryFactory.createGeometryCollection(null)
Copy link
Member

Choose a reason for hiding this comment

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

This emptyGeometry is NOT consistent with PostGIS requirement. ST_PointN should return NULL if no input linestring. You return an empty GeometryCollection but not NULL. https://postgis.net/docs/ST_PointN.html

Please fix this and add corresponding test cases for non-line string input.

@Pooja444 Pooja444 requested a review from jiayuasu April 24, 2022 01:53
@jiayuasu jiayuasu merged commit 2e5c6ef into apache:master Apr 24, 2022
@Pooja444 Pooja444 deleted the sedona-113 branch April 29, 2022 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants