Skip to content
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

GEOSSTRtree_remove behaviour change in 3.10? #544

Closed
dark-panda opened this issue Jan 14, 2022 · 0 comments · Fixed by #545
Closed

GEOSSTRtree_remove behaviour change in 3.10? #544

dark-panda opened this issue Jan 14, 2022 · 0 comments · Fixed by #545
Assignees
Labels
Milestone

Comments

@dark-panda
Copy link
Contributor

dark-panda commented Jan 14, 2022

I am currently upgrading my ffi-geos Ruby gem (https://github.com/dark-panda/ffi-geos) for 3.10 compatibility and ran across a failing test involving GEOSSTRtree_remove. The test is effectively the following (forgive the Ruby code, but this should be hopefully fairly straight forward):

def test_remove
  @tree = Geos::STRtree.new(3)
  @item_1 = { item_1: :test }
  @item_2 = [:test]
  @item_3 = Object.new

  @geom_1 = read('LINESTRING(0 0, 10 10)')
  @geom_2 = read('LINESTRING(20 20, 30 30)')
  @geom_3 = read('LINESTRING(20 20, 30 30)')

  @tree.insert(@geom_1, @item_1)
  @tree.insert(@geom_2, @item_2)
  @tree.insert(@geom_3, @item_3)

  @tree.remove(read('POINT (5 5)'), @item_1)

  assert_equal([],
    @tree.query(read('LINESTRING(5 5, 6 6)')))

  assert_equal([],
    @tree.query(read('LINESTRING(20 0, 30 10)')))

  assert_equal([@item_2, @item_3],
    @tree.query(read('LINESTRING(25 25, 26 26)')))

  assert_equal([@item_2, @item_3],
    @tree.query(read('LINESTRING(0 0, 100 100)')))
end

In 3.9 and prior, the line for @tree.remove(read('POINT (5 5)'), @item_1) correctly removes the item from the tree and the tests succeed. In 3.10 and in main, the tests fail because the item is not removed.

I noticed there is no test coverage for GEOSSTRtree_remove, which is what I base most of my Ruby tests for new code on, so I'm unsure if I'm correctly implementing Geos::STRtree#remove in my Ruby gem, although it has worked up until this point. When I debug the GEOS code via llvm, I'm noticing that it's bailing out at this point:

https://github.com/libgeos/geos/blob/main/include/geos/index/strtree/TemplateSTRtree.h#L309

bool remove(const BoundsType& itemEnv, const ItemType& item) {
  if (root == nullptr) { // the root is a nullptr coming from my Ruby code
    return false;
  }

Is there something amiss with the implementation or something that needs doing in my Ruby code, or has there been a behaviour change that would affect GEOSSTRtree_remove? At a minimum, I believe there may be a missing regression test for GEOSSTRtree_remove specifically, as there are no references to it outside of the capi/ directory.

The code for my implementation of Geos::STRtree#remove can be found here:

https://github.com/dark-panda/ffi-geos/blob/master/lib/ffi-geos/strtree.rb#L98

@dbaston dbaston self-assigned this Jan 15, 2022
@dbaston dbaston added the Bug label Jan 15, 2022
@dbaston dbaston added this to the 3.10.2 milestone Jan 15, 2022
dbaston added a commit to dbaston/libgeos that referenced this issue Jan 15, 2022
dbaston added a commit to dbaston/libgeos that referenced this issue Jan 15, 2022
dbaston added a commit to dbaston/libgeos that referenced this issue Jan 15, 2022
dbaston added a commit that referenced this issue Jan 15, 2022
* Fix STRtree remove on unbuilt tree

References #544

* Fix STRtree query after removing item

References #544

* Fix test for MSVC 2015
dbaston added a commit to dbaston/libgeos that referenced this issue Jan 15, 2022
* Fix STRtree remove on unbuilt tree

References libgeos#544

* Fix STRtree query after removing item

References libgeos#544

* Fix test for MSVC 2015
dbaston added a commit that referenced this issue Jan 15, 2022
* Multiple fixes to STRtree remove method (#545)

* Fix STRtree remove on unbuilt tree

References #544

* Fix STRtree query after removing item

References #544

* Fix test for MSVC 2015

* NEWS update for STRtree fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants