Skip to content
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-1] Make Sedona depend on a LocationTech JTS 1.16.1 fork #484

Merged
merged 6 commits into from
Oct 12, 2020

Conversation

jiayuasu
Copy link
Member

@jiayuasu jiayuasu commented Oct 5, 2020

Is this PR related to a proposed Issue?

#482

#435

#233

What changes were proposed in this PR?

Please see my proposal: #482 (comment)

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

@jiayuasu
Copy link
Member Author

jiayuasu commented Oct 5, 2020

@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

@jiayuasu
Copy link
Member Author

jiayuasu commented Oct 5, 2020

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

@Imbruced
Copy link
Member

Imbruced commented Oct 5, 2020

@jiayuasu I will take care of this today evening.

Copy link
Contributor

@netanel246 netanel246 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Please check the dependency comments.

core/pom.xml Show resolved Hide resolved
core/pom.xml Show resolved Hide resolved
@jiayuasu
Copy link
Member Author

@Imbruced Hi Pawel, could you please take a look at this? I tried to fix it by myself but didn't succeed

@Imbruced
Copy link
Member

@jiayuasu I was able to fix the issue on my local machine. I need to check if CI works fine.

@Imbruced Imbruced mentioned this pull request Oct 11, 2020
* Fix issue with failing python tests.

* Fix issue with failing python tests.

* Add missing dependency.

* Remove skip tests flag.

* Fix issue with failing python tests.
@jiayuasu jiayuasu merged commit 615904b into master Oct 12, 2020
@jiayuasu jiayuasu deleted the jtsplus branch October 18, 2020 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants