Opened 13 years ago
Closed 13 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 by , 13 years ago
comment:2 by , 13 years ago
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:4 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
sorry, my debug lines were wrong, GEODETIC flag is correctly set to false, going on with debugging
comment:10 by , 13 years ago
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:13 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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 !
1.5.4SVN works fine. 2.0.0SVN is broken on both 32 and 64 bit systems.