SUPP: Improve geospatial literals and smoke tests #1928

xmnlab · 2019-08-19T23:06:43Z

Improves geo spatial literals and smoke tests:

changes GeoMockConnection to allow tests for omniscid to postgresql https://github.com/ibis-project/ibis/blob/master/ibis/expr/tests/mocks.py#L423
fixes geo spatial literals used in https://github.com/ibis-project/ibis/blob/master/ibis/expr/tests/test_geospatial.py
improve tests in https://github.com/ibis-project/ibis/blob/master/ibis/expr/tests/test_geospatial.py
fixes black pre-commit hook configuration
allows literal without geo_types (geometry or geography) for postgis backend
fixes translation for geo spatial literals (ibis/common/geospatial.py)

Resolves #1929
Resolves #1931
Resolves partially #1930

xmnlab · 2019-08-20T14:22:36Z

@ian-r-rose it seems there are operations available for omniscidb that postgis doesn't have.
so probably the best approach here is making tests for both omniscidb and postgis.

xmnlab · 2019-08-20T16:17:35Z

@ian-r-rose it is done for review ... if you have time, some feedback would be awesome :)

xmnlab · 2019-08-20T17:19:00Z

@cpcloud it is done for review :) thanks!

cpcloud

Small change request, otherwise LGTM.

.pre-commit-config.yaml

ci/requirements-3.5-dev.yml

ibis/expr/tests/test_geospatial.py

xmnlab

@cpcloud I applied the suggestion you made. it is done for review.

ci/requirements-3.5-dev.yml

ibis/expr/tests/test_geospatial.py

.pre-commit-config.yaml

codecov · 2019-08-21T00:41:07Z

Codecov Report

Merging #1928 into master will decrease coverage by 1.62%.
The diff coverage is 63.88%.

@@            Coverage Diff             @@
##           master    #1928      +/-   ##
==========================================
- Coverage   87.57%   85.94%   -1.63%     
==========================================
  Files          93       93              
  Lines       16814    16837      +23     
  Branches     2138     2144       +6     
==========================================
- Hits        14725    14471     -254     
- Misses       1683     1959     +276     
- Partials      406      407       +1

Impacted Files	Coverage Δ
ibis/common/geospatial.py	`81.39% <62.5%> (+10.62%)`	⬆️
ibis/sql/alchemy.py	`92.52% <75%> (+0.28%)`	⬆️
ibis/bigquery/client.py	`41.1% <0%> (-53.39%)`	⬇️
ibis/bigquery/compiler.py	`59.92% <0%> (-37.5%)`	⬇️
ibis/bigquery/udf/api.py	`80.48% <0%> (-14.64%)`	⬇️
ibis/impala/compiler.py	`91.23% <0%> (-5.2%)`	⬇️
ibis/pandas/dispatch.py	`95.45% <0%> (-4.55%)`	⬇️
ibis/pandas/client.py	`85.54% <0%> (-3.47%)`	⬇️
ibis/bigquery/api.py	`63.33% <0%> (-3.34%)`	⬇️
ibis/expr/schema.py	`89.77% <0%> (-1.14%)`	⬇️
... and 3 more

ibis/expr/tests/conftest.py

cpcloud · 2019-08-22T13:03:59Z

@xmnlab I've temporarily pinned sqlalchemy in #1938. Can you remove the corresponding changes here? I'll let you know when I merge that so that you can rebase and get some passing builds.

cpcloud · 2019-08-22T13:37:18Z

@xmnlab Can you rebase? I merged #1938.

xmnlab

rebased!

ibis/expr/tests/conftest.py

xmnlab · 2019-08-22T15:18:08Z

@cpcloud looks like the CI tests passed! :)

ibis/common/geospatial.py

cpcloud · 2019-08-22T18:48:23Z

ibis/common/geospatial.py

    """Convert a iterable with a linestring to text."""
-    return ', '.join(_format_point_value(point) for point in value)
+    template = '({})' if nested else '{}'
+    # fix wrong structure


Can you elaborate here? What exactly is wrong is about the structure if the first element is a tuple or list?

for example this WKT POLYGON ((30 10, 40 40, 20 40, 10 20, 30 10))

if anyone tries that in python (((30, 10), (40, 40), (20, 40), (10, 20), (30, 10))) the outer parenthesis doesn't have any effect. the "correct" way to write that would be (((30, 10), (40, 40), (20, 40), (10, 20), (30, 10)),) (ensuring the tuple format on the outer parenthesis)

so this code is used to ensure the "correct" format of this data.

I don't think we should try to mirror the syntax of postgres/postgis types here with a Python equivalent. A sequence of pairs of numbers is the contract.

but it should be a tuple, another example shows that:

POLYGON ((35 10, 45 45, 15 40, 10 20, 35 10), (20 30, 35 35, 30 20, 20 30))

I don't think we should be second guessing user input types. Can you write down the type of the input expected?

anymore thoughts about that? should I change anything? or is it already OK?

ah and if I should raise an error .. what would be the best Exception to be used? IbisInputError?

Yeah, I think that makes sense.

sounds good. I will change that! thanks!

done! let me know your thoughts about the message and the type hint.

ibis/common/geospatial.py

ibis/expr/tests/conftest.py

xmnlab · 2019-08-22T20:31:54Z

Can you import the one from tests/all here?

the current structure doesn't work with tests.all.conftest.pytest_pyfunc_call because I am not using an instance of Backend

trying to use 'Backend' it seems it is raising an error related to circular imports

ImportError while loading conftest '/mnt/sda1/dev/quansight/ibis-project/ibis/ibis/expr/tests/conftest.py'.
conftest.py:20: in <module>
    from ibis.expr.tests.mocks import (
mocks.py:23: in <module>
    from ibis.tests.backends import Backend
../../tests/backends.py:19: in <module>
    from ibis.impala.tests.conftest import IbisTestEnv as ImpalaEnv
../../impala/tests/conftest.py:10: in <module>
    from ibis.expr.tests.mocks import MockConnection
E   ImportError: cannot import name 'MockConnection' from 'ibis.expr.tests.mocks' (/mnt/sda1/dev/quansight/ibis-project/ibis/ibis/expr/tests/mocks.py)

the pytest_pyfunc_call I wrote is specifically for mock

cpcloud · 2019-08-22T20:55:51Z

Ah, got it. Thanks for clarifying.

In this PR: - Fixes explain operation Resolves #1921 Depends on #1928 Author: Ivan Ogasawara <ivan.ogasawara@gmail.com> Closes #1933 from xmnlab/fix-explain and squashes the following commits: 6ed5e5b [Ivan Ogasawara] refactoring 980fd4d [Ivan Ogasawara] Fixed explain method

ibis/expr/tests/test_geospatial.py

ibis/common/geospatial.py

ibis/expr/tests/test_geospatial.py

cpcloud · 2019-08-24T12:39:40Z

ibis/expr/tests/test_geospatial.py

+def test_geo_ops_smoke(backend, fn_expr):
+    """Smoke tests for geo spatial operations."""
+    geo_table = backend.table('geo')
+    fn_expr(geo_table).compile()


Let's at least assert that the compiled result is not the empty string.

sounds good!

ibis/common/geospatial.py

cpcloud · 2019-08-26T13:17:12Z

@xmnlab Can you resolve the merge conflicts here?

xmnlab · 2019-08-26T16:30:06Z

@cpcloud, rebased! :)

xmnlab · 2019-08-26T18:17:18Z

it is done for a new review.

cpcloud

LGTM. Thanks @xmnlab!

xmnlab · 2019-08-26T18:24:16Z

thanks @cpcloud!

xmnlab changed the title ~~Fix smoke test for geospatial operations~~ SUPP: Improve smoke test for geospatial operations Aug 19, 2019

xmnlab self-assigned this Aug 19, 2019

xmnlab added geospatial refactor Issues or PRs related to refactoring the codebase tests Issues or PRs related to tests bug Incorrect behavior inside of ibis labels Aug 19, 2019

xmnlab mentioned this pull request Aug 19, 2019

PostGIS enhancements #1925

Merged

xmnlab changed the title ~~SUPP: Improve smoke test for geospatial operations~~ SUPP: Improve geospatial literals and smoke tests Aug 19, 2019

xmnlab requested a review from cpcloud August 20, 2019 16:15

cpcloud requested changes Aug 20, 2019

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

ci/requirements-3.5-dev.yml Outdated Show resolved Hide resolved

ibis/expr/tests/test_geospatial.py Outdated Show resolved Hide resolved

xmnlab mentioned this pull request Aug 20, 2019

BUG: Fixes explain operation #1933

Merged

xmnlab commented Aug 21, 2019

View reviewed changes

ci/requirements-3.5-dev.yml Outdated Show resolved Hide resolved

ibis/expr/tests/test_geospatial.py Outdated Show resolved Hide resolved

.pre-commit-config.yaml Outdated Show resolved Hide resolved

.pre-commit-config.yaml Show resolved Hide resolved

xmnlab requested a review from cpcloud August 21, 2019 01:09

cpcloud reviewed Aug 22, 2019

View reviewed changes

ibis/expr/tests/conftest.py Show resolved Hide resolved

xmnlab force-pushed the change-geo-mock-test-backend branch from 3a65449 to f1ab6ba Compare August 22, 2019 14:35

xmnlab commented Aug 22, 2019

View reviewed changes

ibis/expr/tests/conftest.py Show resolved Hide resolved

cpcloud requested changes Aug 22, 2019

View reviewed changes

xmnlab force-pushed the change-geo-mock-test-backend branch from f1ab6ba to 53eb12c Compare August 22, 2019 20:48

cpcloud reviewed Aug 23, 2019

View reviewed changes

ibis/expr/tests/test_geospatial.py Show resolved Hide resolved

cpcloud reviewed Aug 23, 2019

View reviewed changes

ibis/expr/tests/test_geospatial.py Show resolved Hide resolved

cpcloud reviewed Aug 23, 2019

View reviewed changes

ibis/common/geospatial.py Outdated Show resolved Hide resolved

xmnlab force-pushed the change-geo-mock-test-backend branch from 53eb12c to 33173b9 Compare August 23, 2019 21:37

xmnlab requested a review from cpcloud August 23, 2019 22:06

cpcloud requested changes Aug 24, 2019

View reviewed changes

cpcloud added this to the Next Major Release milestone Aug 26, 2019

Fixed smoke test for geospatial operations

7acb69f

xmnlab force-pushed the change-geo-mock-test-backend branch from 33173b9 to 7acb69f Compare August 26, 2019 16:27

xmnlab requested a review from cpcloud August 26, 2019 17:05

Added an assert for test_geo_ops_smoke

9aac60c

cpcloud approved these changes Aug 26, 2019

View reviewed changes

cpcloud merged commit 7c032e0 into ibis-project:master Aug 26, 2019

xmnlab deleted the change-geo-mock-test-backend branch August 26, 2019 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SUPP: Improve geospatial literals and smoke tests #1928

SUPP: Improve geospatial literals and smoke tests #1928

xmnlab commented Aug 19, 2019 •

edited

Loading

xmnlab commented Aug 20, 2019

xmnlab commented Aug 20, 2019

xmnlab commented Aug 20, 2019

cpcloud left a comment

xmnlab left a comment

codecov bot commented Aug 21, 2019 •

edited

Loading

cpcloud commented Aug 22, 2019

cpcloud commented Aug 22, 2019

xmnlab left a comment

xmnlab commented Aug 22, 2019

cpcloud Aug 22, 2019

xmnlab Aug 22, 2019

cpcloud Aug 22, 2019

xmnlab Aug 22, 2019

cpcloud Aug 22, 2019

xmnlab Aug 22, 2019

xmnlab Aug 22, 2019

cpcloud Aug 23, 2019

xmnlab Aug 23, 2019

xmnlab Aug 23, 2019

xmnlab commented Aug 22, 2019

cpcloud commented Aug 22, 2019

cpcloud Aug 24, 2019

xmnlab Aug 26, 2019

cpcloud commented Aug 26, 2019

xmnlab commented Aug 26, 2019

xmnlab commented Aug 26, 2019

cpcloud left a comment

xmnlab commented Aug 26, 2019

SUPP: Improve geospatial literals and smoke tests #1928

SUPP: Improve geospatial literals and smoke tests #1928

Conversation

xmnlab commented Aug 19, 2019 • edited Loading

xmnlab commented Aug 20, 2019

xmnlab commented Aug 20, 2019

xmnlab commented Aug 20, 2019

cpcloud left a comment

Choose a reason for hiding this comment

xmnlab left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 21, 2019 • edited Loading

Codecov Report

cpcloud commented Aug 22, 2019

cpcloud commented Aug 22, 2019

xmnlab left a comment

Choose a reason for hiding this comment

xmnlab commented Aug 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmnlab commented Aug 22, 2019

cpcloud commented Aug 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcloud commented Aug 26, 2019

xmnlab commented Aug 26, 2019

xmnlab commented Aug 26, 2019

cpcloud left a comment

Choose a reason for hiding this comment

xmnlab commented Aug 26, 2019

xmnlab commented Aug 19, 2019 •

edited

Loading

codecov bot commented Aug 21, 2019 •

edited

Loading