-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Geo: Do not normalize the longitude with value -180 for Lucene shapes #37299
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
Conversation
Pinging @elastic/es-analytics-geo |
@@ -357,7 +357,7 @@ protected static Polygon polygonS4J(GeometryFactory factory, Coordinate[][] poly | |||
double[] x = new double[shell.length]; | |||
double[] y = new double[shell.length]; | |||
for (int i = 0; i < shell.length; ++i) { | |||
x[i] = normalizeLon(shell[i].x); | |||
x[i] = Math.abs(shell[i].x) > 180 ? normalizeLon(shell[i].x) : shell[i].x; |
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.
Could you clarify why this is done only here and not in toPolygonLucene()
and in LineStringBuilder
?
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 am not an expert in JTS, but I guess the implementation understand that -180/+180 are the same point and it creates the right polygon. Lucene implementation, and in particular the tesselator implementation does not understand that and it won't create the right polygon. It will join the points without crossing the dateline.
I don't think it is a bug in Lucene's implementation but a feature and we need to adapt accordingly.
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.
Discussed it with @iverase and he clarified this. Thanks! I think it would be nice to add a javadoc comment here to explain why this is not needed for holes and in toPolygonLucene() to explain why it is not needed there.
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.
LGTM
@@ -357,7 +357,7 @@ protected static Polygon polygonS4J(GeometryFactory factory, Coordinate[][] poly | |||
double[] x = new double[shell.length]; | |||
double[] y = new double[shell.length]; | |||
for (int i = 0; i < shell.length; ++i) { | |||
x[i] = normalizeLon(shell[i].x); | |||
x[i] = Math.abs(shell[i].x) > 180 ? normalizeLon(shell[i].x) : shell[i].x; |
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.
Discussed it with @iverase and he clarified this. Thanks! I think it would be nice to add a javadoc comment here to explain why this is not needed for holes and in toPolygonLucene() to explain why it is not needed there.
…#37299) Lucene based shapes should not normalize the longitude value -180 to 180.
…#37299) Lucene based shapes should not normalize the longitude value -180 to 180.
Lucene based shapes should not normalize the longitude value -180 to 180.
Closes #37297