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

add shape-type metadata to geo_shape's doc-value #50104

Merged
merged 11 commits into from
Dec 18, 2019

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Dec 11, 2019

This commit serializes the ShapeType of the indexed
geometry. The ShapeType can be useful for other future
features. For one thing: #49887 depends on the ability
to determine what the highest dimensional shape is
for centroid calculations.

Since GeometryCollection is opaque, an additional
piece of data about the highest-dimension contained
within it is serialized and resolved at the time of reading.
This overhead is only present when indexing GeometryCollection.

relates #49569, #37206.

This commit serializes the ShapeType of the indexed
geometry. The ShapeType can be useful for other future
features. For one thing: elastic#49887 depends on the ability
to determine what the highest dimensional shape is
for centroid calculations.

GeometryCollection is reduced to the sub-shape of the
higest dimension

relates elastic#37206.
these Extent assertions were breaking tests where rectangles
that crossed the dateline were added. These assertions are
not required within the Extent logic since it is prepared
for bounding boxes that span the dateline
@talevy talevy requested a review from nknize December 11, 2019 23:47
@talevy talevy added the :Analytics/Geo Indexing, search aggregations of geo points and shapes label Dec 11, 2019
@elasticmachine
Copy link
Collaborator

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

@talevy
Copy link
Contributor Author

talevy commented Dec 12, 2019

@iverase I tried introducing a new DimensionalShapeType that represents both the shape and the dimension. I do think it is uglier, but it does save that extra byte. let me know what you think.

What is not included -> a conversion from DimensionalShapeType -> ShapeType. This would only be useful when implementing #49569

@talevy talevy requested a review from iverase December 13, 2019 17:02
@talevy
Copy link
Contributor Author

talevy commented Dec 16, 2019

run elasticsearch-ci/bwc
run elasticsearch-ci/default-distro

Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

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

In general looks good. I just think the dimensional type should be part of the centroid and should be skipped when getting the extent.

@talevy
Copy link
Contributor Author

talevy commented Dec 17, 2019

run elasticsearch-ci/1

@talevy talevy requested a review from iverase December 17, 2019 22:53
@talevy
Copy link
Contributor Author

talevy commented Dec 18, 2019

run elasticsearch-ci/1

@@ -33,8 +33,8 @@
* relations against the serialized triangle tree.
*/
public class TriangleTreeReader {
private static final int CENTROID_HEADER_SIZE_IN_BYTES = 9;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment where this 9 bytes are coming from.

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 will update this in the follow-up PR when this is modified to include the centroid weight!

Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

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

Just left a small comment. LGTM

@talevy
Copy link
Contributor Author

talevy commented Dec 18, 2019

thanks Ignacio!

@talevy talevy merged commit 3337858 into elastic:geoshape-doc-values Dec 18, 2019
@talevy talevy deleted the gdv-shapetype branch December 18, 2019 17:49
talevy added a commit that referenced this pull request Dec 19, 2019
This commit serializes the ShapeType of the indexed
geometry. The ShapeType can be useful for other future
features. For one thing: #49887 depends on the ability
to determine what the highest dimensional shape is
for centroid calculations.

GeometryCollection is reduced to the sub-shape of the
highest dimension

relates #37206.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants