You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sedona.apache.org by "yyy1000 (via GitHub)" <gi...@apache.org> on 2023/07/04 10:38:05 UTC

[GitHub] [sedona] yyy1000 opened a new pull request, #887: [SEDONA-303] Port all Sedona Spark functions to Sedona Flink -- Step 4

yyy1000 opened a new pull request, #887:
URL: https://github.com/apache/sedona/pull/887

   ## Did you read the Contributor Guide?
   
   - Yes, I have read [Contributor Rules](https://sedona.apache.org/latest-snapshot/community/rule/) and [Contributor Development Guide](https://sedona.apache.org/latest-snapshot/community/develop/)
   
   
   ## Is this PR related to a JIRA ticket?
   
   - Yes, the URL of the associated JIRA ticket is https://issues.apache.org/jira/browse/SEDONA-303. The PR name follows the format `[SEDONA-XXX] my subject`.
   
   
   ## What changes were proposed in this PR?
   1. Port missing ST_functions including 
   {ST_PrecisionReduce and ST_MinimumBoundingCircle}to [sedona-flink](https://issues.apache.org/jira/browse/SEDONA-flink).
   2. Change the default value of the second param of ST_MinimumBoundingCircle from 8 to 48 and its tests. See https://postgis.net/docs/ST_MinimumBoundingCircle.html for reference.
   3. Change the name of ST_PrecisionReduce to ST_ReducePrecision, and its tests and docs, see https://postgis.net/docs/ST_ReducePrecision.html for reference. 
   
   ## How was this patch tested?
   1. Integration unit tests in [sedona-flink](https://issues.apache.org/jira/browse/SEDONA-flink).
   2. Integration unit tests in [sedona-sql](https://issues.apache.org/jira/browse/SEDONA-sql), [sedona-flink](https://issues.apache.org/jira/browse/SEDONA-flink), and python.
   
   
   ## Did this PR include necessary documentation updates?
   - Yes, I have updated the documentation update.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [sedona] jiayuasu commented on a diff in pull request #887: [SEDONA-303] Port all Sedona Spark functions to Sedona Flink -- Step 4

Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #887:
URL: https://github.com/apache/sedona/pull/887#discussion_r1255231174


##########
docs/api/sql/Function.md:
##########
@@ -1552,18 +1552,18 @@ SELECT ST_AsText(ST_PointOnSurface(ST_GeomFromText('LINESTRING(0 5 1, 0 0 1, 0 1
 
 ```
 
-## ST_PrecisionReduce
+## ST_ReducePrecision
 
 Introduction: Reduce the decimals places in the coordinates of the geometry to the given number of decimal places. The last decimal place will be rounded.
 
-Format: `ST_PrecisionReduce (A:geometry, B:int)`

Review Comment:
   em... I think the Sedona implementation of this function is very different from PostGIS. I suggest that, in this PR, we don't adopt the PostGIS implementation. We will address this in a different PR.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [sedona] yyy1000 commented on a diff in pull request #887: [SEDONA-303] Port all Sedona Spark functions to Sedona Flink -- Step 4

Posted by "yyy1000 (via GitHub)" <gi...@apache.org>.
yyy1000 commented on code in PR #887:
URL: https://github.com/apache/sedona/pull/887#discussion_r1255179690


##########
docs/api/sql/Function.md:
##########
@@ -1552,18 +1552,18 @@ SELECT ST_AsText(ST_PointOnSurface(ST_GeomFromText('LINESTRING(0 5 1, 0 0 1, 0 1
 
 ```
 
-## ST_PrecisionReduce
+## ST_ReducePrecision
 
 Introduction: Reduce the decimals places in the coordinates of the geometry to the given number of decimal places. The last decimal place will be rounded.
 
-Format: `ST_PrecisionReduce (A:geometry, B:int)`

Review Comment:
   I just noticed that `ST_ReducePrecision(geometry g, float8 gridsize);` in postgis needs float as the second param. That means use '0.1' as 1, '0.01' as 2. Should we follow the function in postgis?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [sedona] jiayuasu commented on a diff in pull request #887: [SEDONA-303] Port all Sedona Spark functions to Sedona Flink -- Step 4

Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #887:
URL: https://github.com/apache/sedona/pull/887#discussion_r1255711819


##########
docs/api/flink/Function.md:
##########
@@ -1244,6 +1244,19 @@ Result:
 +------------------+------------------------+
 ```
 
+## ST_MinimumBoundingCircle
+
+Introduction: Returns the smallest circle polygon that contains a geometry. The optional quadrantSegments parameter determines how many segments to use per quadrant and the default number of segments is 48. 
+
+Format: `ST_MinimumBoundingCircle(geom: geometry, [Optional] quadrantSegments:int)`
+
+Since: `v1.0.1`

Review Comment:
   Why is this version reverted back to v1.0.1? All these functions should be v1.5.0 @yyy1000 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [sedona] jiayuasu commented on pull request #887: [SEDONA-303] Port all Sedona Spark functions to Sedona Flink -- Step 4

Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on PR #887:
URL: https://github.com/apache/sedona/pull/887#issuecomment-1620943844

   @yyy1000 Please fix the failed Python tests


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [sedona] yyy1000 commented on pull request #887: [SEDONA-303] Port all Sedona Spark functions to Sedona Flink -- Step 4

Posted by "yyy1000 (via GitHub)" <gi...@apache.org>.
yyy1000 commented on PR #887:
URL: https://github.com/apache/sedona/pull/887#issuecomment-1625286090

   I updated the doc. :)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [sedona] jiayuasu merged pull request #887: [SEDONA-303] Port all Sedona Spark functions to Sedona Flink -- Step 4

Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu merged PR #887:
URL: https://github.com/apache/sedona/pull/887


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [sedona] yyy1000 commented on a diff in pull request #887: [SEDONA-303] Port all Sedona Spark functions to Sedona Flink -- Step 4

Posted by "yyy1000 (via GitHub)" <gi...@apache.org>.
yyy1000 commented on code in PR #887:
URL: https://github.com/apache/sedona/pull/887#discussion_r1255243093


##########
docs/api/sql/Function.md:
##########
@@ -1552,18 +1552,18 @@ SELECT ST_AsText(ST_PointOnSurface(ST_GeomFromText('LINESTRING(0 5 1, 0 0 1, 0 1
 
 ```
 
-## ST_PrecisionReduce
+## ST_ReducePrecision
 
 Introduction: Reduce the decimals places in the coordinates of the geometry to the given number of decimal places. The last decimal place will be rounded.
 
-Format: `ST_PrecisionReduce (A:geometry, B:int)`

Review Comment:
   Yeah, I think so. Here we can keep Sedona implementation. Should we use the keep the function name the same as PostGIS like 'ST_ReducePrecision '? Or we can revert to the old name 'ST_PrecisionReduce ', which doesn't exsit in PostGIS.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [sedona] jiayuasu commented on a diff in pull request #887: [SEDONA-303] Port all Sedona Spark functions to Sedona Flink -- Step 4

Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #887:
URL: https://github.com/apache/sedona/pull/887#discussion_r1255137234


##########
docs/api/sql/Function.md:
##########
@@ -1552,18 +1552,18 @@ SELECT ST_AsText(ST_PointOnSurface(ST_GeomFromText('LINESTRING(0 5 1, 0 0 1, 0 1
 
 ```
 
-## ST_PrecisionReduce
+## ST_ReducePrecision
 
 Introduction: Reduce the decimals places in the coordinates of the geometry to the given number of decimal places. The last decimal place will be rounded.
 
-Format: `ST_PrecisionReduce (A:geometry, B:int)`

Review Comment:
   Please add one more sentence saying that the name of this function was called ST_PrecisionReduce in versions prior to v1.5.0.



##########
sql/common/src/main/scala/org/apache/sedona/sql/UDF/Catalog.scala:
##########
@@ -102,7 +102,7 @@ object Catalog {
     function[ST_ClosestPoint](),
     function[ST_Boundary](),
     function[ST_MinimumBoundingRadius](),
-    function[ST_MinimumBoundingCircle](BufferParameters.DEFAULT_QUADRANT_SEGMENTS),

Review Comment:
   Please update the doc in Sedona Spark saying that the default param has been changed.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [sedona] jiayuasu commented on a diff in pull request #887: [SEDONA-303] Port all Sedona Spark functions to Sedona Flink -- Step 4

Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #887:
URL: https://github.com/apache/sedona/pull/887#discussion_r1255565235


##########
docs/api/sql/Function.md:
##########
@@ -1552,18 +1552,18 @@ SELECT ST_AsText(ST_PointOnSurface(ST_GeomFromText('LINESTRING(0 5 1, 0 0 1, 0 1
 
 ```
 
-## ST_PrecisionReduce
+## ST_ReducePrecision
 
 Introduction: Reduce the decimals places in the coordinates of the geometry to the given number of decimal places. The last decimal place will be rounded.
 
-Format: `ST_PrecisionReduce (A:geometry, B:int)`

Review Comment:
   @yyy1000 I think changing to `ST_ReducePrecision` is fine. No need to revert it back. Just update the doc then we are good to go.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [sedona] yyy1000 commented on pull request #887: [SEDONA-303] Port all Sedona Spark functions to Sedona Flink -- Step 4

Posted by "yyy1000 (via GitHub)" <gi...@apache.org>.
yyy1000 commented on PR #887:
URL: https://github.com/apache/sedona/pull/887#issuecomment-1621003494

   > @yyy1000 Please fix the failed Python tests
   
   Fixed it. :)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [sedona] yyy1000 commented on a diff in pull request #887: [SEDONA-303] Port all Sedona Spark functions to Sedona Flink -- Step 4

Posted by "yyy1000 (via GitHub)" <gi...@apache.org>.
yyy1000 commented on code in PR #887:
URL: https://github.com/apache/sedona/pull/887#discussion_r1255730774


##########
docs/api/flink/Function.md:
##########
@@ -1244,6 +1244,19 @@ Result:
 +------------------+------------------------+
 ```
 
+## ST_MinimumBoundingCircle
+
+Introduction: Returns the smallest circle polygon that contains a geometry. The optional quadrantSegments parameter determines how many segments to use per quadrant and the default number of segments is 48. 
+
+Format: `ST_MinimumBoundingCircle(geom: geometry, [Optional] quadrantSegments:int)`
+
+Since: `v1.0.1`

Review Comment:
   Ah, sorry for missing that. Updated.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org