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.

Sign up for GitHub

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

Jump to bottom

[SEDONA-196] Add ST_Force3D #856

Merged
merged 24 commits into from
Jun 12, 2023
Merged

[SEDONA-196] Add ST_Force3D #856

merged 24 commits into from
Jun 12, 2023

Conversation

iGN5117
Copy link
Contributor

@iGN5117 iGN5117 commented Jun 9, 2023

Did you read the Contributor Guide?

Is this PR related to a JIRA ticket?

What changes were proposed in this PR?

  • Add ST_Force3D to sedona

How was this patch tested?

  • Comprehensive correctness test cases in java
  • Integration test cases

Did this PR include necessary documentation updates?

iGN5117 added 18 commits June 6, 2023 13:29
@iGN5117
Remove prebuild_index config from search plugin
85ae113
@iGN5117
Update compile the code documentation for sedona
8e829fe
@iGN5117
approximate float comparisons in python test cases
4bcf99c
@iGN5117
Revert "Update compile the code documentation for sedona"
15c6255 
This reverts commit c7f6236.
@iGN5117
Revert "Remove prebuild_index config from search plugin"
471e281 
This reverts commit 85ae113.
@iGN5117
FIxed incorrect indentation
2a5a6d8
@iGN5117
Add ST_NumPoints
cf7e78d
@iGN5117
Update available version to 1.4.1 in documentation
fc23fc9
@iGN5117
Merge branch 'apache:master' into develop_Nilesh_1.4.0
fea1daa
@iGN5117
Fix failing scala test case
816e4ed
@iGN5117
Merge branch 'develop_Nilesh_1.4.0' of https://github.com/iGN5117/sedona
472c383 
 into develop_Nilesh_1.4.0
@iGN5117
Updated documentation to include negative flow.
471a87e 
Changed generic Exception to IllegalArgumentException in ST_NumPoints implementation and its corresponding test
@iGN5117
Removed Spark SQL from flink documentation
b33499d
@iGN5117
Add ST_Force3D to sedona
0d68337
@iGN5117
Merge branch 'sedona-master' into develop_Nilesh_1.4.0
d1e4365 
# Conflicts:
#	common/src/main/java/org/apache/sedona/common/Functions.java
#	common/src/test/java/org/apache/sedona/common/FunctionsTest.java
#	flink/src/main/java/org/apache/sedona/flink/Catalog.java
#	flink/src/main/java/org/apache/sedona/flink/expressions/Functions.java
#	flink/src/test/java/org/apache/sedona/flink/FunctionTest.java
#	python/sedona/sql/st_functions.py
#	python/tests/sql/test_function.py
#	sql/common/src/main/scala/org/apache/sedona/sql/UDF/Catalog.scala
#	sql/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Functions.scala
#	sql/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/st_functions.scala
#	sql/common/src/test/scala/org/apache/sedona/sql/dataFrameAPITestScala.scala
#	sql/common/src/test/scala/org/apache/sedona/sql/functionTestScala.scala
@iGN5117
Updated force3D logic to handle empty geometries
da0314a 
Refactored function name
Made java tests more comprehensive by checking both nDims and WKT of returned geometry
Added more test cases in scala test cases
Updated documentation with empty geometry case and more examples
@iGN5117
Updated force3D dataframe test
020e9b9
@iGN5117
fix error in test case
86de054
jiayuasu
jiayuasu requested changes Jun 11, 2023
View reviewed changes
flink/src/test/java/org/apache/sedona/flink/FunctionTest.java Show resolved Hide resolved
sql/common/src/test/scala/org/apache/sedona/sql/dataFrameAPITestScala.scala Outdated
@@ -957,5 +957,13 @@ class dataFrameAPITestScala extends TestBaseScala {
val expectedResult = 3
assert(actualResult == expectedResult)
}

it("Passed ST_Force3D") {
val lineDf = sparkSession.sql("SELECT ST_Force3D(ST_GeomFromWKT('LINESTRING (0 1, 1 0, 2 0)'), 2.3) AS geom")
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 the test case that has no zvalue supplied.

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

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is, add a test case like ST_Force3D(geom), no z value is supplied. This will test the correctness of the the optional parameter.

Copy link
Member

Choose a reason for hiding this comment

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

In addition, the test cases didn't test the correctness of the DataFrame style API, it mistakenly repeated the SQL test cases. Please read other test cases in this file and learn how to test DataFrame style APIs.

Explanation of Sedona DataFrame style APIs: https://sedona.apache.org/latest-snapshot/api/sql/DataFrameAPI/

sql/common/src/test/scala/org/apache/sedona/sql/functionTestScala.scala Show resolved Hide resolved
flink/src/main/java/org/apache/sedona/flink/Catalog.java Show resolved Hide resolved
docs/api/sql/Function.md Outdated

Input: `LINESTRING(0 1, 1 2, 2 1)`

Output: `LINESTRING(0 1 0, 1 2 0, 2 1 0)`
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the output LINESTRING Z?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you're right, updated the documentation with a note explaining the output

@jiayuasu jiayuasu added this to the sedona-1.4.1 milestone Jun 11, 2023
jiayuasu
jiayuasu requested changes Jun 11, 2023
View reviewed changes
sql/common/src/main/scala/org/apache/sedona/sql/UDF/Catalog.scala Outdated
@@ -148,6 +148,7 @@ object Catalog {
function[ST_AreaSpheroid](),
function[ST_LengthSpheroid](),
function[ST_NumPoints](),
function[ST_Force3D](),
Copy link
Member

Choose a reason for hiding this comment

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

Please use ST_MinimumBoundingCirle's implementation as an example: (1) Function definition: https://github.com/apache/sedona/blob/master/sql/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Functions.scala#LL473C12-L473C36
(2) Function catalog registration: https://github.com/apache/sedona/blob/master/sql/common/src/main/scala/org/apache/sedona/sql/UDF/Catalog.scala#L102

The default value of the optional parameters must be supplied in the Function Catalog otherwise this is not an optional parameter.

sql/common/src/test/scala/org/apache/sedona/sql/dataFrameAPITestScala.scala Outdated
@@ -957,5 +957,13 @@ class dataFrameAPITestScala extends TestBaseScala {
val expectedResult = 3
assert(actualResult == expectedResult)
}

it("Passed ST_Force3D") {
val lineDf = sparkSession.sql("SELECT ST_Force3D(ST_GeomFromWKT('LINESTRING (0 1, 1 0, 2 0)'), 2.3) AS geom")
Copy link
Member

Choose a reason for hiding this comment

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

What I mean is, add a test case like ST_Force3D(geom), no z value is supplied. This will test the correctness of the the optional parameter.

sql/common/src/test/scala/org/apache/sedona/sql/functionTestScala.scala Show resolved Hide resolved
sql/common/src/test/scala/org/apache/sedona/sql/dataFrameAPITestScala.scala Outdated
@@ -957,5 +957,13 @@ class dataFrameAPITestScala extends TestBaseScala {
val expectedResult = 3
assert(actualResult == expectedResult)
}

it("Passed ST_Force3D") {
val lineDf = sparkSession.sql("SELECT ST_Force3D(ST_GeomFromWKT('LINESTRING (0 1, 1 0, 2 0)'), 2.3) AS geom")
Copy link
Member

Choose a reason for hiding this comment

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

In addition, the test cases didn't test the correctness of the DataFrame style API, it mistakenly repeated the SQL test cases. Please read other test cases in this file and learn how to test DataFrame style APIs.

Explanation of Sedona DataFrame style APIs: https://sedona.apache.org/latest-snapshot/api/sql/DataFrameAPI/

@jiayuasu jiayuasu merged commit 800a3f5 into apache:master Jun 12, 2023
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jiayuasu jiayuasu jiayuasu approved these changes

Assignees
No one assigned
Labels
affect public APIs improvement sedona-sql
Projects
None yet
Milestone
sedona-1.4.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@iGN5117 @jiayuasu