-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Conversation
Pinging @elastic/es-core-infra (Team:Core/Infra) |
Pinging @elastic/es-analytics-geo (Team:Analytics) |
@elasticmachine run elasticsearch-ci/packaging-tests-windows-sample |
There was a problem hiding this 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
There was a problem hiding this 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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
_mvt end point gives priority to geometries with larger area
* [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>
* [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>
* [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>
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.