-
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-178] Correctness issue in distance joins #701
Conversation
|
||
<!-- Actual scala version will be set by a profile. | ||
Setting a default value helps IDE:s that can't make sense of profiles. --> | ||
<scala.compat.version>2.12</scala.compat.version> |
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.
This is what spark does. Default values in properties section and overrides in profiles. Without it Intellij can't make sense of the project.
@@ -134,14 +134,15 @@ class functionTestScala extends TestBaseScala with Matchers with GeometrySample | |||
var polygonDf = sparkSession.sql("select ST_GeomFromWKT(polygontable._c0) as countyshape from polygontable") | |||
polygonDf.createOrReplaceTempView("polygondf") | |||
val polygon = "POLYGON ((110.54671 55.818002, 110.54671 55.143743, 110.940494 55.143743, 110.940494 55.818002, 110.54671 55.818002))" | |||
val forceXYExpect = "POLYGON ((471596.69167460164 6185916.951191288, 471107.5623640998 6110880.974228167, 496207.109151055 6110788.804712435, 496271.31937046186 6185825.60569904, 471596.69167460164 6185916.951191288))" | |||
// Floats don't have an exact decimal representation. String format varies across jvm:s. Do an approximate match. |
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.
This fixes builds on java 11. The string representation of floats changed between java 8 and java 11.
There are other tests in sedona-core that fail on java 11 but this fixes sedona-sql
@@ -128,11 +128,10 @@ class SpatialJoinSuite extends TestBaseScala with TableDrivenPropertyChecks { | |||
case "ST_Touches" => (l: Geometry, r: Geometry) => l.touches(r) | |||
case "ST_Within" => (l: Geometry, r: Geometry) => l.within(r) | |||
case "ST_Distance" => | |||
// XXX: ST_Distance has a weird behavior, it is wildly different from `l.distance(r)`. |
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.
This is an interesting comment!
It locks like r-lib/actions removed the branch "master" |
I changed the GitHub Action to r-lib new master branch but failed at some other issues. I will merge this PR anyway as it passed all other tests. |
Did you read the Contributor Guide?
Is this PR related to a JIRA ticket?
[SEDONA-178] my subject
.What changes were proposed in this PR?
Sedona SQL doesn't rely on Circle for distance joins. Correct results are now produces for all geometry types.
Performance could be improved but would require some refactoring of the spatial partitioning code.
See the jira for discussion.
How was this patch tested?
All unit tests pass. New tests added.
Did this PR include necessary documentation updates?