-
Notifications
You must be signed in to change notification settings - Fork 659
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-157] Add coordinate accessors to the Flink API #677
Conversation
@@ -70,6 +70,27 @@ public static double length(Geometry geometry) { | |||
return geometry.getLength(); | |||
} | |||
|
|||
public static Double x(Geometry geometry) { |
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.
Does this need to be the boxed type instead of the native double like the other functions return?
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.
ST_X, ST_Y and ST_Z should return null if the first argument is not a Point, so we have to check the input type somewhere. I chose to do that in the common functions, because if they can direcly return null by using a boxed type, we can simply wrap it with InferredUnaryExpression in the Spark SQL functions. But as you pointed out, returning boxed type looks somewhat strange. Should I change the common functions to return a primitive value? In that case, we have to use UnaryGeometryExpression instead of InferredUnaryExpression so the code will become a bit verbose. Either is fine with me.
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.
Right that makes sense then, nvm
@@ -265,6 +265,27 @@ public void testIsEmpty() { | |||
assertEquals(false, result); | |||
} | |||
|
|||
@Test | |||
public void testX() { |
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.
Can you use the same test in testZ for testX and testY so testX and testY don't need to use point_real?
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.
Updated the PR. Thanks for the review!
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?
This PR adds ST_X, ST_Y and ST_Z to the Flink API.
How was this patch tested?
mvn clean install
locallymkdocs serve
and checked the generate documentDid this PR include necessary documentation updates?
vX.Y.Z
format.