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 handling indexed envelopes crossing the dateline in mvt API #91105

Merged
merged 5 commits into from
Nov 2, 2022

Conversation

iverase
Copy link
Contributor

@iverase iverase commented Oct 25, 2022

This PR makes sure an envelope crossing the dateline generates a mutipolygon when creating a mvt feature so it is render properly.

fixes #91060

@iverase iverase added >bug :Analytics/Geo Indexing, search aggregations of geo points and shapes v8.5.1 v8.6.0 v7.17.8 labels Oct 25, 2022
@iverase iverase requested a review from craigtaverner October 25, 2022 06:54
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 25, 2022
@elasticsearchmachine
Copy link
Collaborator

Hi @iverase, I've created a changelog YAML for you.

@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@craigtaverner craigtaverner left a comment

Choose a reason for hiding this comment

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

LGTM, although I did have one question.

public void testFetchSourcePolygonDateLine() throws IOException {
assertFetchSourceGeometry(
"POLYGON((170 -10, -170 -10, -170 10, 170 10, 170 -10))",
"MULTIPOLYGON (((180.0 -10.0, 180.0 10.0, 170.0 10.0, 170.0 -10.0, 180.0 -10.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'm curious why the polygon test has a single polygon split to a multipolygon by the function:

fetchSourceValue(mapper, sourceValue, "wkt")

But the BBOX is not split by the same function in the envelope test?

Perhaps this is not part of this PR, since this PR focuses on MVT, and the MVT call:

fetchSourceValue(mapper, sourceValue, "mvt(0/0/0@4096)")

Does seem to split both the polygon and the envelope.

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 thought about the same. The didn't like the options I had to return a split geometry. Either return a multipolygon which didn't feel right for an input envelope, or a geometry collection containing two envelopes which I did not like it either.

Therefore I decided to return it unchanged. We can visit it later if it dies not feel right.

List<?> mvtExpected = fetchSourceValue(mapper, mvtPolygonDateLine, "mvt(0/0/0@256)");
List<?> mvt = fetchSourceValue(mapper, sourceValue, "mvt(0/0/0@256)");
List<?> mvtExpected = fetchSourceValue(mapper, mvtEquivalentAsWKT, "mvt(0/0/0@4096)");
List<?> mvt = fetchSourceValue(mapper, sourceValue, "mvt(0/0/0@4096)");
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests still pass with the original @256, what was the reason for changing to @4096? I presume it was just to match the default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's weird, I changed it so the extent is random.

@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.5
7.17 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 91105

iverase added a commit to iverase/elasticsearch that referenced this pull request Nov 2, 2022
…tic#91105)

This commit makes sure an envelope crossing the dateline generates a mutipolygon when creating a mvt feature 
so it is render properly.
@iverase iverase removed the v7.17.8 label Nov 2, 2022
elasticsearchmachine pushed a commit that referenced this pull request Nov 2, 2022
…) (#91254)

This commit makes sure an envelope crossing the dateline generates a mutipolygon when creating a mvt feature 
so it is render properly.
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Nov 3, 2022
* main: (1300 commits)
  update c2id/c2id-server-demo docker image to support ARM (elastic#91144)
  Allow legacy index settings on legacy indices (elastic#90264)
  Skip prevoting if single-node discovery (elastic#91255)
  Chunked encoding for snapshot status API (elastic#90801)
  Allow different decay values depending on the score function (elastic#91195)
  Fix handling indexed envelopes crossing the dateline in mvt API (elastic#91105)
  Ensure cleanups succeed in JoinValidationService (elastic#90601)
  Add overflow behaviour test for RecyclerBytesStreamOutput (elastic#90638)
  More actionable error for ancient indices (elastic#91243)
  Fix APM configuration file delete (elastic#91058)
  Clean up handshake test class (elastic#90966)
  Improve H3#hexRing logic and add H3#areNeighborCells method (elastic#91140)
  Restrict direct use of `ApplicationPrivilege` constructor (elastic#91176)
  [ML] Allow NLP truncate option to be updated when span is set (elastic#91224)
  Support multi-intersection for FieldPermissions (elastic#91169)
  Support intersecting multi-sets of queries with DocumentPermissions (elastic#91151)
  Ensure TermsEnum action works correctly with API keys (elastic#91170)
  Fix NPE in auditing authenticationSuccess for non-existing run-as user (elastic#91171)
  Ensure PKI's delegated_by_realm metadata respect run-as (elastic#91173)
  [ML] Update API documentation for anomaly score explanation (elastic#91177)
  ...

# Conflicts:
#	x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackClientPlugin.java
#	x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/RollupShardIndexer.java
#	x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/TransportRollupIndexerAction.java
#	x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/v2/RollupActionSingleNodeTests.java
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 Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.5.1 v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Envelopes crossing the dateline are not handled properly in vector tiles API
3 participants