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

[2.19.x] G-10122 Fix buffering across the antimeridian #6690

Merged
merged 2 commits into from
Jun 6, 2022

Conversation

jrnorth
Copy link
Contributor

@jrnorth jrnorth commented May 18, 2022

What does this PR do?

  • Enables normWrapLongitude in the Solr schema. Running a DWITHIN query (i.e., a buffered geometry) near the antimeridian often results in a Solr error because the resulting geometry contains points outside the valid longitude range of [-180,180]. The SolrFilterDelegate uses JTS to calculate the buffered geometry but does not enforce the longitude range on the result so it is passed to Solr with out-of-range values. Enabling normWrapLongitude means that Solr will wrap longitudes outside of the range into it.
  • Removes holes from polygons and multipolygons that cross the antimeridian. When handling polygons that cross the antimeridian, spatial4j requires any interior rings to be subsets of the exterior ring, which seems wrong (see https://github.com/locationtech/spatial4j/blob/2926812ae302c6e0f24fb5995b1fd24be7fe0b56/src/main/java/org/locationtech/spatial4j/shape/jts/JtsGeometry.java#L482).

Who is reviewing it?

@jlcsmith
@kcwire
@millerw8

Select relevant component teams:

@codice/solr

How should this be tested?

  1. Run gazetteer:update with https://github.com/codice/ddf/blob/03be912e331024f3bc6856d5e87f8e3b7db0b392/distribution/ddf-common/src/main/resources/data/countries.geo.json
  2. Run gazetteer:build-suggester-index
  3. Ingest the following files:
    russia1.json
{
  "type": "Feature",
  "geometry": {
    "type": "Point",
    "coordinates": [-179.9375, 69.116389]
  },
  "properties": {
    "title": "Russia 1"
  }
}

russia2.json

{
  "type": "Feature",
  "geometry": {
    "type": "Point",
    "coordinates": [179.900833, 71.637778]
  },
  "properties": {
    "title": "Russia 2"
  }
}

These locations are both within a 25km buffer of the Russia polygon, close to the antimeridian.
4. Open Intrigue and create an anyGeo query with the keyword 'Russia'. Run it and verify you get no results.
5. Add a buffer of 25km and verify you get both results.

Checklist:

  • Documentation Updated
  • Update / Add Threat Dragon models
  • Update / Add Unit Tests
  • Update / Add Integration Tests

Notes on Review Process

Please see Notes on Review Process for further guidance on requirements for merging and abbreviated reviews.

Review Comment Legend:

  • ✏️ (Pencil) This comment is a nitpick or style suggestion, no action required for approval. This comment should provide a suggestion either as an in line code snippet or a gist.
  • ❓ (Question Mark) This comment is to gain a clearer understanding of design or code choices, clarification is required but action may not be necessary for approval.
  • ❗ (Exclamation Mark) This comment is critical and requires clarification or action before approval.

Sorry, something went wrong.

@jrnorth
Copy link
Contributor Author

jrnorth commented May 18, 2022

build now

@cxddfbot
Copy link

Internal build has been started, your results will be available at build completion.

@cxddfbot
Copy link

Build SUCCESS See the job results in legacy Jenkins UI or in Blue Ocean UI.

@jrnorth jrnorth requested review from jlcsmith, kcwire and millerw8 May 18, 2022 22:44
@jlcsmith
Copy link
Member

Should this be forward ported to master?

@jlcsmith jlcsmith requested a review from pklinef May 20, 2022 12:06
@jlcsmith
Copy link
Member

Hero successful

@@ -765,6 +773,33 @@ public SolrQuery contains(String propertyName, String wkt) {
return operationToQuery("Contains", propertyName, wkt);
}

public static Geometry removeHoles(final Geometry geo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, obviously you saw some failures with this and had to remove them, but I am having trouble following why JTS doesn't handle these polygons. What was the error you were seeing? One of the assertions failing or an exception being thrown by JTS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to explain the issue in the second bullet point of the PR description. You've reminded me that I still need to add some comments to the code, too. This fix is to prevent this exception from being thrown: https://github.com/locationtech/spatial4j/blob/2926812ae302c6e0f24fb5995b1fd24be7fe0b56/src/main/java/org/locationtech/spatial4j/shape/jts/JtsGeometry.java#L492

Copy link
Contributor Author

Choose a reason for hiding this comment

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

locationtech/spatial4j#220
Trying to get some more information about the issue here.

@kcwire
Copy link
Member

kcwire commented May 20, 2022

With the addition of the normWrapLongitude to the Solr schema, will a full reindex of any data that might be impacted be required?

@jrnorth
Copy link
Contributor Author

jrnorth commented May 20, 2022

@kcwire

With the addition of the normWrapLongitude to the Solr schema, will a full reindex of any data that might be impacted be required?

That's a good question. I would think a reindex would be required, since any records with geometries outside that range should have failed to be indexed in the first place without normWrapLongitude enabled, but I can do some testing to verify that.

@jrnorth
Copy link
Contributor Author

jrnorth commented May 20, 2022

@jlcsmith

Should this be forward ported to master?

I think so. The normWrapLongitude change to the Solr schema has already been in master for a while, but the changes to SolrFilterDelegate are new.

@jrnorth
Copy link
Contributor Author

jrnorth commented May 20, 2022

@kcwire Looks like records outside that range are ingested successfully, they are just marked as invalid.
Turns out I mistakenly had normWrapLongitude enabled when I tested that again. Records outside [-180, 180] are not ingested, so a reindex is required.

ValidationRule.repairConvexHull.name(),
"normWrapLongitude",
"true",
"allowMultiOverlap",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to mention why this is necessary. Sometimes, we have polygons that share a border, such as the polygons of the Russia WKT on the antimeridian:
Screen Shot 2022-05-23 at 1 29 47 PM
Adding a buffer causes them to overlap.

@jrnorth
Copy link
Contributor Author

jrnorth commented Jun 2, 2022

build now

@jrnorth jrnorth added the Needs Hero Somebody to verify the change works as intended and explore for any additional issues label Jun 2, 2022
@cxddfbot
Copy link

cxddfbot commented Jun 2, 2022

Internal build has been started, your results will be available at build completion.

@cxddfbot
Copy link

cxddfbot commented Jun 3, 2022

Build SUCCESS See the job results in legacy Jenkins UI or in Blue Ocean UI.

@jlcsmith jlcsmith merged commit d11874b into codice:2.19.x Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Hero Somebody to verify the change works as intended and explore for any additional issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants