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

Fix overlay op regression with mixed GeometryCollection on 3.9 branch #1050

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

mwtoews
Copy link
Contributor

@mwtoews mwtoews commented Feb 24, 2024

This fixes a regression in the 3.9 branch (only the 3.9.5 release), which permits overlay ops GEOSDifference with mixed-type GeometryCollection inputs. It more-or-less reverts ad62ddf but limits the caught exception to geos::util::IllegalArgumentException.

The added CAPI test passes on newer branches too (with some mods), and I'll possibly add this test into other branches after this PR looks good.

Closes #1023


There's just one outstanding thing that I don't fully understand. By design, the low-level C++ level does not support overlay ops with mixed GeometryCollection types. But it doesn't appear to be consistent, e.g. (here "GC" is a mixed GeometryCollection and "PT" is a Point):

  • GC.difference(PT) // raises IllegalArgumentException
  • PT.difference(GC) // doesn't raise, result is PT

demonstrated with tests/unit/operation/overlayng/OverlayNGMixedPointsTest.cpp:

/**
* Mixed collections not supported at a low-level
*/
template<>
template<>
void object::test<12> ()
{
    std::string a = "GEOMETRYCOLLECTION (POINT (51 -1), LINESTRING (52 -1, 49 2))";
    std::string b = "POINT (2 3)";
    std::unique_ptr<Geometry> geom_a = r.read(a);
    std::unique_ptr<Geometry> geom_b = r.read(b);

    // A.difference(B) does not work
    try {
        std::unique_ptr<Geometry> geom_result = OverlayNG::overlay(geom_a.get(), geom_b.get(), OverlayNG::DIFFERENCE);
        fail();
    } catch(geos::util::IllegalArgumentException const& e) {
        ensure_equals(std::string(e.what()), "IllegalArgumentException: Overlay input is mixed-dimension");
    }
 
    // B.difference(A) works (?!)
    testOverlay(b, a, b, OverlayNG::DIFFERENCE, 1);
} 

is this expected, or unusual?

@mwtoews mwtoews added this to the 3.9.6 milestone Feb 24, 2024
@mwtoews mwtoews changed the title Test overlay op with mixed GeometryCollection Fix overlay op regression with mixed GeometryCollection on 3.9 branch Feb 26, 2024
@mwtoews mwtoews requested a review from strk February 26, 2024 07:39
@mwtoews mwtoews force-pushed the regres-9.5-mixed-gc branch from 6cc22d9 to 1515cab Compare February 26, 2024 07:39
@mwtoews
Copy link
Contributor Author

mwtoews commented Feb 26, 2024

Also curious to know about the broader implications such as https://trac.osgeo.org/postgis/ticket/5401

@pramsey
Copy link
Member

pramsey commented Feb 26, 2024

Sorry, broader implications? I mean, we could try to characterize how different versions deal with mixed inputs, ending up at the current state in which they are explicitly handled and passed through the NG engine entirely... at some level we want to just try and not knock over the Jenga tower, since retrofitting main/3.12 behaviour back onto stable branches would cause other consistency changes in output.

@mwtoews
Copy link
Contributor Author

mwtoews commented Mar 4, 2024

Ok, perhaps we'll let the xref'ed broader implications slide for now, as we are essentially restoring the behaviour to the same as four releases (3.9.0 .. 3.9.4).

@mwtoews mwtoews merged commit f781576 into libgeos:3.9 Mar 4, 2024
6 checks passed
@mwtoews mwtoews deleted the regres-9.5-mixed-gc branch March 4, 2024 20:27
@pramsey
Copy link
Member

pramsey commented Mar 4, 2024

Good plan. I don't see a decent way to "fix" what was reported in 5401 and their eventual move to a more modern release will clear things up for them.

@mwtoews
Copy link
Contributor Author

mwtoews commented Mar 4, 2024

Backports of the test are pushed to be62def (3.10), dc4c486 (3.11), 22fe1a6 (3.12), and bdceea6 (main). No other code changes are needed for these newer branches.

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

Successfully merging this pull request may close these issues.

2 participants