Skip to content

Conversation

@imotov
Copy link
Contributor

@imotov imotov commented May 2, 2019

Adds support for the ST_Z function that returns the altitude of the
first point in a shape.

At the moment ST_Z functionality is quite limited since we are
not storing it in geo_point doc values and geo_shape doesn't
support doc values at the moment. So, this function cannot
be used in the query or aggregation contexts.

Relates to #29872

Adds support for the ST_Z function that returns the altitude of the
first point in a shape.
@imotov imotov added >enhancement :Analytics/Geo Indexing, search aggregations of geo points and shapes :Analytics/SQL SQL querying labels May 2, 2019
@imotov imotov requested review from astefan, costin and matriv May 2, 2019 22:52
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

LGTM, Should we add the limitation to the docs limitations page?

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM in general. Left two minor comments.
Also, I would have expected to see more tests for this function. Unless I'm missing something, atm there is only the one: the one used in docs.


.Description:

Returns the the altitude of the first point in the geometry.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: two "the".

.Description:

Returns the the altitude of the first point in the geometry.
The return type is double.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have added, instead of this short statement that seems to have the same purpose as the *Output*: double from above: Returns the altitude of the first point in the geometry as a double type number.

@astefan
Copy link
Contributor

astefan commented May 3, 2019

Also, you need to add the new function to the functions' index page: https://github.com/elastic/elasticsearch/blob/geosql/docs/reference/sql/functions/index.asciidoc

@astefan
Copy link
Contributor

astefan commented May 3, 2019

@imotov
Copy link
Contributor Author

imotov commented May 3, 2019

@astefan I didn't add any QueryTranslatorTests for ST_Z because as I mentioned in the description, it doesn't work in the query context. It generates a query, but this query has no chances of succeeding, which is wrong. This happens because we don't have doc values for shapes and altitudes and as a result we cannot access this information in a query. It is a common usability issue for all ST_* functions that I am planning to tackle next. I opened #41791 to track it. @matriv I will also describe these limitations as part of the issue, just need to find a good spot for it.

@costin
Copy link
Member

costin commented May 6, 2019

"this function cannot be used in the query or aggregation contexts"
This can be addressed by adding a validation rule to verify that the functions appear only inside a projection and nowhere else (similar to what we do with SCORE).
Considering this is not released yet (will it be part of 7.1?), it can be addressed in a separate PR.

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

Left a comment.

@imotov
Copy link
Contributor Author

imotov commented May 6, 2019

"this function cannot be used in the query or aggregation contexts"

I need to revise this statement. II think I formulated it too strongly. The function can appear in the query context, but it cannot work on fields because we don't have access to source in this contexts. For example, SELECT * FROM foo WHERE some_z > ST_Z(ST_WKTTOSQL('POINT(1 2 3)'); is is somewhat meaningless, but perfectly fine way to use it. Basically the problem is not in ST_Z, but the problem is in geoDocValue() that is generated as part all functions. This shouldn't happen for geo_shapes since they don't have doc_values yet.

Considering this is not released yet (will it be part of 7.1?), it can be addressed in a separate PR.

It's too late for 7.1 I am targeting 7.2 at the moment, I will merge PR that addresses this issue before opening a PR to merge the branch to master and 7.x.

@astefan astefan self-requested a review May 6, 2019 14:24
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM

@imotov imotov merged commit d5a43c3 into elastic:geosql May 6, 2019
@elasticmachine elasticmachine mentioned this pull request May 9, 2019
13 tasks
@imotov imotov deleted the geosql-st-z branch May 1, 2020 22:12
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 :Analytics/SQL SQL querying >enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants