fix(sqlalchemy): multiply DWITHIN/BEYOND distance into metres - #176
Open
C1-BA-B1-F3 wants to merge 1 commit into
Open
C1-BA-B1-F3 wants to merge 1 commit into
C1-BA-B1-F3 wants to merge 1 commit into
Conversation
`spatial()` divided the CQL2 distance by 1000 for `kilometers` and by 1609 for `miles`, so `DWITHIN(geometry, POINT(0 0), 5, kilometers)` reached `ST_DWithin` as 0.005 instead of 5000. The divisors were also keyed on the wrong spellings: the ECQL grammar emits `statute miles`, not `miles`, and it also emits `feet` and `nautical miles`, neither of which was converted. Replace the dead `UNITS_LOOKUP` with a single metres-per-unit table keyed by the grammar's spellings (plus a bare `miles` for direct callers), multiply instead of divide, and leave an unknown unit untouched as before. The radius is still in the coordinate units of the column's CRS, so the conversion is exact only for a projected/metric CRS or a `geography` cast; that caveat is documented on the table. Adds `tests/backends/sqlalchemy/test_spatial_units.py`, which needs neither a database nor a spatial extension: `test_evaluate.py` is skipped wholesale without `mod_spatialite` and comments out its `DWITHIN` cases because spatialite has no `ST_DWithin`. Fixes geopython#164
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #164.
pygeofilter/backends/sqlalchemy/filters.pydivided the distance forkilometers/milesinstead of multiplying, so a 5 km radius reachedST_DWithinas0.005.Beyond the direction, the divisors were keyed on spellings the parsers do not
produce: the ECQL grammar emits
statute miles(nevermiles), and it alsoemits
feetandnautical miles, neither of which was converted at all. Theconversion is now one table keyed by what the grammar accepts:
meterskilometersfeetmiles/statute milesnautical milesAn unrecognized unit still passes through unchanged, so nothing changes beyond
the scaling that was wrong.
Note from the issue, kept as a docstring caveat rather than "fixed" here: the
radius is in the coordinate units of the column's CRS, so the conversion is
exact only for a projected, metric CRS (or a
geographycast). On ageographic column (EPSG:4326) the radius is in degrees, and pygeofilter cannot
know the units of an arbitrary column.
Tests
New
tests/backends/sqlalchemy/test_spatial_units.py, 14 cases, needingneither a database nor a spatial extension — the existing
test_evaluate.pyis skipped wholesale without
mod_spatialiteand itsDWITHIN/BEYONDcasesare commented out because spatialite has no
ST_DWithin. It covers theper-unit factors,
DWITHINandBEYOND, an unknown unit passing through, andan end-to-end ECQL parse →
to_filter→ compiled SQL check that the radiusreaches SQL as
5000, not0.005.testextra (excluding the modules that need a liveElasticsearch/OpenSearch/Solr or GDAL): 330 passed / 62 skipped / 37
errors, against a 316 / 62 / 37 baseline on
main— the delta isexactly the 14 new tests, and the 37 errors are the network-backed backend
suites, unrelated to this change.