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

[Maps] geotile_grid aggregation #29477

Closed
wants to merge 10 commits into from
Closed

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jan 28, 2019

Fixes #28485

Requires elastic/elasticsearch#37842

This PR replaces geohash_grid aggregation with geotile_grid aggregation

@nreese nreese added v7.0.0 [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v6.7.0 labels Jan 28, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@nreese
Copy link
Contributor Author

nreese commented Jan 28, 2019

cc @nyurik

@nyurik
Copy link
Contributor

nyurik commented Jan 28, 2019

This looks awesome! One small thing - I noticed that the precision needs to be rounded to the nearest integer.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@@ -142,13 +140,6 @@ export class ESGeoGridSource extends AbstractESSource {
query: searchFilters.query,
});

if (this._descriptor.requestType === RENDER_AS.GRID) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now convertToGeoJson returns feature collection of grids when renderAs is GRID to avoid looping over data second time.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nyurik
Copy link
Contributor

nyurik commented Jan 31, 2019

The precision requested by the app is too high, and frequently exceeds the 10,000 bucket limit. If we say that a screen is ~20 tiles, with partial tiles visible that makes it to 30. Having zoom+7 means that for each of the 30 tiles visible on screen, you are requesting 4^7 buckets - 16,384 -- way too much, and so is +6 (4,096). +5 (1,024/tile) is doable if you make several requests 9 tiles each (they have to be rectangular, so either 3x3 or 4x2), and stitch them together on the client. Thus, +4 (256/tile) is the maximum reasonable one if you have no more than 39 tiles per request.

Also, make sure you handle server error responses (e.g. bucket count exceeded).

I wonder if we should have a slider instead of a dropdown, with the slider going from (zoom-1 .. zoom+4) ?

Lastly, each request should be aligned to the tile boundaries rather than the screen, and it would be great if the response was cached per tile, not per request.

@nreese
Copy link
Contributor Author

nreese commented Jan 31, 2019

Closing PR. Work has been put on the feature branch upstream feature/geotile_grid and @thomasneirynck is taking over

@nreese nreese closed this Jan 31, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@thomasneirynck
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants