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

SQL: Add basic support for ST_AsWKT geo function #34205

Merged
merged 3 commits into from
Oct 23, 2018

Conversation

imotov
Copy link
Contributor

@imotov imotov commented Oct 1, 2018

Adds basic support for ST_AsWKT function. The function takes accepts
geo shape or geo point and returns its WKT representation.

Adds basic support for ST_AsWKT function. The function takes accepts
geo shape or geo point and returns its WKT representation.
@imotov imotov added >feature :Analytics/Geo Indexing, search aggregations of geo points and shapes v7.0.0 :Analytics/SQL SQL querying labels Oct 1, 2018
@imotov imotov requested review from costin, nknize and astefan October 1, 2018 22:23
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

Copy link
Contributor

@nknize nknize left a comment

Choose a reason for hiding this comment

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

This looks pretty good. I really only have one question re: o.e.xpack.sql.expression.function.scalar.geo.GeoShape. It looks to be mostly just a wrapper around .toWKT and ShapeParser.parse. Is it necessary? Could we move this class to o.e.common.geo? That might give a good starting point for consolidating in the move to BKD backed geoshapes? I'm also okay w/ doing this is a future PR.

Great work!

/**
* Wrapper class to represent a GeoShape in SQL
*/
public class GeoShape implements ToXContentFragment {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, is this class necessary for the SQL plugin? It seems to mostly be a wrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The wrapper overrides the XContent serialization to use WKT instead of GeoJSON, which is default representation on the SQL side. So, now that I am thinking about it I am not sure if it makes sense to move this to sql package as we previously discussed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, no problem. We can take a closer look at moving things around after we get BKD backed shapes committed so I'm not too worried about this atm.

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.

Looks good. Left a comment about adding more tests.

;

selectWithAsWKTInWhere
SELECT city, ST_ASWKT(location) location, region FROM "geo" WHERE LOCATE('114', ST_ASWKT(location)) > 0 ORDER BY "city";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add more tests that use the new functions in ORDER BY, GROUP BY and HAVING? This should test the Painless script in various setups. Same goes for tests using these functions inside other functions or the input of these functions to come from other functions, useful for testing folding.

@astefan
Copy link
Contributor

astefan commented Oct 8, 2018

You should, probably, add documentation for these functions as well.

@imotov
Copy link
Contributor Author

imotov commented Oct 19, 2018

@astefan I have added documentation and more tests. I couldn't come up with any reasonable tests with HAVING mostly because of #33519. I am planning to simplify the ORDER BY clause when we have #33361 fixed and when we have other geo functions. Could you take a look when you have some time?

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 0280428 into elastic:geosql Oct 23, 2018
@jpountz jpountz removed the :Analytics/Geo Indexing, search aggregations of geo points and shapes label Jan 29, 2019
@imotov imotov deleted the geosql-add-format-conversion-function branch May 1, 2020 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants