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-237] Add ST_Dimension #867

Merged
merged 23 commits into from
Jun 25, 2023
Merged

[SEDONA-237] Add ST_Dimension #867

merged 23 commits into from
Jun 25, 2023

Conversation

yyy1000
Copy link
Contributor

@yyy1000 yyy1000 commented Jun 22, 2023

Did you read the Contributor Guide?

Is this PR related to a JIRA ticket?

What changes were proposed in this PR?

Add ST_Dimension function to sedona-common, sedona-sql, sedona-flink, and the dataframe/python API.

How was this patch tested?

  1. Comprehensive unit tests in sedona-common
  2. Integration unit tests in sedona-sql, sedona-flink, and python.

Did this PR include necessary documentation updates?

  • Yes, I have updated the documentation update.


Format: `ST_Dimension (A:geometry), ST_Dimension (C:geometrycolletion), `

Since: `v1.0.0`
Copy link
Member

Choose a reason for hiding this comment

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

Should be v1.5.0

Copy link
Member

Choose a reason for hiding this comment

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

Please provide docs for Sedona Flink as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


Spark SQL example:
```sql
SELECT ST_Dimension('GEOMETRYCOLLECTION(LINESTRING(1 1,0 0),POINT(0 0))');
Copy link
Member

Choose a reason for hiding this comment

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

Please provide the output of the example.

In addition, have you tested other possible types, such as MultiPoint, MultiLineString, MultiPolygon. GeometryCollection can also have nested Multi objects.

We are trying to provide consistent behavior as PostGIS. Please install PostGIS on your end and test the output in PostGIS as well.

@@ -268,6 +268,38 @@ public void splitHeterogeneousGeometryCollection() {
assertNull(actualResult);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please separate different testCases to different tests since these are unit tests.

You can have tests like dimensionGeom2D, dimensionGeomCollection, dimensionGeomEmpty, etc.

Also, use assertEquals(expected, actual) format so that the test failure flags expected and actual properly.

Additionally, please add test cases testing 3D geometry as well

@@ -137,6 +137,20 @@ class functionTestScala extends TestBaseScala with Matchers with GeometrySample
assert(functionDf.count() > 0);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You can follow the map way of specifying multiple test cases and their expected output here, it is more readable


Introduction: Return the topological dimension of this Geometry object, which must be less than or equal to the coordinate dimension. OGC SPEC s2.1.1.1 - returns 0 for POINT, 1 for LINESTRING, 2 for POLYGON, and the largest dimension of the components of a GEOMETRYCOLLECTION. If the dimension is unknown (e.g. for an empty GEOMETRYCOLLECTION) 0 is returned.

Format: `ST_Dimension (A:geometry), ST_Dimension (C:geometrycolletion), `
Copy link
Contributor

Choose a reason for hiding this comment

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

type in collection here


Introduction: Return the topological dimension of this Geometry object, which must be less than or equal to the coordinate dimension. OGC SPEC s2.1.1.1 - returns 0 for POINT, 1 for LINESTRING, 2 for POLYGON, and the largest dimension of the components of a GEOMETRYCOLLECTION. If the dimension is unknown (e.g. for an empty GEOMETRYCOLLECTION) 0 is returned.

Format: `ST_Dimension (A:geometry), ST_Dimension (C:geometrycolletion), `
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in collection here

@iGN5117
Copy link
Contributor

iGN5117 commented Jun 22, 2023

Additionally, I noticed that your PR description does not have anything in the "How was this PR tested". Please fill that section too.

Example PR: #742

@yyy1000
Copy link
Contributor Author

yyy1000 commented Jun 24, 2023

Ah, sorry for the CI problem. It is not clear for the error message. XD. It's OK now.

@jiayuasu jiayuasu merged commit 20e0de4 into apache:master Jun 25, 2023
@yyy1000 yyy1000 deleted the dimension branch June 25, 2023 07:06
Kontinuation pushed a commit to Kontinuation/sedona that referenced this pull request Oct 11, 2024
Co-authored-by: Jia Yu <jiayu@apache.org>
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.

3 participants