You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sedona.apache.org by GitBox <gi...@apache.org> on 2021/11/29 19:53:52 UTC

[GitHub] [incubator-sedona] kseebaldt opened a new pull request #565: 3d

kseebaldt opened a new pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565


   ## Is this PR related to a proposed Issue?
   
   [SEDONA-75](https://issues.apache.org/jira/browse/SEDONA-75)
   
   ## What changes were proposed in this PR?
   
   Preserve Z coordinates on geometries when serializing.
   Enhance ST_AsText to output Z coordinate if it exists.  
   Add ST_Z function to access Z coordinate
   Add ST_3DDistance to compute 3D distance between geometries
   
   ## How was this patch tested?
   
   Added unit tests.  Used in internal projects with 2D & 3D datasets
   
   ## Did this PR include necessary documentation updates?
   
   Yes
   


-- 
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] [incubator-sedona] Imbruced commented on pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
Imbruced commented on pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565#issuecomment-985092495


   @jiayuasu I am finishing code review, should be finished within an hour. 
   About 3D join, I know it will work as it is now but as you said wont be optimal, bcs we drop one dimension. It would be interesting to add 3D spatial partitioning and indexing. 


-- 
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] [incubator-sedona] jiayuasu commented on pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
jiayuasu commented on pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565#issuecomment-983539832


   @Imbruced This looks good to me. What do you think?


-- 
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] [incubator-sedona] jiayuasu commented on pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
jiayuasu commented on pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565#issuecomment-985083882


   @kseebaldt @Imbruced I just realized 3D geometries will not be considered in SpatialJoin. For example, this SQL query won't trigger a Sedona optimized join
   
   ```
   SELECT t1.geom, t2.geom
   FROM t1, t2
   WHERE ST_3dDistance(t1.geom, t2.geom)
   ```
   
   Anyway, I think it is good to accept this PR as is, and have another PR to add the support of 3D spatial join.
   


-- 
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] [incubator-sedona] Imbruced commented on pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
Imbruced commented on pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565#issuecomment-985087744


   But in case of 3D spatial join we should include tree structures like octree. That sounds interesting :)  


-- 
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] [incubator-sedona] Imbruced commented on pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
Imbruced commented on pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565#issuecomment-985107698


   @jiayuasu It looks good to me, but we should avoid showing dataframes within the tests or any unnecessary print statements. 


-- 
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] [incubator-sedona] jiayuasu edited a comment on pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
jiayuasu edited a comment on pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565#issuecomment-985083882


   @kseebaldt @Imbruced I just realized 3D geometries will not be considered in SpatialJoin. For example, this SQL query won't trigger a Sedona optimized join
   
   ```
   SELECT t1.geom, t2.geom
   FROM t1, t2
   WHERE ST_3dDistance(t1.geom, t2.geom) < 10
   ```
   
   Anyway, I think it is good to accept this PR as is, and have another PR to add the support of 3D spatial join.
   


-- 
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] [incubator-sedona] Imbruced commented on pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
Imbruced commented on pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565#issuecomment-982078673


   @kseebaldt Hi, Thank you for the PR (it's awesome). Can you also add tests for Python ? 


-- 
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] [incubator-sedona] kseebaldt commented on a change in pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
kseebaldt commented on a change in pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565#discussion_r761614410



##########
File path: python/tests/sql/test_constructor_test.py
##########
@@ -56,6 +60,24 @@ def test_st_geom_from_wkt(self):
         polygon_df.show(10)
         assert polygon_df.count() == 100
 
+    def test_st_geom_from_wkt_3d(self):
+        input_df = self.spark.createDataFrame([
+            ("Point(21 52 87)",),
+            ("Polygon((0 0 1, 0 1 1, 1 1 1, 1 0 1, 0 0 1))",),
+            ("Linestring(0 0 1, 1 1 2, 1 0 3)",),
+            ("MULTIPOINT ((10 40 66), (40 30 77), (20 20 88), (30 10 99))",),
+            ("MULTIPOLYGON (((30 20 11, 45 40 11, 10 40 11, 30 20 11)), ((15 5 11, 40 10 11, 10 20 11, 5 10 11, 15 5 11)))",),
+            ("MULTILINESTRING ((10 10 11, 20 20 11, 10 40 11), (40 40 11, 30 30 11, 40 20 11, 30 10 11))",),
+            ("MULTIPOLYGON (((40 40 11, 20 45 11, 45 30 11, 40 40 11)), ((20 35 11, 10 30 11, 10 10 11, 30 5 11, 45 20 11, 20 35 11), (30 20 11, 20 15 11, 20 25 11, 30 20 11)))",),
+            ("POLYGON((0 0 11, 0 5 11, 5 5 11, 5 0 11, 0 0 11), (1 1 11, 2 1 11, 2 2 11, 1 2 11, 1 1 11))",),
+        ], ["wkt"])
+
+        input_df.createOrReplaceTempView("input_wkt")
+        input_df.show()

Review comment:
       No problem.  I was just following the pattern of the existing 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] [incubator-sedona] kseebaldt commented on pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
kseebaldt commented on pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565#issuecomment-982087380


   Sure!  I'll work on that.


-- 
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] [incubator-sedona] jiayuasu merged pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
jiayuasu merged pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565


   


-- 
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] [incubator-sedona] Imbruced commented on pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
Imbruced commented on pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565#issuecomment-984113463


   One of our action is failing due to depricated exception we should fix that in first place :) I can do that 


-- 
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] [incubator-sedona] jiayuasu commented on pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
jiayuasu commented on pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565#issuecomment-984178945


   @Imbruced Sure. I will wait for your response tomorrow.
   
   BTW, the failed GitHub action of Example project has been fixed by me. @kseebaldt On your local branch, can you click "fetch upstream updates" on your GitHub interface? This will load my latest commit.


-- 
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] [incubator-sedona] Imbruced commented on a change in pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
Imbruced commented on a change in pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565#discussion_r761559267



##########
File path: python/tests/sql/test_constructor_test.py
##########
@@ -56,6 +60,24 @@ def test_st_geom_from_wkt(self):
         polygon_df.show(10)
         assert polygon_df.count() == 100
 
+    def test_st_geom_from_wkt_3d(self):
+        input_df = self.spark.createDataFrame([
+            ("Point(21 52 87)",),
+            ("Polygon((0 0 1, 0 1 1, 1 1 1, 1 0 1, 0 0 1))",),
+            ("Linestring(0 0 1, 1 1 2, 1 0 3)",),
+            ("MULTIPOINT ((10 40 66), (40 30 77), (20 20 88), (30 10 99))",),
+            ("MULTIPOLYGON (((30 20 11, 45 40 11, 10 40 11, 30 20 11)), ((15 5 11, 40 10 11, 10 20 11, 5 10 11, 15 5 11)))",),
+            ("MULTILINESTRING ((10 10 11, 20 20 11, 10 40 11), (40 40 11, 30 30 11, 40 20 11, 30 10 11))",),
+            ("MULTIPOLYGON (((40 40 11, 20 45 11, 45 30 11, 40 40 11)), ((20 35 11, 10 30 11, 10 10 11, 30 5 11, 45 20 11, 20 35 11), (30 20 11, 20 15 11, 20 25 11, 30 20 11)))",),
+            ("POLYGON((0 0 11, 0 5 11, 5 5 11, 5 0 11, 0 0 11), (1 1 11, 2 1 11, 2 2 11, 1 2 11, 1 1 11))",),
+        ], ["wkt"])
+
+        input_df.createOrReplaceTempView("input_wkt")
+        input_df.show()

Review comment:
       Can we avoid showing data frame during tests ? I dont know if github actions have simmilar issue to travis, but tests failed due 2 a lot of prints in console. 




-- 
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] [incubator-sedona] jiayuasu commented on pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
jiayuasu commented on pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565#issuecomment-985089647


   @Imbruced I would say it is yes and no.
   
   "No": The existing code structure and index structure will work for 3D geoms, and guarantee the correctness of the result.
   "Yes": the existing code such as spatial partitioning and indexes does not filter upon the z axis so the join performance is not optimal.
   
   BTW, will you be OK if I accept this 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] [incubator-sedona] Imbruced commented on pull request #565: [SEDONA-75] Add support for "3D" geometries

Posted by GitBox <gi...@apache.org>.
Imbruced commented on pull request #565:
URL: https://github.com/apache/incubator-sedona/pull/565#issuecomment-984112969


   To me it looks good but if you dont mind I will check this branch locally tomorrow. 


-- 
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