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 GeoPoint in StreamInput/StreamOutput #13632

Merged
merged 1 commit into from Oct 8, 2015

Conversation

clement-tourriere
Copy link

Fixes transport serialization for geo_point type in multi node environment (possibility to use geo_point in script and fielddata_fields).

Closes #13340

@nik9000
Copy link
Member

nik9000 commented Oct 7, 2015

I'd love to see a test here so it doesn't break in future and so I can be sure exactly what this fixes.

@nik9000 nik9000 self-assigned this Oct 7, 2015
@jpountz
Copy link
Contributor

jpountz commented Oct 8, 2015

Another thing that is needed would be for GeoPoint to implement ToXContent so that it is properly rendered in the response. I think otherwise we will use toString() and the current GeoPoint.toString() produces an output that looks like geo-json except that it puts the latitude first :s By using ToXContent at least we could use the non-ambiguous representation that writes geo points as an object with "lat" and "lon" properties.

@javanna
Copy link
Member

javanna commented Oct 8, 2015

Another thing that is needed would be for GeoPoint to implement ToXContent

/me wonders why we haven't done it as part of the query-refactoring. sounds like a nice improvement.

One more tiny comment is if we do introduce a public StreamInput#readGeoPoint method, I would also add a corresponding StreamOutput#writeGeoPoint.

@jpountz
Copy link
Contributor

jpountz commented Oct 8, 2015

You can ignore my previous comment: I assumed we would use GeoPoint.toString given that GeoPoint does not implement ToXContent but actually we special-case the GeoPoint class in XContentBuilder so everything should be ok.

@s1monw s1monw merged commit cc0f8c2 into elastic:master Oct 8, 2015
@s1monw
Copy link
Contributor

s1monw commented Oct 8, 2015

merged thanks!

@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Fielddata labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories v2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants