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 doc_values mapping option to geo_shape field mapping #47519

Merged
merged 11 commits into from
Feb 24, 2020

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Oct 3, 2019

This PR adds support for the doc_values field mapping parameter.

true and false supported by the GeoShapeFieldMapper,
only false is supported by the LegacyGeoShapeFieldMapper.

relates #37206

This PR adds TypeParsers.parseField to the fieldType parsing in
AbstractGeometryFieldMapper. This enables support for common mapping
configuration options like `copy_to`, `doc_values`, `store`, `index`,
`index_options`.

some options are not necessarily used, but all MappedFieldTypes recognize
them: `similarity`, `boost`.
@talevy talevy added the :Analytics/Geo Indexing, search aggregations of geo points and shapes label Oct 3, 2019
@talevy talevy requested a review from imotov October 3, 2019 17:33
@elasticmachine
Copy link
Collaborator

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

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.

I think it might make sense to add tests for everything that we added support for here. I played with copy_to for a bit and it looks like it no longer complain about it being present, but it doesn't seem to copy anything.

@iverase
Copy link
Contributor

iverase commented Oct 9, 2019

+1 to add tests to the different options.

@talevy
Copy link
Contributor Author

talevy commented Oct 9, 2019

ah, thanks for the reviews. I didn't realize more had to be implemented on that front. thought all the copy_to behavior would be inherited

@talevy talevy changed the title add support for common field mapping options in geometry field types add doc_values mapping option to geo_shape field mapping Jan 6, 2020
@talevy talevy requested a review from imotov January 6, 2020 23:21
@talevy
Copy link
Contributor Author

talevy commented Jan 6, 2020

Hi Igor, after re-visiting this / forgetting about it... I realize that it is unclear how to support other options that TypeParsers supports like copy_to and such. For this reason, I am only adding doc_values parsing support.

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.

I feel like there are still a few bits missing. For example I would expect it to show up in doXContentBody as well if have some merge handling.

@talevy talevy requested a review from iverase February 20, 2020 00:55
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.

lgtm

@talevy talevy merged commit 4c410f0 into elastic:geoshape-doc-values Feb 24, 2020
@talevy talevy deleted the gdv-parsefields branch February 24, 2020 17:58
talevy added a commit that referenced this pull request Feb 24, 2020
This PR adds support for the `doc_values` field mapping parameter.

`true` and `false` supported by the GeoShapeFieldMapper,
only `false` is supported by the LegacyGeoShapeFieldMapper.

relates #37206
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants