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.

Sign up for GitHub

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

Jump to bottom

PostGIS enhancements #1925

Merged
merged 5 commits into from
Aug 25, 2019
Merged

PostGIS enhancements #1925

merged 5 commits into from
Aug 25, 2019

Conversation

ian-r-rose
Copy link
Contributor

  1. Allows for null geometries (it previously failed in unhelpful ways upon encountering those)
  2. Add types for some missing geometry types, namely multilinestrings and multipoints.

I still need to add some tests, but wanted to put this out for comment.

@cpcloud cpcloud added this to the Next Major Release milestone Aug 15, 2019
@cpcloud cpcloud added feature Features or general enhancements postgis postgres The PostgreSQL backend labels Aug 15, 2019
@ian-r-rose ian-r-rose changed the title [WIP] PostGIS enhancements PostGIS enhancements Aug 16, 2019
@ian-r-rose
Copy link
Contributor Author

@cpcloud I've added a few tests exercising the ibis APIs, but I have not done any testing against the actual PostGIS backend, which will likely take more time, and will involve changes to the test data. If that's acceptable to you for the time-being, this should be good from my perspective. Otherwise I can add more tests next week.

The CI failures seem to be unrelated, and afflicting master as well.

xmnlab
xmnlab reviewed Aug 19, 2019
View reviewed changes
Copy link
Contributor

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

@ian-r-rose looks pretty good.
I just added a comment.
I will take a look into the mock tests you commented

ibis/expr/datatypes.py Outdated Show resolved Hide resolved
@xmnlab
Copy link
Contributor

xmnlab commented Aug 19, 2019

@ian-r-rose related to mock tests (https://github.com/ibis-project/ibis/blob/master/ibis/expr/tests/mocks.py#L423), I think it makes more sense now changing the compiler and dialect from omniscidb to postgres. what do you think?

@ian-r-rose
Copy link
Contributor Author

@xmnlab I think that makes sense -- it might take me a day or two to get to it.

@xmnlab
Copy link
Contributor

xmnlab commented Aug 19, 2019

@ian-r-rose, great! I can work on that, I will open a PR for that and you can pick the commit from there.

@xmnlab
Copy link
Contributor

xmnlab commented Aug 19, 2019

@ian-r-rose related to smoke test I am working on this PR #1928

@cpcloud
Copy link
Member

cpcloud commented Aug 22, 2019

@ian-r-rose Can you rebase and force push to pick up some changes that should make your builds pass?

@ian-r-rose
Copy link
Contributor Author

Rebased

@ian-r-rose
Copy link
Contributor Author

In case there's any interest, I put together a binder with a local PostGIS server running to demo some of this work:
Binder

cpcloud
cpcloud approved these changes Aug 25, 2019
View reviewed changes
Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @ian-r-rose!

@cpcloud cpcloud merged commit 2aaa4a0 into ibis-project:master Aug 25, 2019
@ian-r-rose
Copy link
Contributor Author

Thanks for the review @cpcloud, and enjoy your break!

And thanks for the help with testing @xmnlab.

@xmnlab
Copy link
Contributor

xmnlab commented Aug 26, 2019

thanks @ian-r-rose for working on that! :)

This was referenced Oct 2, 2019
Merged
Merged
xmnlab pushed a commit that referenced this pull request Oct 16, 2019
@ian-r-rose @xmnlab
FEAT: Add more POSTGIS operations
46d18af 
Follow up to #1787 and #1925 . This adds a number of new operations
for the PostGIS backend:    * `ST_GeometryType`  * `ST_GeometryN`  *
`ST_IsValid`  * `ST_LineLocatePoint`  * `ST_LineMerge`  *
`ST_LineSubstring`  * `ST_OrderingEquals`  * `ST_Union`    The last
one requires special attention. That
[`ST_Union`](http://postgis.net/docs/ST_Union.html) is available as
both a value operation and a column aggregation operation. To avoid a
name collision in the ibis APIs, I have named the aggregation variant
`unary_union`. This is following the same choice made by [GeoPandas](h
ttp://geopandas.org/reference.html#geopandas.GeoSeries.unary_union).
Author: Ian Rose <ian.r.rose@gmail.com>

Closes #1987 from ian-r-rose/more-postgis-ops and squashes the following commits:

aa3de1d [Ian Rose] Work on docstring styles.
379156f [Ian Rose] Export new ops as part of the top-level expr api.
dfba5de [Ian Rose] Add ST_Union functionality.
02ba505 [Ian Rose] Add some tests for the new functions.
259a477 [Ian Rose] Work on some additional geospatial operations and their implementation in postgis.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xmnlab xmnlab xmnlab left review comments

@cpcloud cpcloud cpcloud approved these changes

Assignees
No one assigned
Labels
feature Features or general enhancements postgres The PostgreSQL backend
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ian-r-rose @xmnlab @cpcloud