-
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
Returning nested data in fields API #67432
Conversation
4b1a092
to
bceb692
Compare
After discussing this internally there have been some changes to the output format that I'll try to summarize:
Here are some examples:
returns
returns
Directly querying a nested field has no output. This is consistent with querying an object field in the current fields API:
BUT using "user.*" returns and groups all leave fields underneath "user":
|
I have added some more example to this gist but leave them out here for brevity. |
Pinging @elastic/es-search (Team:Search) |
While there are still some non-final decisions on this PR and I'd expect the implementation code to get cleaned up quite a bit, I removed the WIP status since I think the output is what we are aiming for. |
server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/search/fetch/subphase/InnerHitsIT.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java
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.
This approach makes sense to me overall. I still need to go over a few details but left some initial comments.
server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/NestedFieldValueFetcher.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/ValueFetcher.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/NestedFieldValueFetcher.java
Outdated
Show resolved
Hide resolved
Collections.sort(in); | ||
String lastAddedEntry = in.get(0); | ||
shortestPrefixes.add(lastAddedEntry); | ||
for (String entry : in) { |
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.
Will this work correctly when nested field names are naturally prefixes of each other? Like nested
and nested2
? I wonder if we need to consult the mapping information directly to get the parent nested mappings. Another edge case that comes to mind is if field names contain special characters that could sort before periods .
.
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.
true, I changed an existing test so it shows this shortcoming and changed the lookup for the nested parents out of all available nested mappers.
@jtibshirani thanks for the feedback, hope the last one fixes the last problem |
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.
Approved! Reposting our follow-ups here so it's easy to track:
- Handling mixed object and dot notation
- Making sure DocValueFetcher in nested documents don't error
- Adding a note in the docs
@elasticmachine run elasticsearch-ci/packaging-sample-unix |
Add a note about the changes in the `fields` API around the response format for nested fields. Relates to elastic#67432
@jtibshirani I also added a note to the migration docs in #68813 as we discussed, maybe you can take a quick look |
Add a note about the changes in the `fields` API around the response format for nested fields. Relates to #67432
This change adds tests around the handling of mixed object and dot notation in document source when using the `fields` API with nested fields left out of elastic#67432. After merging elastic#68540, this test can now be added. Relates to elastic#67432
This is a POC on one way of returning nested data with the fields API (#63709).
This PR includes fields of the "nested" type in the output of the fields API response and applies the fields API logic to each individual sub-document under that nested field path recursively. This means we return a flattend list of leaf values for all fields outside of any nested object and group leaf fields inside each nested document indivudually.
For anl example document, where both "user" and "user.adress" are field of the nested type:
For a "fields" : ["*"] query we would return
An alternative would be to not return "user.adress" as an individual key in the above example but only inside "user". If wanted, we could also choose to return all the flattened leave fields (also the ones that are inside nested documents) in the root of the fields response as a flattened list like we do now.
This is just a draft with only basic testing. I want to extend testing with unmapped fields inside nested documents and also try solving the problem that currently I don't pass down the "ignore_field" content inside the NestedFieldValueFetcher.