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.

Sign up for GitHub

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

Jump to bottom

Set readFromDocValues to false for geo_shape fields #64014

Merged
merged 2 commits into from
Apr 22, 2020
Merged

Set readFromDocValues to false for geo_shape fields #64014

merged 2 commits into from
Apr 22, 2020

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Apr 20, 2020

Elasticsearch has added doc value support for geo_shape. geo_shape field does not support docvalue_fields, see elastic/elasticsearch#55487 (comment) for context.

This PR updates shouldReadFieldFromDocValues to return false for geo_shape fields.

@nreese nreese added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels Apr 20, 2020
@nreese nreese requested review from a team as code owners April 20, 2020 22:10
@nreese nreese added the Feature:Data Views Data Views code and UI - index patterns before 8.0 label Apr 20, 2020
@wylieconlon
Copy link
Contributor

@nreese Looks like this would also cause a migration to be needed?

@nreese
Copy link
Contributor Author

nreese commented Apr 20, 2020

Looks like this would also cause a migration to be needed?

Why do you say that? In the past geo_shape fields were not aggregatable so it was not an issue. Now, in Elasticsearch 7.8, geo_shape fields are aggregatable but can not be read from docvalue_fields. No existing index pattern will have marked geo_shape readFromDocValues as true

@nreese
Copy link
Contributor Author

nreese commented Apr 22, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

wylieconlon
wylieconlon reviewed Apr 22, 2020
View reviewed changes
x-pack/plugins/maps/public/layers/sources/es_search_source/es_search_source.js
@@ -51,7 +51,7 @@ function getDocValueAndSourceFields(indexPattern, fieldNames) {
lang: field.lang,
},
};
} else if (field.type !== ES_GEO_FIELD_TYPE.GEO_SHAPE && field.readFromDocValues) {
} else if (field.readFromDocValues) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay so the two cases here are:

  • Old index pattern, readFromDocValues is false on geo_shape fields because it wasn't previously supported
  • New index pattern, readFromDocValues is false because of the logic change in the index pattern service

Is that right?

Copy link
Contributor Author

@nreese nreese Apr 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That is correct.

For old index pattern readFromDocValues was false because geo_shape did not support docvalues and aggregations.

For new index patterns readFromDocValues should be false even though geo_shape has docvalues support and can support aggregations. This is do to changed in Elasticsearch 7.8 adding docvalue support to geo_shape fields

Copy link
Contributor Author

@nreese nreese Apr 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This line is removing a quick fix just merged (into master and 7.8) a few days ago. We decided a better fix is at the index pattern level.

@nreese nreese requested a review from wylieconlon April 22, 2020 18:27
lukeelmers
lukeelmers approved these changes Apr 22, 2020
View reviewed changes
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

App arch code changes here LGTM; thanks for the added tests!

wylieconlon
wylieconlon approved these changes Apr 22, 2020
View reviewed changes
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Code change LGTM, but did not run the change.

@nreese nreese merged commit 65264aa into elastic:master Apr 22, 2020
nreese added a commit to nreese/kibana that referenced this pull request Apr 22, 2020
@nreese @elasticmachine
Set readFromDocValues to false for geo_shape fields (elastic#64014)
4429791 
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@nreese nreese mentioned this pull request Apr 22, 2020
Merged
nreese added a commit that referenced this pull request Apr 23, 2020
@nreese @elasticmachine
Set readFromDocValues to false for geo_shape fields (#64014) (#64246)
fc6c0b9 
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukeelmers lukeelmers lukeelmers approved these changes

@wylieconlon wylieconlon wylieconlon approved these changes

Assignees
No one assigned
Labels
Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nreese @wylieconlon @kibanamachine @lukeelmers @elasticmachine