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

Support WKT point conversion to geo_point type #44107

Merged
merged 10 commits into from
Jul 12, 2019
Merged

Support WKT point conversion to geo_point type #44107

merged 10 commits into from
Jul 12, 2019

Conversation

Hohol
Copy link
Contributor

@Hohol Hohol commented Jul 9, 2019

This PR adds support for parsing geo_point values from WKT POINT format.
Also, a few minor bugs in geo_point parsing were fixed.

Closes #41821

@cbuescher cbuescher added :Analytics/Geo Indexing, search aggregations of geo points and shapes v8.0.0 labels Jul 9, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

@imotov imotov self-requested a review July 9, 2019 12:42
@imotov
Copy link
Contributor

imotov commented Jul 9, 2019

@elasticmachine test this please

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

Looks like a great start! I left some comments.

-fix parameters in docs
-use WellKnownText parser
-add negative tests
@imotov
Copy link
Contributor

imotov commented Jul 10, 2019

@elasticmachine test this please

@Hohol
Copy link
Contributor Author

Hohol commented Jul 10, 2019

I've found the cause of SQL query failure. I'll see what can be done with it.

@imotov
Copy link
Contributor

imotov commented Jul 10, 2019

Perhaps the division of logic between FieldMapper parser and GeoUtil is not quite right.

@Hohol
Copy link
Contributor Author

Hohol commented Jul 10, 2019

The reason was in code duplication. Namely, the same check if (val.contains(",")) repeated in multiple places. To get rid of it, I moved some logic from GeoUtils to GeoPoint.
Also, I noticed even more redundant code can be removed in GeoPointFieldMapper.

@imotov
Copy link
Contributor

imotov commented Jul 10, 2019

@elasticmachine test this please

@Hohol
Copy link
Contributor Author

Hohol commented Jul 11, 2019

Do you have any idea why these tests failed? Something flaky again? Maybe I need to merge with master?

@imotov
Copy link
Contributor

imotov commented Jul 11, 2019

@elasticmachine run elasticsearch-ci/bwc
@elasticmachine run elasticsearch-ci/packaging-sample

@imotov
Copy link
Contributor

imotov commented Jul 11, 2019

Yeah, might be good idea to merge in master.

@Hohol
Copy link
Contributor Author

Hohol commented Jul 11, 2019

Merged.

@imotov
Copy link
Contributor

imotov commented Jul 11, 2019

@elasticmachine test this please

@talevy talevy self-requested a review July 11, 2019 20:26
Copy link
Contributor

@talevy talevy left a comment

Choose a reason for hiding this comment

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

awesome @Hohol, looks great!

I just left one comment I thought would help others. let me know what you think!

docs/reference/mapping/types/geo-point.asciidoc Outdated Show resolved Hide resolved
@Hohol
Copy link
Contributor Author

Hohol commented Jul 11, 2019

@talevy, done.

@talevy
Copy link
Contributor

talevy commented Jul 11, 2019

thanks!

@elasticmachine test this please

Copy link
Contributor

@talevy talevy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@imotov imotov merged commit a851992 into elastic:master Jul 12, 2019
@Hohol Hohol deleted the wkt-geo-point branch July 12, 2019 17:02
imotov pushed a commit that referenced this pull request Jul 12, 2019
This PR adds support for parsing geo_point values from WKT POINT format.
Also, a few minor bugs in geo_point parsing were fixed.

Closes #41821
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Nov 13, 2019
Relates: elastic/elasticsearch#44107

This commit adds support for Well Known Text (WKT) representations for GeoLocation, the type used
to represent geo_point in the client. Similar to WKT support in IGeoShape, an internal format field
is used to store whether an instance was deserialized from JSON or WKT, so that re-serialization
honours the original form deserialized from.
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Nov 14, 2019
Relates: elastic/elasticsearch#44107

This commit adds support for Well Known Text (WKT) representations for GeoLocation, the type used
to represent geo_point in the client. Similar to WKT support in IGeoShape, an internal format field
is used to store whether an instance was deserialized from JSON or WKT, so that re-serialization
honours the original form deserialized from.
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Nov 14, 2019
Relates: elastic/elasticsearch#44107

This commit adds support for Well Known Text (WKT) representations for GeoLocation, the type used
to represent geo_point in the client. Similar to WKT support in IGeoShape, an internal format field
is used to store whether an instance was deserialized from JSON or WKT, so that re-serialization
honours the original form deserialized from.

(cherry picked from commit 3f3f98c)
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Nov 14, 2019
Relates: elastic/elasticsearch#44107

This commit adds support for Well Known Text (WKT) representations for GeoLocation, the type used
to represent geo_point in the client. Similar to WKT support in IGeoShape, an internal format field
is used to store whether an instance was deserialized from JSON or WKT, so that re-serialization
honours the original form deserialized from.

(cherry picked from commit 3f3f98c)
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 >enhancement v7.4.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Question\Sugestion] Support WKT point conversion to geo_point type
7 participants