Skip to content

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

Merged
jiayuasu merged 10 commits into
apache:masterfrom
Pooja444:sedona-113
Apr 24, 2022
Merged

[SEDONA-113] Add ST_PointN to Apache Sedona#621
jiayuasu merged 10 commits into
apache:masterfrom
Pooja444:sedona-113

Conversation

@Pooja444

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread docs/api/flink/Function.md
Comment thread sql/src/main/scala/org/apache/sedona/sql/UDF/Catalog.scala
@Pooja444
Pooja444 requested a review from jiayuasu April 23, 2022 04:59
Comment thread docs/api/flink/Function.md Outdated
FROM df
```

Input: `POLYGON((0 0, 1 1, 2 1, 0 1, 1 -1))`

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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