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

Switch to using EqualsExact in geometry comparer #18946

Merged
merged 1 commit into from
Nov 16, 2019

Conversation

roji
Copy link
Member

@roji roji commented Nov 16, 2019

Fixes #18921

@roji roji requested a review from bricelam November 16, 2019 20:25
@bricelam
Copy link
Contributor

We should review the translations for the various equals methods too

@roji roji merged commit 293dab3 into master Nov 16, 2019
@roji roji deleted the GeometryCollectionComparer branch November 16, 2019 22:22
@roji
Copy link
Member Author

roji commented Nov 16, 2019

Yeah.

Here's a useful PostGIS page on the different kind of equalities etc: https://postgis.net/workshops/postgis-intro/equality.html

Npgsql translates EqualsExact to the PostGIS equality operator, and EqualsTopologically to PostGIS ST_Equals. After reading the equality page above it seems like translating EqualsExact to ST_OrderingEquals may be more correct...

@roji
Copy link
Member Author

roji commented Nov 16, 2019

Opened npgsql/efcore.pg#1131 for the Npgsql side.

@roji
Copy link
Member Author

roji commented Nov 19, 2019

@bricelam checked and there don't appear to be translations of EqualsExact for either SQL Server or Sqlit (SQL Server doesn't even allow equals operator: Invalid operator for data type. Operator equals equal to, type equals geometry.)...

Will do the translation in Npgsql (is it worth adding tests in SpatialQueryTestBase test suite if neither provider will actually exercise them...).

BTW all the tests in SpatialQueryTestBase have the tested expression in a top-level projection, which is a bit dangerous since it will do client evaluation for providers which don't translate. Any reason not to exercise them in a Where clause instead?

@bricelam
Copy link
Contributor

You can add a test to the base to encourage other providers to implement it if possible.

Using Where got too tricky since precision, etc. was very different between providers. I decided asserting the SQL and loosely verifying the result was the most pragmatic thing to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use exact instead of topological comparison when comparison spatial types
2 participants