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

FEAT: PostGIS support #1787

Merged
merged 34 commits into from Jun 17, 2019

Conversation

3 participants
@ian-r-rose
Copy link
Contributor

commented May 16, 2019

Fixes #1786

This is in an extremely early state, but I wanted to open this for comment now as I'm not likely to get the chance to work much on it for a few days.

@@ -14,6 +14,14 @@
from sqlalchemy.ext.compiler import compiles as sa_compiles
from sqlalchemy.sql.elements import Over as _Over

try:

This comment has been minimized.

Copy link
@ian-r-rose

ian-r-rose May 16, 2019

Author Contributor

We may want to put all this geoalchemy logic in another submodule, but for now I have it inline with the rest of the sqlalchemy stuff.

Show resolved Hide resolved ibis/sql/alchemy.py Outdated
Show resolved Hide resolved ibis/sql/alchemy.py
@@ -881,7 +939,17 @@ def _fetch(self, cursor):
columns=cursor.proxy.keys(),
coerce_float=True,
)
return self.schema().apply_to(df)
df = self.schema().apply_to(df)
if GEO:

This comment has been minimized.

Copy link
@ian-r-rose

ian-r-rose May 16, 2019

Author Contributor

We may want to move this logic elsewhere (possibly even to Schema.apply_to), but for now this is convenient.

@xmnlab

This comment has been minimized.

Copy link
Collaborator

commented May 21, 2019

it is looking pretty good :)

it seems there are some pep8 issues ... if you want you can run make develop that will install git pre-commit hook. it has some useful things such as flake8, black and isort

@ian-r-rose ian-r-rose force-pushed the ian-r-rose:postgis branch from a5e0fbc to a573f8d May 21, 2019

@ian-r-rose

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Thanks for the tip @xmnlab. At this point a lot of the functionality I'm looking for is in, but it needs a lot of testing. Any tips as to how to go about that? In particular, it would be useful to have a PostGIS-enabled PostgreSQL and a Spatialite-enabled SQLite in CI.

@codecov

This comment has been minimized.

Copy link

commented May 21, 2019

Codecov Report

Merging #1787 into master will decrease coverage by 1.87%.
The diff coverage is 51.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1787      +/-   ##
==========================================
- Coverage   87.33%   85.45%   -1.88%     
==========================================
  Files          81       80       -1     
  Lines       15266    15219      -47     
  Branches     1965     1956       -9     
==========================================
- Hits        13333    13006     -327     
- Misses       1567     1860     +293     
+ Partials      366      353      -13
Impacted Files Coverage Δ
ibis/expr/rules.py 99.41% <100%> (+0.01%) ⬆️
ibis/expr/operations.py 95.85% <100%> (+0.11%) ⬆️
ibis/expr/datatypes.py 94.91% <100%> (-0.11%) ⬇️
ibis/sql/alchemy.py 89.14% <15.38%> (-3.9%) ⬇️
ibis/expr/api.py 89.53% <33.33%> (-4.27%) ⬇️
ibis/bigquery/client.py 41.1% <0%> (-53.39%) ⬇️
ibis/bigquery/compiler.py 58.67% <0%> (-38.76%) ⬇️
ibis/bigquery/udf/api.py 80.48% <0%> (-14.64%) ⬇️
ibis/bigquery/api.py 63.33% <0%> (-10.01%) ⬇️
ibis/impala/compiler.py 91.28% <0%> (-5.27%) ⬇️
... and 31 more
@xmnlab

This comment has been minimized.

Copy link
Collaborator

commented May 22, 2019

hey @ian-r-rose you can add the tests here: https://github.com/ibis-project/ibis/blob/f6220bb29ea05035ebeb44437c7db36ce0ba8562/ibis/tests/all/test_geospatial.py

related to postgis, you can try to change

image: postgres

to one docker image from https://wiki.osgeo.org/wiki/DockerImages or https://hub.docker.com/r/geographica/postgis/ (this last one I just find right now)

probably you will need to add the the new libs you are using to ci/requirements-dev yaml files

related to spatialite extension ... not sure .. maybe you need to add libspatialite to requirements-dev.yml files and change the import here:

import sqlalchemy as sa

maybe you also will need to change something here:

def init_database(driver, params, schema=None, recreate=True, **kwargs):

@cpcloud what do you think about this?

@ian-r-rose ian-r-rose force-pushed the ian-r-rose:postgis branch from 7f1a979 to df9235a May 28, 2019

@ian-r-rose ian-r-rose changed the title [WIP] PostGIS suppot [WIP] PostGIS support Jun 4, 2019

@ian-r-rose ian-r-rose force-pushed the ian-r-rose:postgis branch from bcd7c7d to ab48fd6 Jun 4, 2019

@ian-r-rose

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Okay, I think this is close-to-ready for some feedback. Tests are mostly passing except for docs (any idea what is going on there?), and most of the functionality I am looking for is in.

There are still some tests to write, and some coverage to improve, but querying geospatial data with PostGIS basically works!

@ian-r-rose ian-r-rose force-pushed the ian-r-rose:postgis branch 2 times, most recently from cb9c296 to b1d8aef Jun 5, 2019

@xmnlab

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2019

hey @ian-r-rose is this still WIP or is it done for a review?

@ian-r-rose

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

I think it will need some more test coverage (hence still having WIP), but some review would be helpful at this point.

@xmnlab

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2019

OK ... I will review it today :)

@ian-r-rose

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Thanks!

.isort.cfg Outdated
@@ -1,2 +1,2 @@
[settings]
known_third_party = asv,click,clickhouse_driver,dateutil,google,graphviz,impala,jinja2,kudu,multipledispatch,numpy,pandas,pkg_resources,plumbum,psycopg2,pyarrow,pydata_google_auth,pymapd,pymysql,pytest,pytz,regex,requests,ruamel,setuptools,sphinx_rtd_theme,sqlalchemy,toolz
known_third_party = asv,click,clickhouse_driver,dateutil,geopandas,google,graphviz,impala,jinja2,kudu,multipledispatch,numpy,pandas,pkg_resources,plumbum,psycopg2,pyarrow,pydata_google_auth,pymapd,pymysql,pytest,pytz,regex,requests,ruamel,setuptools,sphinx_rtd_theme,sqlalchemy,toolz

This comment has been minimized.

Copy link
@xmnlab

xmnlab Jun 6, 2019

Collaborator

maybe also gdal and geoalchemy2 should be added here.

This comment has been minimized.

Copy link
@ian-r-rose

ian-r-rose Jun 6, 2019

Author Contributor

This was automatically added by the pre-commit hook. Should I edit it manually?

This comment has been minimized.

Copy link
@xmnlab

xmnlab Jun 6, 2019

Collaborator

actually not sure .. @cpcloud do you know?

This comment has been minimized.

Copy link
@cpcloud

cpcloud Jun 7, 2019

Member

No, this is fine. Third-party dependencies get automatically added when they are detected.

This comment has been minimized.

Copy link
@cpcloud

cpcloud Jun 7, 2019

Member

We should only add gdal and geoalchemy2 if they are imported directly inside of ibis and they are not detected by seed-isort-config.

This comment has been minimized.

Copy link
@cpcloud

cpcloud Jun 7, 2019

Member

Yes, please do.

This comment has been minimized.

Copy link
@ian-r-rose

ian-r-rose Jun 11, 2019

Author Contributor

Hmm, seed-isort-config doesn't seem to like my manually adding imports here. It is automatically removing them in the pre-commit hook.

This comment has been minimized.

Copy link
@cpcloud

cpcloud Jun 12, 2019

Member

Ok, I think that's probably fine. Does it cause issues elsewhere? If not then I'd say leave as is.

This comment has been minimized.

Copy link
@ian-r-rose

ian-r-rose Jun 12, 2019

Author Contributor

Doesn't seem to cause any issues.

This comment has been minimized.

Copy link
@ian-r-rose

ian-r-rose Jun 12, 2019

Author Contributor

And now the file is unchanged. It seems that isort isn't smart enough to see pytest.importorskip or imports in a try/except block.

Show resolved Hide resolved ci/datamgr.py
Show resolved Hide resolved ibis/sql/alchemy.py Outdated
@xmnlab
Copy link
Collaborator

left a comment

looks awesome @ian-r-rose ! I just added small comments.

@xmnlab

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2019

mmm it seems now there is a file with conflict

Show resolved Hide resolved ci/datamgr.py
Show resolved Hide resolved ci/requirements-3.5-dev.yml Outdated
@cpcloud

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

This is looking pretty close, just a few more review comments and the merge conflict and we should be good to go.

@cpcloud cpcloud added the postgres label Jun 7, 2019

@ian-r-rose ian-r-rose force-pushed the ian-r-rose:postgis branch from 2d064b1 to 10a10fc Jun 12, 2019

@ian-r-rose

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

I've made a demonstration gist showing this PR in action: https://gist.github.com/ian-r-rose/255295a5a98ca59259b55aee9a7cfdae

It's a little hokey, but shows some non-trivial geospatial operations.

@xmnlab

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

@ian-r-rose your notebook looks great!

@cpcloud

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Agree with @xmnlab: sweet notebook.

@cpcloud

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@ian-r-rose As a follow up we might consider adding it to our tutorial notebook collection.

@ian-r-rose ian-r-rose changed the title [WIP] PostGIS support PostGIS support Jun 12, 2019

@ian-r-rose

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Okay, I think I am happy with this, feature-wise. Ideas for follow-ups:

  • Implement some missing operations
  • More/better tests
  • Tutorial notebook
  • Clean up some of the logic around loading geo data in CI
@xmnlab

xmnlab approved these changes Jun 13, 2019

Copy link
Collaborator

left a comment

LGTM! thanks @ian-r-rose ! @cpcloud do you think it is done to be merged?

@xmnlab xmnlab requested a review from cpcloud Jun 17, 2019

@cpcloud
Copy link
Member

left a comment

Bravo! LGTM, merging.

@cpcloud

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@ian-r-rose Thanks for all your work on this!

@cpcloud cpcloud merged commit 52cb979 into ibis-project:master Jun 17, 2019

13 checks passed

ci/circleci: python35_test Your tests passed on CircleCI!
Details
ci/circleci: python36_benchmark Your tests passed on CircleCI!
Details
ci/circleci: python36_conda_build Your tests passed on CircleCI!
Details
ci/circleci: python36_docs Your tests passed on CircleCI!
Details
ci/circleci: python36_test Your tests passed on CircleCI!
Details
ci/circleci: python37_conda_build Your tests passed on CircleCI!
Details
ci/circleci: python37_test Your tests passed on CircleCI!
Details
ibis-project.ibis Build #20190612.12 succeeded
Details
ibis-project.ibis (WindowsCondaBuild py36) WindowsCondaBuild py36 succeeded
Details
ibis-project.ibis (WindowsCondaBuild py37) WindowsCondaBuild py37 succeeded
Details
ibis-project.ibis (WindowsTest py35) WindowsTest py35 succeeded
Details
ibis-project.ibis (WindowsTest py36) WindowsTest py36 succeeded
Details
ibis-project.ibis (WindowsTest py37) WindowsTest py37 succeeded
Details

New Operations and Types automation moved this from To do to Done Jun 17, 2019

@cpcloud cpcloud changed the title PostGIS support FEAT: PostGIS support Jun 17, 2019

@ian-r-rose

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

Thanks for the patient review @cpcloud and @xmnlab!

@xmnlab

This comment has been minimized.

Copy link
Collaborator

commented Jun 17, 2019

thanks for you hard work @ian-r-rose these new features are awesome!

@xmnlab

This comment has been minimized.

Copy link
Collaborator

commented Jun 17, 2019

it seems also it is necessary to add smoke tests for the new ibis expressions: ibis/expr/tests/test_geospatial.py

maybe parametrize the tests would be good

I will open an issue for that.

@cpcloud

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Cool. @ian-r-rose Can you open up issues for your bullet points above if you haven't already?

@ian-r-rose ian-r-rose deleted the ian-r-rose:postgis branch Jun 17, 2019

@xmnlab

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2019

@ian-r-rose just a question, did you add translation for literals for geotypes on postgis? (ex: point_0 = ibis.literal((0, 0), type='point:geometry').name('p'))

@ian-r-rose

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

No, I don't think so. If you can point me to some examples, I can try to do that in a follow-up.

@xmnlab

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2019

awesome! this is how geo literals were implemented on mapd: https://github.com/ibis-project/ibis/blob/master/ibis/mapd/operations.py#L390

I think you can add these translations here for postgis: https://github.com/ibis-project/ibis/blob/master/ibis/sql/postgres/compiler.py#L585

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.