Ticket #1273 (closed defect: fixed)

Opened 19 months ago

Last modified 18 months ago

ST_Equals( mypoint, postgis_addbbox(mypoint) ) = FALSE

Reported by: strk Owned by: pramsey
Priority: high Milestone: PostGIS 2.0.0
Component: postgis Version: trunk
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

Changed 19 months ago by strk

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

Changed 19 months 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.

Changed 19 months ago by strk

#1274 is the bug about the outermost use case

Changed 19 months 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...

Changed 19 months 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.

Changed 19 months 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.

Changed 19 months 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

Changed 19 months 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

Changed 19 months ago by strk

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

Changed 19 months 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 ?

Changed 19 months ago by pramsey

ST_Summary suggestion noted in #1277

Changed 19 months ago by pramsey

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

Changed 19 months 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 ? :)

Changed 18 months 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...

Changed 18 months 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))

Changed 18 months ago by strk

  • status changed from new to closed
  • resolution set to fixed

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.