Skip to content

[Maps] properly handle id collisions in Kibana index pattern - #48594

Merged
nreese merged 2 commits into
elastic:masterfrom
nreese:id_collision
Oct 21, 2019
Merged

[Maps] properly handle id collisions in Kibana index pattern#48594
nreese merged 2 commits into
elastic:masterfrom
nreese:id_collision

Conversation

@nreese

@nreese nreese commented Oct 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

ES document source FEATURE_ID_PROPERTY_NAME implemenation assumed _id would be unique across an Kibana index pattern. This is not the case. This PR updates the logic to namespace _id with _index to ensure uniqueness and fix problems where two features share the same _id for a layer.

To test the problem run the following in console. Then create an index pattern test*. Create a documents source from test* and verify both points are visible and you can page through tooltips for each

PUT test1
{}

PUT test1/_mapping
{
  "properties": {
  "location": {
    "type": "geo_point"
  },
  "color": {
    "type": "keyword"
  }
}
}

PUT test1/_doc/1
{
    "location": [-100, 60],
    "color": "blue"
}

PUT test2
{}

PUT test2/_mapping
{
  "properties": {
  "location": {
    "type": "geo_point"
  },
  "color": {
    "type": "keyword"
  }
}
}

PUT test2/_doc/1
{
    "location": [-100, 60],
    "color": "red"
}

Screen Shot 2019-10-17 at 4 07 48 PM

@nreese nreese added release_note:fix Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 v7.5.0 v7.6.0 labels Oct 17, 2019
@nreese
nreese requested a review from thomasneirynck October 17, 2019 22:07
@elasticmachine

elasticmachine commented Oct 17, 2019

Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@elasticmachine

elasticmachine commented Oct 17, 2019

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@elasticmachine

elasticmachine commented Oct 21, 2019

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

thomasneirynck
thomasneirynck approved these changes Oct 21, 2019
View reviewed changes

@thomasneirynck thomasneirynck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

great catch.

I would not roll back that performance optimization, it remains relevant afaic.

Comment thread x-pack/legacy/plugins/maps/public/elasticsearch_geo_utils.js
type: 'Feature',
geometry: tmpGeometriesAccumulator[j],
properties: properties
properties: {

@thomasneirynck thomasneirynck Oct 21, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This recreates the properties-object, copying everything. the reason the previous code specifically did not do this (https://github.com/elastic/kibana/pull/48594/files#diff-9c78f255b553d9556966390c977b5a08L80-L81) is because decoding should execute as tightly as possible. These are tight loops that benefit from being fast and have low mem-footprint. The main bottleneck is geometries (which can have deeply nested coordinate-arrays), but the same principle applies for properties. There's no harm in updating properties in place and copying the reference because the geojson collection is not reused elsewhere.

@nreese nreese Oct 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline, there is no way around this because otherwise we need a unique id per geometry in a document. I created issue #48829 to track how we handle array's of geometries.

The performance issues are much smaller then deep cloning the geometry since its just a single object where the geometry could contain multitudes of nested arrays

@nreese
nreese merged commit cc6aff7 into elastic:master Oct 21, 2019
nreese added a commit to nreese/kibana that referenced this pull request Oct 21, 2019
@nreese
[Maps] properly handle id collisions in Kibana index pattern (elastic…
9b79b3f 
…#48594)
@nreese nreese mentioned this pull request Oct 21, 2019
Merged
nreese added a commit to nreese/kibana that referenced this pull request Oct 21, 2019
@nreese
[Maps] properly handle id collisions in Kibana index pattern (elastic…
e9e2737 
…#48594)
@nreese nreese mentioned this pull request Oct 21, 2019
Merged
nreese added a commit that referenced this pull request Oct 22, 2019
@nreese
[Maps] properly handle id collisions in Kibana index pattern (#48594) (…
ff8b1e9 
…#48830)
nreese added a commit that referenced this pull request Oct 22, 2019
@nreese
[Maps] properly handle id collisions in Kibana index pattern (#48594) (…
cf339b0 
…#48831)
@nreese nreese mentioned this pull request Dec 11, 2019
Closed
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
@nreese
[Maps] properly handle id collisions in Kibana index pattern (elastic…
1f2fdf3 
…#48594)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@thomasneirynck thomasneirynck thomasneirynck approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release_note:fix Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.5.0 v7.6.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nreese @elasticmachine @thomasneirynck