Skip to content

Conversation

@NickCrews
Copy link
Contributor

@NickCrews NickCrews commented Sep 2, 2025

Issues closed

Fixes #11585

Description of changes

  • Refactor some of the postgres-specific tests to a different file, so you can run them all at once. Also reuses the TestConf setup from the postgres tests instead of re-inventing the wheel here. This also gives us access to the "geo" table which is loaded into the postgres database for the test.
  • Add support for GeospatialValue.to_binary() for duckdb. No direct test added, though this is tested indirectly by all of the geospatial tests.
  • When materializing geospatial expressions, we always materialize them as WKB format. Before, we took care of this at the compiler level. Now, we take care of this at the backend level. This change is what allows us to read from postgres. Before, the compiler would insert bogus AsWKB() casts as described here whenever you tried to create a table from a select. eg you would get

CREATE TABLE t as (SELECT asWKB(<col that is already binary>) as c from pg_table)

With the casting pushed up to the backend level, now this bogus cast is removed.

I think this is the right abstraction? The other, maybe better way, would be to somehow detect if the expression is already a binary, and skip the asWKB conversion if so. But that seems like that might require runtime introspection of the DB, which I was trying to avoid.

TODOs

Deal with the failing tests. EDIT: actually, these look to be passing in CI, I think it is some mismatched dependencies in my local environment that are the issue?? I think there is actually nothing to do here.
There are two of them, both that look to be about rounding errors, eg

E FAILED ibis/backends/duckdb/tests/test_geospatial.py::test_geospatial_buffer_point - AssertionError: 248 out of 263 geometries are not equal.
E           The first not equal geometry:
E           Left: POINT (935996.8210162063 191376.74953083202)
E           Right: POINT (935996.8210162065 191376.74953083202)

I would say just make these tests less fussy? Not exactly sure if this is actually indicative of a problem.

@github-actions github-actions bot added tests Issues or PRs related to tests duckdb The DuckDB backend sql Backends that generate SQL labels Sep 2, 2025
params: Mapping[ir.Expr, Any] | None = None,
pretty: bool = False,
) -> str:
if isinstance(expr, ir.Table):
Copy link
Member

Choose a reason for hiding this comment

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

Won't this fail if expr is geospatial and not a table?

@cpcloud
Copy link
Member

cpcloud commented Oct 10, 2025

The problem with this approach is that now ibis.to_sql and ibis.duckdb.compile produce different SQL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duckdb The DuckDB backend sql Backends that generate SQL tests Issues or PRs related to tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(duckdb): Reading WKB_BLOB column from postgres connection incorrectly interpreted as ibis.dt.Binary()

2 participants