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

Vector tiles: order hits by geometry size by default #75621

Merged
merged 4 commits into from
Jul 27, 2021

Conversation

iverase
Copy link
Contributor

@iverase iverase commented Jul 22, 2021

Vector tiles can only display polygons and lines that are big enough for the precision of the tile. This might create strange results where the hits layer of a vector tile contains no data even though the query have hits. In order to provide expected results we are ordering the hits result by geometry size so we always render polygon and lines which are big enough on respect to the tile precision.

The first issue is to define how to compute the geometry size. This size needs to computed in respect to the spherical mercator projection as it introduces distortion. In this approach we use the diagonal size of the geometry bounding in the spherical mercator projection as ordering value.

In order to perform this, we introduce the following changes:

  • Introduce two new methods in our ScriptDocValue geometry interface:
    ** getMercatorWidth(): width of the geometry in the spherical mercator projection.
    ** getMercatorHeight(): Height of the geometry in the spherical mercator projection.

  • Move SphericalMercatorUtil to server under common/geo.

Then if there is no ordering provided by the user when calling the _mvt API and the size of the request > 0, then we add a order by using a script that computes the diagonal of the geometry square. This is used to order the results.

Note: We have consider to use filtering but this would affect the results on the aggregation.

@iverase iverase added >non-issue :Analytics/Geo Indexing, search aggregations of geo points and shapes :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v8.0.0 v7.15.0 labels Jul 22, 2021
@iverase iverase requested a review from imotov July 22, 2021 10:40
@elasticmachine elasticmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Core/Infra Meta label for core/infra team labels Jul 22, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticmachine
Copy link
Collaborator

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

@iverase iverase changed the title Vector tiles: order hit by geometry size by default Vector tiles: order hits by geometry size by default Jul 22, 2021
@iverase
Copy link
Contributor Author

iverase commented Jul 22, 2021

@elasticmachine run elasticsearch-ci/packaging-tests-windows-sample

Copy link
Contributor

@jdconrad jdconrad left a comment

Choose a reason for hiding this comment

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

LGTM from the scripting side

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

As I mentioned before I think it would have been much better if we had a dedicated sort for that. But considering that this is a somewhat exotic feature we can probably go with the script for now, but we shouldn't generate this script on the fly.

new ScriptSortBuilder(
new Script(
"double w = doc[\""
+ getField()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the script should be a static string and field names should be passed into it as a script parameter, building this script on the fly just feels wrong for some reason.

@jdconrad would it make sense to do the doc lookup here only once to avoid an additional HashMap lookup in the local cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes! Good catch. I see @iverase has already incorporated this suggestion 👍

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@iverase iverase merged commit 02c2943 into elastic:master Jul 27, 2021
@iverase iverase deleted the orderBySize branch July 27, 2021 16:34
iverase added a commit that referenced this pull request Jul 28, 2021
_mvt end point gives priority to geometries with larger area
ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request Jul 30, 2021
_mvt end point gives priority to geometries with larger area
elasticsearchmachine pushed a commit that referenced this pull request Aug 5, 2021
* [DOCS] Document `_mvt` API

Documents the `_mvt` API endpoint added with #73872.

Relates to #75242.

* Reword

* Rename API

* Fix doc.url in JSON spec

* Reword

* Reword

* Add content type to JSON spec

* Edits

* Fix typo

* Reword

* Update docs after meeting

* Fix typos

* Fix `size` default

* Updates for #75522

* Fixes

* Clean up JSON spec

* Fix extent tag

* [DOCS] Add `<field>` constraints

* Minor clarification

* Update for #75697

* Reword

* Update for #75621

* Reword default sort

* Update for #75367

* Remove unneeded whitespace

* Add experimental admon and if flags

* [DOCS] Remove ifdefs

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
elasticsearchmachine pushed a commit to elasticsearchmachine/elasticsearch that referenced this pull request Aug 5, 2021
* [DOCS] Document `_mvt` API

Documents the `_mvt` API endpoint added with elastic#73872.

Relates to elastic#75242.

* Reword

* Rename API

* Fix doc.url in JSON spec

* Reword

* Reword

* Add content type to JSON spec

* Edits

* Fix typo

* Reword

* Update docs after meeting

* Fix typos

* Fix `size` default

* Updates for elastic#75522

* Fixes

* Clean up JSON spec

* Fix extent tag

* [DOCS] Add `<field>` constraints

* Minor clarification

* Update for elastic#75697

* Reword

* Update for elastic#75621

* Reword default sort

* Update for elastic#75367

* Remove unneeded whitespace

* Add experimental admon and if flags

* [DOCS] Remove ifdefs

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
elasticsearchmachine added a commit that referenced this pull request Aug 5, 2021
* [DOCS] Document `_mvt` API

Documents the `_mvt` API endpoint added with #73872.

Relates to #75242.

* Reword

* Rename API

* Fix doc.url in JSON spec

* Reword

* Reword

* Add content type to JSON spec

* Edits

* Fix typo

* Reword

* Update docs after meeting

* Fix typos

* Fix `size` default

* Updates for #75522

* Fixes

* Clean up JSON spec

* Fix extent tag

* [DOCS] Add `<field>` constraints

* Minor clarification

* Update for #75697

* Reword

* Update for #75621

* Reword default sort

* Update for #75367

* Remove unneeded whitespace

* Add experimental admon and if flags

* [DOCS] Remove ifdefs

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
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 :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Core/Infra Meta label for core/infra team v7.15.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants