Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Search Source] Fix retrieval of unmapped fields; Add field filters #89837
[Search Source] Fix retrieval of unmapped fields; Add field filters #89837
Changes from 7 commits
e870118
a691322
5321c06
170cb50
08a25e6
892d18c
0788854
736d6a7
0364594
ff9cd2b
2872c36
f7c845e
21b011e
0bca218
259cf6b
4279ef8
c1aef7d
a9c9389
5b94504
01aa170
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm not sure if the check for
body._source
is needed?indexPatterns.getSourceFiltering()
will always return something, even if it's just{ excludes: [] }
, so if_source
isn't previously set it will already be set in L539.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.
We already have
fieldWildcardFilter
above which does this, maybe that can be reused, or this filtering can be done earlier on?Or actually: It seems like if we know we always want to filter items from
excludes
, we should filterbody.script_fields
andbody.runtime_mappings
as early as possible, so those values are never there to begin with?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.
Agreed, but we'd still need to explicitly exclude
fieldsFromSource
, right? So we'd end up with the same result basically?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.
Yeah, I guess what I'm getting at is: If
remainingFields
is determined by comparinguniqFieldNames
with script_fields and runtime_mappings, anduniqFieldNames
includesfields
andfieldsFromSource
, we could probably do this filtering a bit earlier on in one go by filteringfields
,fieldsFromSource
,script_fields
, andruntime_mappings
. Then you wouldn't have to worry about using any of those items in the rest of the function body because you'd know they are already filtered.Not a huge concern or anything, just felt like it might make this method (marginally) more readable.
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 may be overlooking something (and things might have changed since I last looked at this stuff), but I'm not sure if this part should be necessary.
The idea with search source is that you provide the index pattern, and this stuff happens internally so you don't need to worry about it. The filtering (and meta fields handling) should already happen inside search source via
fieldsWildcardFilter
.As a consumer of search source, all you should be responsible for is providing a field list, and search source would ideally handle everything else.
But I might be oversimplifying: Is this addressing a case that is not already covered by search source?
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.
So this particular use case is supposed to take care of
field filters
when using the fields API. Unlike_source
, fields API has no notion ofexcludes
, but rather -includes
. So we'd have to request a list of all fields, except the ones specified by the filters, which is what this code does. The logic doesn't exist (or doesn't work 😅 ) insearch_source
yet. TBF, I am not sure where the right place for it is. It felt cleaner to be in Discover, but happy to discuss alternatives.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.
Ah, this is the point I missed. Yeah this does ultimately feel like something search source should handle. I would expect that if you called
setField('fields', ['*'])
, then search source would do this internally. But let's see what @ppisljar thinksThere 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.
After some discussions on
date_nanos
handling, we have reached the conclusion that Discover would explicitly specify the date format for any date field when requesting fields (#90415 (comment)). I believe this logic belongs to the same place as requesting fields to include, so if we could now decide where that would be, it would be appreciated :)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.
Current functionality was that when not setting the fields we would get you all of them. We changed this on the searchSource level, so that we would no longer return unmapped ones when using the fieldsAPI. I think there is place for this logic to live in the searchsource, thus get to the old behaviour. We could also make this optional by exposing `showUnmappedFields' on the searchsource.
it seems from the linked PR that we are handling date_nanos on searchsource level as well, which makes sense imo
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 will change this then so that the logic lives in search source 👍