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 Point property for mapping point datatype #4735

Merged
merged 6 commits into from
Sep 18, 2020
Merged

Conversation

russcam
Copy link
Contributor

@russcam russcam commented Jun 4, 2020

Relates: #4718, elastic/elasticsearch#53804

This commit adds a Point property to map the new point data type.
A CartesianPoint type is introduced to model points.

@russcam
Copy link
Contributor Author

russcam commented Jun 4, 2020

The new point datatype can be used in shape queries. Shape queries however are bound to IGeoShape

/// <summary>
/// The geo shape to search with
/// </summary>
[DataMember(Name ="shape")]
IGeoShape Shape { get; set; }

which CartesianPoint does not implement. Think we may need to introduce new shape types here

@russcam russcam added the v7.8.1 label Jun 16, 2020
@russcam russcam added Feature and removed v7.8.1 labels Sep 4, 2020
@russcam
Copy link
Contributor Author

russcam commented Sep 4, 2020

After conversation with the geo team, it's clarified some questions around CartesianPoint:

CartesianPoint can be used for the null_value of a Point property field mapping, and allow users to use it on their documents to index into the Point field. To query on a Point field using the shape query, the PointGeoShape type can be used.

Another type (CartesianPoint) needs to be introduced because of the formats that must be supported for both the null_value of Point field mappings, and also for formats that may be used on _source documents outside of the client that the client must be able to deserialize:

  • Object: { "x": 41.12, "y": -71.34 }
  • String: "41.12,-71.34"
  • Array: [41.12,-71.34]
  • WKT: POINT (41.12 -71.34)

@russcam russcam force-pushed the feature/point-property branch from 76bdda6 to 5aac188 Compare September 4, 2020 01:57
@russcam russcam marked this pull request as ready for review September 8, 2020 00:05
@russcam
Copy link
Contributor Author

russcam commented Sep 8, 2020

This PR has been rebased against master and is ready for review, @Mpdreamz.

I think we should seriously consider renaming IGeoShape to IGeometry in 8.x, and the shape types that implement IGeoShape to remove the Geo name part. The renames better reflect their intent and usage, though I do appreciate that such a change could be a reasonably large breaking change, as geo queries are popular.

@russcam russcam requested a review from Mpdreamz September 15, 2020 05:16
@russcam russcam force-pushed the feature/point-property branch from 5aac188 to b2d449c Compare September 15, 2020 05:21
@russcam russcam force-pushed the feature/point-property branch from b2d449c to 39162dc Compare September 18, 2020 04:15
@russcam
Copy link
Contributor Author

russcam commented Sep 18, 2020

Merging this one in; last test run was green and this test run fails with tests unrelated to the changes.

@russcam russcam merged commit bbf6886 into master Sep 18, 2020
@russcam russcam deleted the feature/point-property branch September 18, 2020 11:09
github-actions bot pushed a commit that referenced this pull request Sep 18, 2020
Relates: #4718, elastic/elasticsearch#53804

This commit adds a Point property to map the new point data type.
A CartesianPoint type is introduced to model points.
russcam added a commit that referenced this pull request Sep 20, 2020
Relates: #4718, elastic/elasticsearch#53804

This commit adds a Point property to map the new point data type.
A CartesianPoint type is introduced to model points.

Co-authored-by: Russ Cam <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant