-
Notifications
You must be signed in to change notification settings - Fork 656
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-303] Port all Sedona Spark functions to Sedona Flink -- Step 4 #887
Conversation
@yyy1000 Please fix the failed Python tests |
Fixed it. :) |
|
||
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)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add one more sentence saying that the name of this function was called ST_PrecisionReduce in versions prior to v1.5.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@@ -102,7 +102,7 @@ object Catalog { | |||
function[ST_ClosestPoint](), | |||
function[ST_Boundary](), | |||
function[ST_MinimumBoundingRadius](), | |||
function[ST_MinimumBoundingCircle](BufferParameters.DEFAULT_QUADRANT_SEGMENTS), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the doc in Sedona Spark saying that the default param has been changed.
I updated the doc. :) |
docs/api/flink/Function.md
Outdated
|
||
Format: `ST_MinimumBoundingCircle(geom: geometry, [Optional] quadrantSegments:int)` | ||
|
||
Since: `v1.0.1` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this version reverted back to v1.0.1? All these functions should be v1.5.0 @yyy1000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry for missing that. Updated.
Did you read the Contributor Guide?
Is this PR related to a JIRA ticket?
[SEDONA-XXX] my subject
.What changes were proposed in this PR?
{ST_PrecisionReduce and ST_MinimumBoundingCircle}to sedona-flink.
How was this patch tested?
Did this PR include necessary documentation updates?