-
Notifications
You must be signed in to change notification settings - Fork 180
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
Conversation
build now |
Internal build has been started, your results will be available at build completion. |
Build SUCCESS See the job results in legacy Jenkins UI or in Blue Ocean UI. |
Should this be forward ported to master? |
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
With the addition of the |
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 |
I think so. The |
@kcwire |
ValidationRule.repairConvexHull.name(), | ||
"normWrapLongitude", | ||
"true", | ||
"allowMultiOverlap", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build now |
Internal build has been started, your results will be available at build completion. |
Build SUCCESS See the job results in legacy Jenkins UI or in Blue Ocean UI. |
What does this PR do?
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. EnablingnormWrapLongitude
means that Solr will wrap longitudes outside of the range into it.Who is reviewing it?
@jlcsmith
@kcwire
@millerw8
Select relevant component teams:
@codice/solr
How should this be tested?
gazetteer:update
with https://github.com/codice/ddf/blob/03be912e331024f3bc6856d5e87f8e3b7db0b392/distribution/ddf-common/src/main/resources/data/countries.geo.jsongazetteer:build-suggester-index
russia1.json
russia2.json
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:
Notes on Review Process
Please see Notes on Review Process for further guidance on requirements for merging and abbreviated reviews.
Review Comment Legend: