Skip to content

Add support for missing value fetchers.#63515

Merged
jtibshirani merged 5 commits intoelastic:masterfrom
jtibshirani:value-fetchers
Oct 12, 2020
Merged

Add support for missing value fetchers.#63515
jtibshirani merged 5 commits intoelastic:masterfrom
jtibshirani:value-fetchers

Conversation

@jtibshirani
Copy link
Copy Markdown
Contributor

@jtibshirani jtibshirani commented Oct 8, 2020

This PR implements value fetching for the following field types:

  • text phrase and prefix subfields
  • search_as_you_type, plus its subfields
  • token_count, which is implemented by fetching doc values

Supporting these types helps ensure that retrieving all fields through
"fields": ["*"] doesn't fail because of unsupported value fetchers.

@jtibshirani jtibshirani added >enhancement :Search/Search Search-related issues that do not fall into other categories labels Oct 8, 2020
@jtibshirani jtibshirani marked this pull request as ready for review October 12, 2020 15:49
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Oct 12, 2020

Pinging @elastic/es-search (:Search/Search)

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Oct 12, 2020
nik9000
nik9000 approved these changes Oct 12, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

When I was prototyping grok runtime fields I had a similar problem with throwing from valueFetcher. I think maybe we should add some guidance that you should never throw from this.

I wonder if it'd make sense to have some kind of "there are no values in this field" kind of marker so that * skips them. Not a "for now" problem - this seems perfect.

romseygeek
romseygeek reviewed Oct 12, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

I'm not sure that the implementation is correct for the text/search-as-you-type subfields? I may just be missing something though.

@jtibshirani
Copy link
Copy Markdown
Contributor Author

jtibshirani commented Oct 12, 2020 •
edited
Loading

I'm not sure that the implementation is correct for the text/search-as-you-type subfields? I may just be missing something though.

This current implementation indeed works (and is tested through FieldFetcherTests#testTextSubFields). Because the internal fields are modelled as multi-fields, the logic in SourceValueFetcher to resolve multi-fields in _source kicks in and we successfully retrieve the value from the parent field. It felt simplest to just treat them as multi-fields instead of adding special (and redundant) logic to look in the parent. I should really at least add a comment, at first glance it's unclear why this works!

romseygeek
romseygeek approved these changes Oct 12, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

I should really at least add a comment, at first glance it's unclear why this works!

That would be great, thanks. LGTM otherwise!

@jtibshirani
Copy link
Copy Markdown
Contributor Author

jtibshirani commented Oct 12, 2020

@nik9000 your DocValueFetcher addition is coming in really handy :)

I wonder if it'd make sense to have some kind of "there are no values in this field" kind of marker so that * skips them.

I guess an alternative approach would be to return no values instead of throwing. Right now, I'd prefer to keep the approach simple and return values for all registered fields. If we don't want to return values, then I think a better option is to avoid registering internal text fields at all -- I filed #63446 to discuss this idea.

@jtibshirani jtibshirani merged commit 62857b4 into elastic:master Oct 12, 2020
@jtibshirani jtibshirani deleted the value-fetchers branch October 12, 2020 20:57
@jtibshirani jtibshirani mentioned this pull request Oct 12, 2020
Merged
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Oct 12, 2020
@jtibshirani
Add support for missing value fetchers. (elastic#63515)
9334899 
This PR implements value fetching for the following field types:
* `text` phrase and prefix subfields
* `search_as_you_type`, plus its subfields
* `token_count`, which is implemented by fetching doc values

Supporting these types helps ensure that retrieving all fields through
`"fields": ["*"]` doesn't fail because of unsupported value fetchers.
@jtibshirani jtibshirani mentioned this pull request Oct 12, 2020
Merged
@andreidan andreidan added v7.10.0 and removed v7.10.1 labels Oct 15, 2020
@Mpdreamz Mpdreamz mentioned this pull request Nov 16, 2020
Closed
61 tasks
@stevejgordon stevejgordon mentioned this pull request Dec 17, 2020
Closed
@cbuescher cbuescher mentioned this pull request Feb 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nik9000 nik9000 nik9000 approved these changes

@romseygeek romseygeek romseygeek approved these changes

Assignees

No one assigned

Labels

>enhancement :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v7.10.0 v7.11.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jtibshirani @elasticmachine @nik9000 @romseygeek @jakelandis @andreidan