Skip to content

Commit

Permalink
Allow nullptr to be a valid item in the tree
Browse files Browse the repository at this point in the history
  • Loading branch information
@pramsey
pramsey committed Nov 28, 2020
1 parent b3043f9 commit ad01225
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 27 deletions.
2 changes: 1 addition & 1 deletion include/geos/index/strtree/SimpleSTRnode.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ class GEOS_DLL SimpleSTRnode : public ItemBoundable {

bool isLeaf() const override
{
return item != nullptr;
return childNodes.size() == 0;
}

bool isComposite() const
Expand Down
41 changes: 15 additions & 26 deletions tests/unit/capi/GEOSSTRtreeTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -258,46 +258,43 @@ void object::test<7>
GEOSSTRtree_destroy(tree);
}


// querying tree with box
template<>
template<>
void object::test<8>
()
{
GEOSSTRtree* tree = GEOSSTRtree_create(10);

GEOSGeometry* g = GEOSGeomFromWKT("POINT (2 3)");
GEOSSTRtree_insert(tree, g, g);
int payload = 876;
GEOSSTRtree_insert(tree, g, &payload);

GEOSGeometry* q = GEOSGeomFromWKT("POLYGON ((0 0, 10 0, 10 10, 0 10, 0 0))");

typedef std::vector<GEOSGeometry*> GList;
GList geoms;
ensure_equals(geoms.size(), 0);
typedef std::vector<int*> IList;
IList items;
ensure_equals(items.size(), 0);
GEOSSTRtree_query(
tree,
q,
[](void* item, void* userdata) {
GList* gl = (GList*)userdata;
gl->push_back((GEOSGeometry*)item);
IList* il = (IList*)userdata;
il->push_back((int*)item);
},
&geoms);
&items);

ensure_equals(geoms.size(), 1);
const GEOSCoordSequence* seq = GEOSGeom_getCoordSeq(geoms[0]);
ensure_equals(items.size(), 1);

double x = -1;
double y = -1;
GEOSCoordSeq_getXY(seq, 0, &x, &y);
ensure_equals(x, 2.0);
ensure_equals(y, 3.0);
ensure_equals(*(items[0]), payload);

GEOSGeom_destroy(q);
GEOSGeom_destroy(g);
GEOSSTRtree_destroy(tree);
}


// querying tree with box
// Index a null pointer
template<>
template<>
void object::test<9>
Expand All @@ -306,8 +303,7 @@ void object::test<9>
GEOSSTRtree* tree = GEOSSTRtree_create(10);

GEOSGeometry* g = GEOSGeomFromWKT("POINT (2 3)");
int payload = 876;
GEOSSTRtree_insert(tree, g, &payload);
GEOSSTRtree_insert(tree, g, (void*)0);

GEOSGeometry* q = GEOSGeomFromWKT("POLYGON ((0 0, 10 0, 10 10, 0 10, 0 0))");

Expand All @@ -325,21 +321,14 @@ void object::test<9>

ensure_equals(items.size(), 1);

ensure_equals(*(items[0]), payload);
ensure_equals(items[0], (void*)0);

GEOSGeom_destroy(q);
GEOSGeom_destroy(g);
GEOSSTRtree_destroy(tree);
}


// >>> import pygeos
// >>> pygeos.geos_version
// (3, 9, 0)
// >>> point = pygeos.Geometry("POINT (2 3)")
// >>> tree = pygeos.STRtree([point])
// >>> tree.query(pygeos.box(0, 0, 10, 10))
// array([], dtype=int64)


} // namespace tut
Expand Down

4 comments on commit ad01225

@dbaston
Copy link
Member

Choose a reason for hiding this comment

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

If we can not support sorting nullptr in the tree, I'd prefer that. In the tree implementation I'm working on a node uses 64 bytes, and supporting nullptr as an item will, I think, push that up to 72 bytes which is not so nicely aligned.

The use case for storing nullptr seems to PyGEOS casting an integer to a pointer to store integers in the tree: https://github.com/pygeos/pygeos/blob/master/src/strtree.c#L175
I'd be surprised if this isn't undefined behavior.

@jorisvandenbossche

@dbaston
Copy link
Member

@dbaston dbaston commented on ad01225 Nov 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I take that back, looks like I'm still at 56 bytes. Not sure that changes my doubt about the need to store nullptr though.

@jorisvandenbossche
Copy link
Contributor

Choose a reason for hiding this comment

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

I we can fix it on the PyGEOS side, I am certainly fine with GEOS not supporting storing null pointers. PyGEOS is a young enough project that it is fine to have us do a new release to correctly support GEOS 3.9, I think.

Now, I suppose the reason we cast the int to void* is because that's the signature of GEOSSTRtree_insert_r in the C API. But we're not experienced C programmers, so we might misinterpreted / miscoded this.

@dbaston
Copy link
Member

Choose a reason for hiding this comment

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

GEOS doesn't provide a way to store values in the tree, only pointers to values. Casting an int to void* probably usually works, since they're usually the same size. But the compiler is not required to make it work. (That's what I mean by "undefined behavior"). To store integers in the tree, you need to keep the integers themselves in memory somewhere and store pointers to the integers in the tree.

Please sign in to comment.