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

Geo: Fixes indexing of linestrings that go around the globe #47471

Merged

Conversation

imotov
Copy link
Contributor

@imotov imotov commented Oct 2, 2019

Fixes indexing of linestrings that go around the globe.
LINESTRING (0 0, 720 20) is now decomposed into 3 strings:

multilinestring (
  (0.0 0.0, 180.0 5.0),
  (-180.0 5.0, 180 15),
  (-180.0 15.0, 0 20)
)

It also fixes issues with linestrings that intersect antimeridian
more than 5 times and adds optimization of decomposition
by -180 and 180 degrees at the same time.

Fixes #43837
Fixes #43826

LINESTRING (0 0, 720 20) is now decomposed into 3 strings:
multilinestring (
  (0.0 0.0, 180.0 5.0),
  (-180.0 5.0, 180 15),
  (-180.0 15.0, 0 20)
)

It also fixes issues with linestrings that intersect antimeridian
more than 5 times.

Fixes elastic#43837
Fixes elastic#43826
@imotov imotov added >bug :Analytics/Geo Indexing, search aggregations of geo points and shapes v8.0.0 v7.5.0 labels Oct 2, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Geo)

@imotov imotov requested review from iverase and talevy October 4, 2019 16:02
Copy link
Contributor

@talevy talevy left a comment

Choose a reason for hiding this comment

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

Overall looks good, left some comments that I hope will clear things up to uneducated readers

}
return lines;
}

double calculateShift(double lon, int direction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

since this is a very specific operation/definition of a normalized lon, maybe javadocs would be helpful here

lons[offset + i - 1] = intersection.getX();
lats[offset + i - 1] = intersection.getY();

double shift = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it difficult to keep this, calculateShift(..), and shift(..) separate. would some more distinct naming help, or would that just make things more confusing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shift is used all over the place and it is the actual shift as opposed to the amount of shift getting calculated and stored. I have renamed the method shift into performShift() here, I will rename the rest of them as I go through the changes in the areas where they are used.

@imotov imotov requested a review from talevy October 8, 2019 17:48
Copy link
Contributor

@talevy talevy left a comment

Choose a reason for hiding this comment

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

LGTM

@imotov imotov merged commit f9cb294 into elastic:master Oct 9, 2019
imotov added a commit that referenced this pull request Oct 9, 2019
LINESTRING (0 0, 720 20) is now decomposed into 3 strings:
multilinestring (
  (0.0 0.0, 180.0 5.0),
  (-180.0 5.0, 180 15),
  (-180.0 15.0, 0 20)
)

It also fixes issues with linestrings that intersect antimeridian
more than 5 times.

Fixes #43837
Fixes #43826
@imotov imotov deleted the issue-43837-fix-around-the-world-linestring branch May 1, 2020 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes >bug v7.5.0 v8.0.0-alpha1
Projects
None yet
4 participants