[SEDONA-468] Add useSpheroid provision in ST_DWithin #1208

iGN5117 · 2024-01-19T16:40:21Z

Did you read the Contributor Guide?

Yes, I have read Contributor Rules and Contributor Development Guide

Is this PR related to a JIRA ticket?

Yes, the URL of the associated JIRA ticket is https://issues.apache.org/jira/browse/SEDONA-468. The PR name follows the format [SEDONA-XXX] my subject.

What changes were proposed in this PR?

Add provision to useSpheroid parameter in ST_DWithin, which uses ST_DistanceSpheroid
Add optimized spatial and broadcast join support for the new variant

How was this patch tested?

Added new tests

Did this PR include necessary documentation updates?

Yes, I am adding a new API. I am using the current SNAPSHOT version number in since vX.Y.Z format.

into develop_Nilesh_1.5.1 # Conflicts: # python/tests/sql/test_dataframe_api.py # spark/common/src/main/scala/org/apache/sedona/sql/UDF/Catalog.scala # spark/common/src/test/scala/org/apache/sedona/sql/dataFrameAPITestScala.scala

# Conflicts: # spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/strategy/join/JoinQueryDetector.scala # spark/common/src/test/scala/org/apache/sedona/sql/BroadcastIndexJoinSuite.scala # spark/common/src/test/scala/org/apache/sedona/sql/TestBaseScala.scala # spark/common/src/test/scala/org/apache/sedona/sql/predicateJoinTestScala.scala

jiayuasu · 2024-01-20T03:49:13Z

.../common/src/main/scala/org/apache/spark/sql/sedona_sql/strategy/join/JoinQueryDetector.scala

@@ -132,6 +132,12 @@ class JoinQueryDetector(sparkSession: SparkSession) extends Strategy {
          Some(JoinQueryDetection(left, right, leftShape, ST_Buffer(Seq(rightShape, distance)), SpatialPredicate.INTERSECTS, isGeography = false, condition, Some(extraCondition)))
        case Some(And(extraCondition, ST_DWithin(Seq(leftShape, rightShape, distance)))) =>
          Some(JoinQueryDetection(left, right, leftShape, ST_Buffer(Seq(rightShape, distance)), SpatialPredicate.INTERSECTS, isGeography = false, condition, Some(extraCondition)))
+        case Some(ST_DWithin(Seq(leftShape, rightShape, distance, useSpheroid))) =>


When useSpheroid is true , this should not use ST_Buffer because buffering a lon/lat geometry with meter distance does not make any sense and will lead to wrong result. When useSpheroid = true, we should make it equivalent to ST_DistanceSpheroid() < XXX.

I think we should totally get rid of ST_Buffer to avoid confusion. All ST_DWithin patterns, once get matched, should be translated to distance join, like what we did for ST_Distance and ST_DistanceSpheroid. Buffering a geometry with a spheroid distance is already taken care by the optimized distance join triggered ST_DistanceSpheroid() < XXX, which is quite complex.

case Some(LessThanOrEqual(ST_Distance(Seq(leftShape, rightShape)), distance)) => Some(JoinQueryDetection(left, right, leftShape, rightShape, SpatialPredicate.INTERSECTS, false, condition, Some(distance))) // ST_DistanceSpheroid case Some(LessThanOrEqual(ST_DistanceSpheroid(Seq(leftShape, rightShape)), distance)) => Some(JoinQueryDetection(left, right, leftShape, rightShape, SpatialPredicate.INTERSECTS, true, condition, Some(distance)))

Got it, done

jiayuasu · 2024-01-20T03:58:16Z

docs/api/flink/Predicate.md

@@ -60,9 +60,11 @@ true

 ## ST_DWithin

-Introduction: Returns true if 'leftGeometry' and 'rightGeometry' are within a specified 'distance'. This function essentially checks if the shortest distance between the envelope of the two geometries is <= the provided distance.
+Introduction: Returns true if 'leftGeometry' and 'rightGeometry' are within a specified 'distance' (in metres). This function essentially checks if the shortest distance between the envelope of the two geometries is <= the provided distance.


Please change every Sphere to Spheroid in the doc and other code. In the geodesic distance world, Sphere distance means great circle distance while spheroid distance means geodesic distance. These are 2 different things.

In our case, we use spheroid not sphere.

In addition, it is worth noting that when we use spheroid distance, there is a current limitation in Sedona: we calculate the distance between the centroids of two geometries, not the shortest distance between two geometries. This tiny error usually does not matter much to end users because spheroid distances are mainly used for getting the distance between two objects across the globe.

The explanation should be

Introduction: Returns true if 'leftGeometry' and 'rightGeometry' are within a specified 'distance'.

If useSpheroid is passed true, ST_DWithin uses Sedona's ST_DistanceSpheroid to check the spheroid distance between the centroids of two geometries. The unit of the distance in this case is meter.

If useSpheroid is passed false, ST_DWithin uses Euclidean distance and the unit of the distance is the same as the CRS of the geometries. To obtain the correct result, please consider using ST_Transform to put data in an appropriate CRS.

If useSpheroid is not given, it defaults to false

jiayuasu · 2024-01-22T04:10:21Z

docs/api/flink/Predicate.md

@@ -60,9 +60,11 @@ true

 ## ST_DWithin

-Introduction: Returns true if 'leftGeometry' and 'rightGeometry' are within a specified 'distance'. This function essentially checks if the shortest distance between the envelope of the two geometries is <= the provided distance.
+Introduction: Returns true if 'leftGeometry' and 'rightGeometry' are within a specified 'distance' (in metres). This function essentially checks if the shortest distance between the envelope of the two geometries is <= the provided distance.


The explanation should be

Introduction: Returns true if 'leftGeometry' and 'rightGeometry' are within a specified 'distance'.

If useSpheroid is passed true, ST_DWithin uses Sedona's ST_DistanceSpheroid to check the spheroid distance between the centroids of two geometries. The unit of the distance in this case is meter.

If useSpheroid is passed false, ST_DWithin uses Euclidean distance and the unit of the distance is the same as the CRS of the geometries. To obtain the correct result, please consider using ST_Transform to put data in an appropriate CRS.

If useSpheroid is not given, it defaults to false

jiayuasu · 2024-01-22T04:12:40Z

docs/api/sql/Predicate.md


-Format: `ST_DWithin (leftGeometry: Geometry, rightGeometry: Geometry, distance: Double)`
+If `useSpheroid` is passed true, ST_DWithin uses Sedona's ST_DistanceSpheroid. If `useSpheroid` is passed false or not passed at all, DWithin uses Euclidean distance.


Please use the same explanation above.

jiayuasu · 2024-01-22T04:25:46Z

spark/common/src/test/scala/org/apache/sedona/sql/TestBaseScala.scala

@@ -95,6 +99,28 @@ trait TestBaseScala extends FunSpec with BeforeAndAfterAll {
    sparkSession.read.format("binaryFile").load(path)
  }

+
+  def generateTestData(): Seq[(Int, Double, Geometry)] = {


Manually generating test data is good. But please do not use randomized data for test because this will create uncertain behaviors in the test and it is really hard to debug. In addition, please make sure it takes a parameter that allows to generate any size of test data as desired.

In addition, when create test data for ST_DWithin join, please make sure you know what the exact number of rows should appear in the result. And you should always use test cases that produce non-zero results.

jiayuasu · 2024-01-26T03:22:57Z

@Kontinuation Please take a look when you have time. Thanks!

.../common/src/main/scala/org/apache/spark/sql/sedona_sql/strategy/join/JoinQueryDetector.scala

…essions

…rgl" This reverts commit 5352fb9.

jiayuasu · 2024-01-29T17:12:57Z

@Kontinuation Would you please review again when you have time? Thanks!

* Add ST_DWithin * Add documentation for ST_DWithin * Remove unwanted code * removed null check test for ST_DWithin * Fix EOF lint error * Add explanation for ST_DWithin * Remove CRS checking logic in ST_DWithin * Add optimized join support for ST_DWithin * Remove test change to resourceFolder * remove unnecessary cast to double * Add broadcast join test * Add example of ST_DWithin in Optimizer.md * Add useSpheroid version to ST_DWithin | Add optimized join support * remove accidental resourceFolder change * Fix mistake in making useSpheroid optional in ST_DWithin * Fix incorrect test data in test_dataframe_api.py * fix failing test in test_predicate.py * Address PR changes | Move ST_DWithin to DistanceJoin * fix failing test * Remove randomness from sphere test case generation * Refactor documentation of ST_DWithin * revert resourceFolder path * Handle complex boolean expressions in ST_DWithin * add a blanket try catch for ST_DWithin to handle complex boolean expressions * add collect to the python test * replace head() with count() * Add null check for geometry column while adding a df to keplergl * Revert "Add null check for geometry column while adding a df to keplergl" This reverts commit 5352fb9.

iGN5117 added 19 commits January 1, 2024 11:05

Add ST_DWithin

21258e8

Add documentation for ST_DWithin

e887506

Remove unwanted code

f1b3f84

removed null check test for ST_DWithin

341017d

Fix EOF lint error

4f6f09d

Add explanation for ST_DWithin

a41339d

Remove CRS checking logic in ST_DWithin

043223d

Add optimized join support for ST_DWithin

0079af5

Remove test change to resourceFolder

1c61199

remove unnecessary cast to double

f9ee1ec

Add broadcast join test

8a0fc7d

Add example of ST_DWithin in Optimizer.md

158325f

Add useSpheroid version to ST_DWithin | Add optimized join support

cda4802

remove accidental resourceFolder change

91f3ca7

Fix mistake in making useSpheroid optional in ST_DWithin

3bf9c99

Fix incorrect test data in test_dataframe_api.py

ecac80d

fix failing test in test_predicate.py

57f3f94

jiayuasu requested changes Jan 20, 2024

View reviewed changes

jiayuasu added improvement attention needed sedona-common sedona-spark labels Jan 20, 2024

jiayuasu modified the milestones: 1.6.0, sedona-1.6.0 Jan 20, 2024

iGN5117 added 2 commits January 20, 2024 12:44

Address PR changes | Move ST_DWithin to DistanceJoin

ae9f131

fix failing test

8488cad

jiayuasu requested changes Jan 22, 2024

View reviewed changes

iGN5117 added 4 commits January 25, 2024 11:52

Remove randomness from sphere test case generation

c1e85b9

Merge branch 'sedona-master' into develop_Nilesh_1.5.1

22422d7

Refactor documentation of ST_DWithin

e486724

revert resourceFolder path

5a31de2

jiayuasu requested a review from Kontinuation January 26, 2024 03:18

jiayuasu approved these changes Jan 26, 2024

View reviewed changes

Kontinuation requested changes Jan 26, 2024

View reviewed changes

.../common/src/main/scala/org/apache/spark/sql/sedona_sql/strategy/join/JoinQueryDetector.scala Outdated Show resolved Hide resolved

iGN5117 added 7 commits January 27, 2024 09:18

Handle complex boolean expressions in ST_DWithin

b569d89

add a blanket try catch for ST_DWithin to handle complex boolean expr…

a5022b2

…essions

add collect to the python test

7e6cd49

replace head() with count()

d0bfbaa

Merge branch 'sedona-master' into develop_Nilesh_1.5.1

51b3d11

Add null check for geometry column while adding a df to keplergl

5352fb9

Revert "Add null check for geometry column while adding a df to keple…

172ae05

…rgl" This reverts commit 5352fb9.

jiayuasu requested a review from Kontinuation January 29, 2024 17:12

Kontinuation approved these changes Jan 31, 2024

View reviewed changes

jiayuasu approved these changes Jan 31, 2024

View reviewed changes

jiayuasu merged commit a24a9ff into apache:master Jan 31, 2024
47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SEDONA-468] Add useSpheroid provision in ST_DWithin #1208

[SEDONA-468] Add useSpheroid provision in ST_DWithin #1208

iGN5117 commented Jan 19, 2024

jiayuasu Jan 20, 2024

iGN5117 Jan 20, 2024

jiayuasu Jan 20, 2024

iGN5117 Jan 20, 2024

jiayuasu Jan 22, 2024

jiayuasu Jan 22, 2024

jiayuasu Jan 22, 2024

jiayuasu Jan 22, 2024

jiayuasu commented Jan 26, 2024

jiayuasu commented Jan 29, 2024


		Format: `ST_DWithin (leftGeometry: Geometry, rightGeometry: Geometry, distance: Double)`
		If `useSpheroid` is passed true, ST_DWithin uses Sedona's ST_DistanceSpheroid. If `useSpheroid` is passed false or not passed at all, DWithin uses Euclidean distance.

[SEDONA-468] Add useSpheroid provision in ST_DWithin #1208

[SEDONA-468] Add useSpheroid provision in ST_DWithin #1208

Conversation

iGN5117 commented Jan 19, 2024

Did you read the Contributor Guide?

Is this PR related to a JIRA ticket?

What changes were proposed in this PR?

How was this patch tested?

Did this PR include necessary documentation updates?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiayuasu commented Jan 26, 2024

jiayuasu commented Jan 29, 2024