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 2020/10/05 06:25:02 UTC
[GitHub] [incubator-sedona] jiayuasu opened a new pull request #484: [SEDONA-1] Make Sedona depend on a LocationTech JTS 1.16.1 fork
jiayuasu opened a new pull request #484:
URL: https://github.com/apache/incubator-sedona/pull/484
## Is this PR related to a proposed Issue?
#482
#435
#233
## What changes were proposed in this PR?
Please see my proposal: https://github.com/apache/incubator-sedona/pull/482#issuecomment-700532690
Now Sedona is depending on LocationTech JTS 1.16.1 https://github.com/jiayuasu/jts The master branch of this repo also supports 1.17.x but because GeoTools and Wololo GeoJson converter work best on JTS 1.16, I only update Sedona to JTS 1.16.1
In the future, it is very easy to update Sedona to JTS 1.17 or other version.
## How was this patch tested?
Passed all unit tests in Core, SQL, Viz. Only Scala and Java APIs were tested. Python module is not tested yet.
## Did this PR include necessary documentation updates?
N/A
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] netanel246 commented on a change in pull request #484: [SEDONA-1] Make Sedona depend on a LocationTech JTS 1.16.1 fork
Posted by GitBox <gi...@apache.org>.
netanel246 commented on a change in pull request #484:
URL: https://github.com/apache/incubator-sedona/pull/484#discussion_r500749898
##########
File path: core/pom.xml
##########
@@ -32,10 +32,16 @@
<packaging>jar</packaging>
<dependencies>
+ <dependency>
+ <groupId>org.locationtech.jts</groupId>
+ <artifactId>jts-core</artifactId>
+ <version>1.16.2-SNAPSHOT</version>
Review comment:
Alright, Thanks :+1:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] jiayuasu commented on pull request #484: [SEDONA-1] Make Sedona depend on a LocationTech JTS 1.16.1 fork
Posted by GitBox <gi...@apache.org>.
jiayuasu commented on pull request #484:
URL: https://github.com/apache/incubator-sedona/pull/484#issuecomment-703430710
@Imbruced Hi Pawel, although I think there are no actual technical changes to Sedona itself, it would be good if you can take a look at the Python part. Most changes are placed in https://github.com/jiayuasu/jts.
@netanel246 @Imbruced If there are no outstanding issues after you review it, I will merge this PR. Then we can move on to other stuff (1) in the code, change all "GeoSpark" to "Sedona" (2) support Spark 3.0 Python API
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] jiayuasu commented on a change in pull request #484: [SEDONA-1] Make Sedona depend on a LocationTech JTS 1.16.1 fork
Posted by GitBox <gi...@apache.org>.
jiayuasu commented on a change in pull request #484:
URL: https://github.com/apache/incubator-sedona/pull/484#discussion_r500018075
##########
File path: core/pom.xml
##########
@@ -35,7 +35,7 @@
<dependency>
<groupId>org.wololo</groupId>
<artifactId>jts2geojson</artifactId>
- <version>0.10.0</version>
+ <version>0.13.0</version>
Review comment:
@netanel246 It looks like 0.14.3 was compiled by Java 11 and other components in Sedona was compiled by Java 8. So I didn't use 0.14.3.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] jiayuasu commented on pull request #484: [SEDONA-1] Make Sedona depend on a LocationTech JTS 1.16.1 fork
Posted by GitBox <gi...@apache.org>.
jiayuasu commented on pull request #484:
URL: https://github.com/apache/incubator-sedona/pull/484#issuecomment-703437703
@Imbruced Apparently, the Python test has failed because of the path issue. But I believe it should be a quick fix. Can you fix it and push to the Sedona "jtsplus" branch?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] jiayuasu edited a comment on pull request #484: [SEDONA-1] Make Sedona depend on a LocationTech JTS 1.16.1 fork
Posted by GitBox <gi...@apache.org>.
jiayuasu edited a comment on pull request #484:
URL: https://github.com/apache/incubator-sedona/pull/484#issuecomment-703437703
@Imbruced Apparently, the Python test has failed (https://travis-ci.org/github/apache/incubator-sedona/builds/732868873) because of the path issue. But I believe it should be a quick fix. Can you fix it and push to the Sedona "jtsplus" branch?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] Imbruced commented on pull request #484: [SEDONA-1] Make Sedona depend on a LocationTech JTS 1.16.1 fork
Posted by GitBox <gi...@apache.org>.
Imbruced commented on pull request #484:
URL: https://github.com/apache/incubator-sedona/pull/484#issuecomment-706704336
@jiayuasu I was able to fix the issue on my local machine. I need to check if CI works fine.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] Imbruced commented on pull request #484: [SEDONA-1] Make Sedona depend on a LocationTech JTS 1.16.1 fork
Posted by GitBox <gi...@apache.org>.
Imbruced commented on pull request #484:
URL: https://github.com/apache/incubator-sedona/pull/484#issuecomment-703504794
@jiayuasu I will take care of this today evening.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] jiayuasu merged pull request #484: [SEDONA-1] Make Sedona depend on a LocationTech JTS 1.16.1 fork
Posted by GitBox <gi...@apache.org>.
jiayuasu merged pull request #484:
URL: https://github.com/apache/incubator-sedona/pull/484
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] netanel246 commented on a change in pull request #484: [SEDONA-1] Make Sedona depend on a LocationTech JTS 1.16.1 fork
Posted by GitBox <gi...@apache.org>.
netanel246 commented on a change in pull request #484:
URL: https://github.com/apache/incubator-sedona/pull/484#discussion_r499519523
##########
File path: core/pom.xml
##########
@@ -32,10 +32,16 @@
<packaging>jar</packaging>
<dependencies>
+ <dependency>
+ <groupId>org.locationtech.jts</groupId>
+ <artifactId>jts-core</artifactId>
+ <version>1.16.2-SNAPSHOT</version>
Review comment:
There is a special reason why you're using the SNAPSHOT version?
Why we can't use deployable version 1.16.1?
##########
File path: core/pom.xml
##########
@@ -35,7 +35,7 @@
<dependency>
<groupId>org.wololo</groupId>
<artifactId>jts2geojson</artifactId>
- <version>0.10.0</version>
+ <version>0.13.0</version>
Review comment:
Why not using [0.14.3](https://github.com/bjornharrtell/jts2geojson/blob/0.14.3/pom.xml) (newest version)?.
It's also compatible with JTS version 1.16.1
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] jiayuasu commented on a change in pull request #484: [SEDONA-1] Make Sedona depend on a LocationTech JTS 1.16.1 fork
Posted by GitBox <gi...@apache.org>.
jiayuasu commented on a change in pull request #484:
URL: https://github.com/apache/incubator-sedona/pull/484#discussion_r500019542
##########
File path: core/pom.xml
##########
@@ -32,10 +32,16 @@
<packaging>jar</packaging>
<dependencies>
+ <dependency>
+ <groupId>org.locationtech.jts</groupId>
+ <artifactId>jts-core</artifactId>
+ <version>1.16.2-SNAPSHOT</version>
Review comment:
@netanel246 Note that, this dependency is actually the JTS submodule of the git repo, rather than a standard JTS Maven release. It was compiled by Sedona "mvn install" and automatically added to Sedona, locally. The "SNAPSHOT" comes from here: https://github.com/jiayuasu/jts/blob/1.16.x/modules/core/pom.xml
The git submodule is Sedona Maven submodule: https://github.com/apache/incubator-sedona/blob/jtsplus/pom.xml#L12
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] jiayuasu commented on pull request #484: [SEDONA-1] Make Sedona depend on a LocationTech JTS 1.16.1 fork
Posted by GitBox <gi...@apache.org>.
jiayuasu commented on pull request #484:
URL: https://github.com/apache/incubator-sedona/pull/484#issuecomment-706640944
@Imbruced Hi Pawel, could you please take a look at this? I tried to fix it by myself but didn't succeed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org