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

ESQL: Documentation for support for geo_point and point #103207

Merged
merged 5 commits into from
Dec 12, 2023

Conversation

craigtaverner
Copy link
Contributor

In #102177 we added support for geo_point and cartesian point to ES|QL. This PR adds the docs for that.

@craigtaverner craigtaverner added >docs General docs changes :Analytics/Geo Indexing, search aggregations of geo points and shapes Team:QL (Deprecated) Meta label for query languages team :Analytics/ES|QL AKA ESQL labels Dec 8, 2023
Copy link
Contributor

github-actions bot commented Dec 8, 2023

Documentation preview:

@craigtaverner craigtaverner marked this pull request as ready for review December 8, 2023 15:56
@elasticsearchmachine elasticsearchmachine added Team:Docs Meta label for docs team Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Dec 8, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-docs (Team:Docs)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-ql (Team:QL)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

@craigtaverner craigtaverner changed the title Start working on geo_point and point docs for ESQL ESQL: Documentation for support for geo_point and point Dec 11, 2023
|===

If the input parameter is of numeric type `long`, its value will be interpreted as
the Lucene encoded value, with `x` encoded in the high four bytes,
Copy link
Member

Choose a reason for hiding this comment

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

We have to make sure we want to expose this. And, presuming we do, probably good to link to something here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This capability is very useful internally and for testing, but it is not clear to me that it does need to be exposed. It will become even more useful when we have both source and doc-values versions of the block data, each with specific purposes. For now, we could either remove it from the docs, or leave it there for now while the system is still in tech-preview, or keep it if in the end we want to expose it permanently.

Copy link
Contributor

@iverase iverase Dec 12, 2023

Choose a reason for hiding this comment

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

I think this is an implementation detail and it does not need to be exposed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should accept longs anyway

row long = 5009771769843126025
| eval pt = to_cartesianpoint(long);
| eval pt = to_cartesianpoint(long)
Copy link
Member

Choose a reason for hiding this comment

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

It'd be so nice if we could hex encode here. Oh well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A hex literal? That sounds nice! It is a language feature though, so not in scope for this PR.

include::{esql-specs}/spatial.csv-spec[tag=to_geopoint-str-result]
|===

If the input parameter is of numeric type `long`, its value will be interpreted as
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, we should not accept longs

Boolean *true* will be converted to long *1*, *false* to *0*.
==== Spatial points

If the input parameter is a spatial point (`geo_point` or `point`),
Copy link
Contributor

Choose a reason for hiding this comment

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

This again an implementation detail? do we need to expose it?

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.

Reading the docs makes me uncomfortable as we are leaking too many implementation details to the user:

  1. Our internal long representations of points should no be leaked to the user. This gives as the change to change it if we want to do so.

  2. Spatial point is not a elasticsearch concept but something introduce to handle internally geo and cartesian points. I don't think it should be leaked to the user API.

Would it be possible to hide those things to the user?

@iverase
Copy link
Contributor

iverase commented Dec 12, 2023

I think it is wrong that we can apply to_long operators to points. Sorry I did not catch this in the Pr but we should throw an error if a user tries to do that.

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.

Much simpler now.

I still think we should throw an error if we try to cast a point to a long or try to create a point from a long.

@craigtaverner craigtaverner merged commit e1835c9 into elastic:main Dec 12, 2023
15 checks passed
craigtaverner added a commit to craigtaverner/elasticsearch that referenced this pull request Jan 8, 2024
* Start working on geo_point and point docs for ESQL

* Added to_cartesianpoint and includes

* Sub-headings for easier reading

* Improve sub-headings

* Hide to_long and support for longs in to_geopoint and to_cartesianpoint
craigtaverner added a commit that referenced this pull request Jan 8, 2024
* ESQL: Documentation for support for geo_point and point (#103207)

* Start working on geo_point and point docs for ESQL

* Added to_cartesianpoint and includes

* Sub-headings for easier reading

* Improve sub-headings

* Hide to_long and support for longs in to_geopoint and to_cartesianpoint

* Fix missing links in geo_point docs for ESQL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL :Analytics/Geo Indexing, search aggregations of geo points and shapes >docs General docs changes Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Docs Meta label for docs team Team:QL (Deprecated) Meta label for query languages team v8.12.0 v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants