Opened 8 years ago

Closed 8 years ago

#1273 closed defect (fixed)

ST_Equals( mypoint, postgis_addbbox(mypoint) ) = FALSE

Reported by: strk Owned by: pramsey
Priority: high Milestone: PostGIS 2.0.0
Component: postgis Version: master
Keywords: Cc:

Description

Try this:

WITH p AS ( SELECT 'POINT(832694.188 816254.625)'::geometry as g ) 
SELECT st_equals(p.g, postgis_addbbox(p.g)) from p;

It will give you _false_ !

Change History (16)

comment:1 Changed 8 years ago by strk

1.5.4SVN works fine. 2.0.0SVN is broken on both 32 and 64 bit systems.

comment:2 Changed 8 years ago by strk

For the record: this case came out from an attempt to convert a set of polygons into a topology using topology.ST_CreateTopoGeom. There is _no_ call to postgis_addbbox() in there, so some other function is also producing a point-with-a-box, which in turn makes ST_Equals fail. That other function would probably be another bug.

comment:3 Changed 8 years ago by strk

#1274 is the bug about the outermost use case

comment:4 Changed 8 years ago by strk

I guess r8116 was meant to fix this, but it didn't and I don't think that using a tolerance is the way to go here...

comment:5 Changed 8 years ago by strk

So, postgis_addbbox uses lwgeom_add_bbox over a deserialized geometry, which is then re-serialized, while ST_Equals uses gserialized_get_bbox_p over the serialized geometry.

lwgeom_add_bbox in turn calls lwgeom_calculate_gbox_p. gserialized_get_bbox_p also calls lwgeom_calculate_gbox_p.

This is the theory. But something goes wrong in between.

comment:6 Changed 8 years ago by strk

This notice (I've added to the code) is interesting enough:

=# select postgis_addbbox('POINT(832694.188 816254.625)'::geometry); NOTICE: lwgeom already has a box

postgis_addbbox


0101000000378941606C69294100000040FDE82841

It should _not_ have a bbox... The code checks by looking at the LWGEOM.bbox, not at the flag.

comment:7 Changed 8 years ago by strk

Is it normal for a point to be marked as being GEODETIC ?

=# WITH p AS ( SELECT 'POINT(832694.188 816254.625)'::geometry as g ) 
SELECT _st_equals(p.g, postgis_addbbox(p.g)) from p;
NOTICE:  GEOM 0x7fcd42084230 marked as being GEODETIC
NOTICE:  GEOM 0x7fcd42084230 marked as being GEODETIC
NOTICE:  GEOM 0x7fcd42084260 marked as having a BBOX
NOTICE:  short-circuiting boxes of geom 0x7fcd42084230 and 0x7fcd42084260
NOTICE:   box1: 832694.188 816254.625,832694.188 816254.625
NOTICE:   box2: 832694.1875 816254.625,832694.25 816254.625
 _st_equals 
------------
 f

comment:8 Changed 8 years ago by strk

I guess we'd want ST_Summary report a [G] flag for "GEODETIC" ? Might be a useful replacement for the now-mandatory [S]rid

comment:9 Changed 8 years ago by strk

sorry, my debug lines were wrong, GEODETIC flag is correctly set to false, going on with debugging

comment:10 Changed 8 years ago by strk

Ok, found the issue. gserialized_read_gbox_p is forcing a cached box to float: g_serialized.c:

        /* Has pre-calculated box */
        if ( FLAGS_GET_BBOX(g->flags) )
        {
                int i = 0;
                float *fbox = (float*)(g->data);
                gbox->xmin = fbox[i++];
                gbox->xmax = fbox[i++];
                gbox->ymin = fbox[i++];
                gbox->ymax = fbox[i++];

Why is that ?

comment:11 Changed 8 years ago by pramsey

ST_Summary suggestion noted in #1277

comment:12 Changed 8 years ago by pramsey

The cached box *is* float, so there's no forcing going on here.

comment:13 Changed 8 years ago by strk

r8134 fixes this case by having gserialized_read_gbox_p correctly produce the BOX "as if it was just read from cache" even in the POINT and 2-points LINE case.

I'm not completely sure about what would happen with other kind of geometries as I see gserialized_get_gbox_p calls lwgeom_calculate_gbox in that case, which probably doesn't produce a FLOAT-aware version of them.

I guess it will be worth adding much more tests for that. Probably involving postgis_dropbbox and a MULTIPOINT would do. I'll leave this ticket in case someone wants to work on the tests (paul, you do want to work on the tests don't you ? :)

comment:14 Changed 8 years ago by strk

Ok, here's the next failing case:

WITH p AS ( SELECT 'MULTIPOINT((832694.188 816254.625))'::geometry as g ) SELECT '#1273.1', st_equals(p.g, postgis_dropbbox(p.g)) from p;

As expected by last comment...

comment:15 Changed 8 years ago by strk

Notices from the code:

NOTICE:  Got gbox from cache: GBOX((832694.19,816254.62),(832694.25,816254.62))
NOTICE:     Calculating gbox: GBOX((832694.19,816254.62),(832694.19,816254.62))

comment:16 Changed 8 years ago by strk

Resolution: fixed
Status: newclosed

r8203 fixes the latter case, hopefully closing this specific issue forever. Regress test for the new case added too.

Unfortunately, the commit log points to the _wrong_ bug (1023 instead of this one). Sorry for that !

Note: See TracTickets for help on using tickets.