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

Bugfix: ExactEquals not correct when both geoms are empty points #444

Merged
merged 1 commit into from
Mar 21, 2022

Conversation

peterstace
Copy link
Owner

Description

Fix bug in `ExactEquals` for empty Points

The `ExactEquals` method didn't properly take into consideration
coordinates type (XY vs. XYZ vs. XYM vs. XYZM) when both Point arguments
were empty.

The actual fix is only in the `pointsEq` method. In the previous code,
`ok1` and `ok2` would both evaluate to `false` (because the `Points` are
both empty). This would then skip the call to `c.eq(c1, c2)`, which was
where the coordinates type comparison occurred.

The fix is to explicitly check for the case where both `Point`s are
empty, and handle the coordinates type check there. The `c.eq` method
still needs to check for coordinate type, to ensure that inputs such as
`POINT (1 2)` and `POINT Z (1 2 3)` continue to compared as non-equal.

Check List

Have you:

  • Added unit tests? Yes.

  • Add cmprefimpl tests? (if appropriate?) N/A

  • Updated release notes? (if appropriate?) N/A

Related Issue

  • N/A

Benchmark Results

N/A

The `ExactEquals` method didn't properly take into consideration
coordinates type (XY vs. XYZ vs. XYM vs. XYZM) when both Point arguments
were empty.

The actual fix is only in the `pointsEq` method. In the previous code,
`ok1` and `ok2` would both evaluate to `false` (because the `Points` are
both empty). This would then skip the call to `c.eq(c1, c2)`, which was
where the coordinates type comparison occurred.

The fix is to explicitly check for the case where both `Point`s are
empty, and handle the coordinates type check there. The `c.eq` method
still needs to check for coordinate type, to ensure that inputs such as
`POINT (1 2)` and `POINT Z (1 2 3)` continue to compared as non-equal.
@peterstace peterstace self-assigned this Mar 18, 2022
@peterstace peterstace changed the base branch from master to fix_pct_w_verb_in_Errorf March 18, 2022 00:51
@@ -155,9 +155,12 @@ func (c exactEqualsComparator) lineStringsEq(ls1, ls2 LineString) bool {
}

func (c exactEqualsComparator) pointsEq(p1, p2 Point) bool {
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is the actual bugfix. The other changes are just additions to the unit test cases.

@@ -155,9 +155,12 @@ func (c exactEqualsComparator) lineStringsEq(ls1, ls2 LineString) bool {
}

func (c exactEqualsComparator) pointsEq(p1, p2 Point) bool {
if p1.IsEmpty() && p2.IsEmpty() {
return p1.CoordinatesType() == p2.CoordinatesType()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can two non-empty points with different coordinate types ever be considered equal? Or perhaps this is handled by c.eq(c1, c2).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, that case is handled by the call to c.eq(c1, c2).

Base automatically changed from fix_pct_w_verb_in_Errorf to master March 21, 2022 20:47
@peterstace peterstace merged commit 1f6fc08 into master Mar 21, 2022
@peterstace peterstace deleted the fix_bug_in_exact_equals branch March 21, 2022 22:18
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.

None yet

2 participants