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

fix(duckdb): load extension when executing geospatial expressions #9080

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

ncclementi
Copy link
Contributor

@@ -347,3 +347,14 @@ def test_cast_wkb_to_geo(con):
geo_expr = t.geometry.cast("geometry")
assert geo_expr.type().is_geospatial()
assert isinstance(con.execute(geo_expr), gpd.GeoSeries)


def test_load_spatial_casting(ext_dir):
Copy link
Member

Choose a reason for hiding this comment

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

If you're going to use a custom extension directory you need to produce a new one specific to this test call.

As is, ext_dir is a session-level fixture, which means that if another test runs before this one that happens to install the geospatial extension then you're not testing the thing that needs to be tested (or its success is implicitly depending on the fact that another test ran before it, and that'll lead to a flaky test), which is that geospatial operations load the extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to be able have a new connection that doesn't have the extension loaded. I was following what we did here

@pytest.fixture(scope="session")
def ext_dir(tmp_path_factory):
# this directory is necessary because of Windows extension downloads race
# condition
return tmp_path_factory.mktemp("extensions")
def test_geo_unop_geo_literals(ext_dir, geo_line_lit):
"""GeoSpatialUnOp operation on a geospatial literal"""
con = ibis.duckdb.connect(extension_directory=ext_dir)
expr = geo_line_lit.length()
assert con.execute(expr) == 2

Which sounds like it's also being missed used here too? I can scope it to "function", though I'm not sure that's the path we want.

Copy link
Member

Choose a reason for hiding this comment

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

Which sounds like it's also being missed used here too?

Probably 😅

If you change the fixture decorator from

@pytest.fixture(scope="session")

to

@pytest.fixture

that should be sufficient (no scope="function" necessary).

@cpcloud cpcloud added this to the 9.0 milestone Apr 30, 2024
@cpcloud cpcloud added feature Features or general enhancements geospatial Geospatial related functionality duckdb The DuckDB backend bug Incorrect behavior inside of ibis and removed feature Features or general enhancements labels Apr 30, 2024
@cpcloud cpcloud changed the title fix: load extension when geospatial type fix(duckdb): load extension when executing expressions with geospatial type Apr 30, 2024
@cpcloud cpcloud changed the title fix(duckdb): load extension when executing expressions with geospatial type fix(duckdb): load extension when executing geospatial expressions Apr 30, 2024
@cpcloud cpcloud merged commit 1960d54 into ibis-project:main Apr 30, 2024
92 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior inside of ibis duckdb The DuckDB backend geospatial Geospatial related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(geo-duckdb): ibis.read_parquet() binary cast to geometry
2 participants