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

resolve ST_Equals TODO #40

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
@dbaston
Member

dbaston commented Jul 1, 2015

No description provided.

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 1, 2015

Member

Beware of rounded boxes.
Basically a bounding box may be slightly larger than needed when
it is created by reading the version cached in a serialized geometry.

This is because serialization forces use of floats by ensuring the
float version of the box fully covers the double version.
When reading things back the "fitting" box info is discarded.

Depending on where the values passed to ST_Equals originate
(read from disk or output from another function) the exact equality
might fail.

It would be fun to try at reproducing this scenario in a regression
test to proof what I'm saying and ensure it's not broken in the future.

Member

strk commented Jul 1, 2015

Beware of rounded boxes.
Basically a bounding box may be slightly larger than needed when
it is created by reading the version cached in a serialized geometry.

This is because serialization forces use of floats by ensuring the
float version of the box fully covers the double version.
When reading things back the "fitting" box info is discarded.

Depending on where the values passed to ST_Equals originate
(read from disk or output from another function) the exact equality
might fail.

It would be fun to try at reproducing this scenario in a regression
test to proof what I'm saying and ensure it's not broken in the future.

@dbaston

This comment has been minimized.

Show comment
Hide comment
@dbaston

dbaston Jul 1, 2015

Member

If the inputs to ST_Equals are both of type GSERIALIZED, won't both bounding boxes have forced to float somewhere upstream?

Member

dbaston commented Jul 1, 2015

If the inputs to ST_Equals are both of type GSERIALIZED, won't both bounding boxes have forced to float somewhere upstream?

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 1, 2015

Member

There are cases in which the bounding box is not included in the GSERIALIZED.
In those cases, it is computed with full precision when needed.

Your patch seems to be actually only using gbox_same_2d when both
inputs do have the cache, so should be safe.

Only I thought to mention the possibly hiding problem
(may be worth a comment in the code)

Member

strk commented Jul 1, 2015

There are cases in which the bounding box is not included in the GSERIALIZED.
In those cases, it is computed with full precision when needed.

Your patch seems to be actually only using gbox_same_2d when both
inputs do have the cache, so should be safe.

Only I thought to mention the possibly hiding problem
(may be worth a comment in the code)

@dbaston

This comment has been minimized.

Show comment
Hide comment
@dbaston

dbaston Jul 1, 2015

Member

I was also thinking about a gbox_same_float using the same rounding logic as gserialized_from_gbox, or maybe a more loose gbox_same_within_tol. Do you think one of those would be preferable?

Member

dbaston commented Jul 1, 2015

I was also thinking about a gbox_same_float using the same rounding logic as gserialized_from_gbox, or maybe a more loose gbox_same_within_tol. Do you think one of those would be preferable?

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 1, 2015

Member
Member

strk commented Jul 1, 2015

@pramsey

This comment has been minimized.

Show comment
Hide comment
@pramsey

pramsey Jul 7, 2015

Member

Applied to SVN at r13788, thanks

Member

pramsey commented Jul 7, 2015

Applied to SVN at r13788, thanks

@pramsey pramsey closed this Jul 7, 2015

strk pushed a commit to strk/postgis that referenced this pull request Jul 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment