Refactor: Share spatial bind conversion helpers across dialects - #614
Conversation
5448675 to
f8b04e3
Compare
There was a problem hiding this comment.
Pull request overview
This PR centralizes WKB/EWKB bind-conversion (bytes/hex handling, SRID extraction/validation, and constructor detection) into shared helpers and then reuses those helpers across multiple dialect implementations, aiming for consistent behavior and fewer per-dialect conversion divergences.
Changes:
- Add shared bind conversion utilities in
geoalchemy2.types.dialects.commonand apply them to PostgreSQL, SQLite/GeoPackage, MySQL/MariaDB, and MSSQL bind processors. - Update dialect-specific SQL compilation paths (notably for
ST_GeomFrom(E)WKBon SQLite/GeoPackage/PostgreSQL and callable bindparam sharing on MSSQL) to align with the new conversion rules. - Expand/adjust test coverage for SRID edge-cases (notably SRID=0/unknown), constructor fast-paths, and dynamic bindparam behavior; add a new WKTElement conversion benchmark tool.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/benchmark_wkt_element_fast_paths.py | Adds a benchmark script to compare WKTElement conversion fast-paths vs converter-backed paths. |
| tests/test_types.py | Adds unit tests for new shared helpers (SRID validation, constructor detection, WKB/EWKB conversion helpers) across dialects. |
| tests/test_types_mssql.py | Extends MSSQL tests for zero-SRID EWKB handling, converter behavior, and callable bindparam reuse. |
| tests/test_elements.py | Adjusts expectations around zero-SRID EWKB auto-detection behavior. |
| tests/gallery/test_orm_mapped_v2.py | Makes ORM gallery test robust to non-deterministic inserted IDs. |
| tests/benchmarks/test_insert_select.py | Refactors benchmark setup/insert logic and updates representation labels/expected outcomes. |
| TEST.rst | Documents the wkb-wkt-converter dependency as part of manual requirements. |
| geoalchemy2/types/dialects/common.py | Introduces shared WKB/EWKB conversion + SRID validation/constructor-detection helpers. |
| geoalchemy2/types/dialects/sqlite.py | Replaces Shapely-based WKB conversions with shared helpers + converter-backed WKT conversion. |
| geoalchemy2/types/dialects/geopackage.py | Adds GeoPackage-specific WKB/EWKB constructor handling using shared helpers, otherwise defers to SQLite behavior. |
| geoalchemy2/types/dialects/postgresql.py | Uses shared helpers for WKB/EWKB constructors and adds support for raw bytes/hex bindvalues. |
| geoalchemy2/types/dialects/mysql.py | Uses shared helpers for WKB constructor paths and converter-backed WKB→WKT conversion for non-WKB constructors. |
| geoalchemy2/types/dialects/mariadb.py | Uses shared helpers for WKB constructor paths and preserves MariaDB MULTIPOINT WKT normalization behavior. |
| geoalchemy2/types/dialects/mssql.py | Switches MSSQL WKB parsing to converter-backed conversion + shared SRID validation semantics. |
| geoalchemy2/admin/dialects/sqlite.py | Adds EWKB-to-hex bind coercion for GeomFromEWKB (incl. literal binds) and reuses unwrap helpers. |
| geoalchemy2/admin/dialects/geopackage.py | Reuses SQLite compiler helper; adds EWKB coercion rules to avoid double-processing with Geometry bind processors. |
| geoalchemy2/admin/dialects/postgresql.py | Adds a bind type to ensure EWKB bytes are supplied for ST_GeomFromEWKB where needed; unwraps nested constructor clauses for literals. |
| geoalchemy2/admin/dialects/mssql.py | Refactors EWKT/EWKB bind coercion for callable bindparams to share callable results across dynamic/fixed SRID consumers. |
| geoalchemy2/elements.py | Tweaks WKBElement extended auto-detection (notably around SRID=0 behavior) and adjusts docstrings. |
| geoalchemy2/types/init.py | Improves AttributeError message to use type name (avoids noisy type repr). |
| geoalchemy2/_wkb_wkt.py | Simplifies SRID/header handling by relying on converter primitives and centralizing SRID sentinel policy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f97d704e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
… from bind key helper
129f1e6 to
e241c26
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Centralize WKB/EWKB bind conversion logic and apply it across dialects.
This builds on #613 by moving shared WKB/EWKB byte, hex, SRID validation, and constructor-detection behavior into common type helpers, then using those helpers in PostgreSQL, SQLite, GeoPackage, MySQL, MariaDB, and MSSQL paths.