Fix: Add MariaDB-specific reflection

geoalchemy / geoalchemy2

Geospatial extension to SQLAlchemy

http://geoalchemy-2.readthedocs.org

MIT License

637 stars 112 forks source link

Fix: Add MariaDB-specific reflection #522

Closed tomkcook closed 2 weeks ago

tomkcook commented 3 months ago

Description

Fixes: #521

This resolves #521 -- at least for me -- by adding a MariaDB-specific query for geometry column reflection.

Apologies that I am out of time today to add a test case. I'll look into it tomorrow.

Checklist

This pull request is:

[ ] A documentation / typographical error fix
- Good to go, no issue or tests are needed
[x] A short code fix
- [x] Please include: Fixes: #<issue number> in the description if it solves an existing issue (which must include a complete example of the issue).
- [ ] Please include tests that fail with the main branch and pass with the provided fix.
[ ] A new feature implementation or update an existing feature
- [ ] Please include: Fixes: #<issue number> in the description if it solves an existing issue (which must include a complete example of the feature).
- [ ] Please include tests that cover every lines of the new/updated feature.
- [ ] Please update the documentation to describe the new/updated feature.

tomkcook commented 2 months ago

TBH I struggle a bit to articulate a test that would fail without this change and pass with it, at least within the current test regime. I can see two options:

Set up a separate test_functional_mariadb.py that replicates most of test_functional_mysql.py but with a MariaDB database instead of a MySQL one.
Write a test that exercises reflect_geometry_column() specifically, mock out the engine and check that the executed SQL includes the SRS_ID column if the dialect is MySQL but not if it's MariaDB.

Do the maintainers have a preference for which approach to take?

adrien-berchet commented 2 months ago

Hi @tomkcook, Sorry for the delay, I'm currently on vacation. Thank you very much for pointing this issue out and for proposing a solution. As far as I can see, your solution looks good but it could be reworked to be a bit more compact and to avoid duplicated code. And for your question, I think that just one test for reflect_geometry_column is enough. I will have some time next week to review this more carefully.

adrien-berchet commented 4 weeks ago

Hi @tomkcook ! I made a few tests in https://github.com/geoalchemy/geoalchemy2/pull/524 and I realized that MariaDB was not as close to MySQL as I expected, so I think it should be processed as a complete different dialect. Nevertheless, there is something I don't understand: I could not find a way to insert a geometry using a WKB string, is it possible? Otherwise we will have to convert all WKB strings into WKT strings, which is quite bad for perf.

adrien-berchet commented 3 weeks ago

Hi @tomkcook ! I think #524 solved most of the issues with MariaDB, could you test it in your workflow to ensure everything is all right please?

adrien-berchet commented 2 weeks ago

This PR was included in #524 so it can be closed now. Thanks again for your work @tomkcook !