-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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 unmapped fields in search 'fields' option #65386
Conversation
Currently, the 'fields' option only supports fetching mapped fields. Since 'fields' is meant to be the central place to retrieve document content, it should allow for loading unmapped values. This change adds implementation and tests for this feature. Closes elastic#63690
Pinging @elastic/es-search (Team:Search) |
3193e0d
to
81b05d3
Compare
81b05d3
to
7bb350e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall approach looks good to me, I left some detailed comments.
docs/reference/search/search-your-data/retrieve-selected-fields.asciidoc
Outdated
Show resolved
Hide resolved
docs/reference/search/search-your-data/retrieve-selected-fields.asciidoc
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldAndFormat.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/SearchRequestBuilder.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/src/test/resources/rest-api-spec/test/flattened/10_basic.yml
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java
Show resolved
Hide resolved
@jtibshirani thanks a lot for your review so far, I pushed an update that should address your comments. |
server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some small final comments (accidentally split across two reviews).
server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java
Show resolved
Hide resolved
@jtibshirani I pushed another update to the branch and think I adressed your additional comments, thanks. Anything else you think needs doing here before merging? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I left one last note about an open comment, but it looks ready to merge after we sort that out.
Currently, the 'fields' option only supports fetching mapped fields. Since 'fields' is meant to be the central place to retrieve document content, it should allow for loading unmapped values. This change adds implementation and tests for this feature. Closes elastic#63690
Currently, the 'fields' option only supports fetching mapped fields. Since 'fields' is meant to be the central place to retrieve document content, it should allow for loading unmapped values. This change adds implementation and tests for this feature. Closes #63690
Currently, the 'fields' option only supports fetching mapped fields. Since
'fields' is meant to be the central place to retrieve document content, it
should allow for loading unmapped values. This change adds implementation and
tests for this feature.
Closes #63690