[Search Source] Fix retrieval of unmapped fields; Add field filters #89837

majagrubic · 2021-02-01T10:00:50Z

Summary

This PR fixes the issue in the current where unmapped fields are not retrieved when retrieving fields from source.

It also adds source filter functionality to fields API, by explicitly requesting a list of fields from index pattern that are NOT excluded by the field filters.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

majagrubic · 2021-02-01T13:02:09Z

@elasticmachine merge upstream

kibanamachine · 2021-02-01T13:09:31Z

⏳ Build in-progress, with failures

continuous-integration/kibana-ci/pull-request
Commit: a691322
This comment will update when the build is complete

Failed CI Steps

History

💔 Build #102515 failed e870118

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

lukeelmers · 2021-02-03T17:18:19Z

src/plugins/data/common/search/search_source/search_source.ts

@@ -539,7 +538,7 @@ export class SearchSource {
      if (!body.hasOwnProperty('_source')) {
        body._source = sourceFilters;
      }
-      if (body._source.excludes) {
+      if (body._source && body._source.excludes) {


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.

lukeelmers · 2021-02-03T17:30:19Z

src/plugins/data/common/search/search_source/search_source.ts

+      if (body._source && body._source.excludes) {
+        // TODO: optimize complexity
+        remainingFields = remainingFields.filter((remainingField) => {
+          return !body._source.excludes.includes(remainingField);
+        });
+      }


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 filter body.script_fields and body.runtime_mappings as early as possible, so those values are never there to begin with?

Agreed, but we'd still need to explicitly exclude fieldsFromSource, right? So we'd end up with the same result basically?

Agreed, but we'd still need to explicitly exclude fieldsFromSource, right?

Yeah, I guess what I'm getting at is: If remainingFields is determined by comparing uniqFieldNames with script_fields and runtime_mappings, and uniqFieldNames includes fields and fieldsFromSource, we could probably do this filtering a bit earlier on in one go by filtering fields, fieldsFromSource, script_fields, and runtime_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.

src/plugins/data/common/search/search_source/search_source.ts

lukeelmers · 2021-02-03T17:41:35Z

src/plugins/discover/public/application/helpers/get_field_list.ts

+  const fieldsToInclude = fields.filter((field) => {
+    return !sourceFiltersValues.includes(field.name) && !META_FIELDS.includes(field.name);
+  });


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?

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 of excludes, 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 😅 ) in search_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.

fields API has no notion of excludes, 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.

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 thinks

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 :)

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

I will change this then so that the logic lives in search source 👍

spalger · 2021-02-03T18:20:19Z

jenkins test this

nreese

LGTM - This PR does a better job of removing duplicate fields from _source which is great, thanks.
code review, tested in chrome

nreese · 2021-02-05T15:11:54Z

src/plugins/data/common/search/search_source/search_source.test.ts

+      const request = await searchSource.getSearchRequestBody();
+      expect(request.stored_fields).toEqual(['geometry', 'prop1']);
+      expect(request.docvalue_fields).toEqual(['prop1']);
+      expect(request._source).toEqual({


Thanks for moving this into its own test case.

After inspection, the behavior in master branch, where _source: { includes: ['geometry', 'prop1']} is not optimal and this PR's behavior is better. It is safe to update the expect statement to _source: { 'geometry'}. This PR does a better job of removing duplicate fields from _source which is great, thanks.

The existing behavior in master branch results in hits like the below. Notice how prop1's value is returned twice, once in _source and once in fields.

{ "_index": "geo_shapes", "_id": "4", "_score": 0, "_source": { "prop1": 4, "geometry": { ...omitted } }, "fields": { "prop1": [ 4 ] } }

With this PR in place, the behavior is optimal and prop1's value is only returned a single time.

{ "_index": "geo_shapes", "_id": "4", "_score": 0, "_source": { "geometry": {...omitted} }, "fields": { "prop1": [ 4 ] } }

majagrubic · 2021-02-08T06:47:31Z

@elasticmachine merge upstream

elasticmachine · 2021-02-08T12:14:34Z

Pinging @elastic/kibana-app (Team:KibanaApp)

majagrubic · 2021-02-10T15:03:22Z

@elasticmachine merge upstream

src/plugins/data/common/search/search_source/search_source.ts

lukeelmers · 2021-02-11T03:39:48Z

src/plugins/data/common/search/search_source/search_source.ts

@@ -539,7 +595,7 @@ export class SearchSource {
      if (!body.hasOwnProperty('_source')) {
        body._source = sourceFilters;
      }
-      if (body._source.excludes) {
+      if (body._source) {


This will always evaluate to true and can probably be removed. (If body._source is undefined, we assign it on L596 above, and index.getSourceFiltering() will always be truthy).

lukeelmers · 2021-02-11T03:44:11Z

src/plugins/data/common/search/search_source/search_source.ts

@@ -614,6 +673,7 @@ export class SearchSource {
        // if items that are in the docvalueFields are provided, we should
        // inject the format from the computed fields if one isn't given
        const docvaluesIndex = keyBy(filteredDocvalueFields, 'field');
+        body.fields = this.getFieldsWithoutSourceFilters(index, body.fields);


I haven't run this, but I'm wondering why the logic for getFieldsWithoutSourceFilters wouldn't be colocated with the other source filtering stuff starting on L599. They are doing very similar things in applying filters to body.fields, and it seems could be combined (unless there's a reason we need the "unfiltered" values between L599 and here?)

[Edit] Apologies, just realized I'm repeating myself and already mentioned it in the thread above. Been a long week! 😬

It's been a long week for all of us, don't worry 😂 Thanks for the comments, will address this soon.

lukeelmers

LGTM, thanks for incorporating feedback & braving the murky waters of search source @majagrubic!

majagrubic · 2021-02-12T16:04:30Z

@elasticmachine merge upstream

ppisljar

LGTM and thanks a lot @majagrubic for making this work!

majagrubic · 2021-02-15T07:28:27Z

@elasticmachine merge upstream

kibanamachine · 2021-02-15T09:37:47Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`data`	614	701	+87

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`data`	236.5KB	236.5KB	+2.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`data`	798.7KB	904.6KB	+105.9KB

History

💚 Build #106376 succeeded 5b94504
💛 Build #106055 was flaky a9c9389
💚 Build #105611 succeeded c1aef7d
💚 Build #105596 succeeded 259cf6b
💚 Build #104584 succeeded 0bca218

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

kertal

Code LGTM 👍 🙇 for this fix. Testing Chrome, Safari, Firefox, I think I've encountered one hopefully final issue:

I've excluded category.keyword, it's excluded in the doc viewer, but selectable in the sidebar

One add-on, I think the intro text when adding the field filter is no longer valid:

…lastic#89837) * [Search Source] Remove includes when retrieving fields from source * Removing unusded imports * Use exclusion filters to retrieve a list of fields * Exclude _source from fields list * Fix small check in getting the field list * Fixing faulty import * Filter logic * Adding a unit test for maps use case * Updating maps unit & functional test * Add unit test * Move logic for requesting a field list inside search_source * Remove unnecessary mock * Code cleanup as per PR comments Co-authored-by: Kibana Machine <[email protected]>

…ndition-for-hiding-recommded-allocation * 'master' of github.com:elastic/kibana: [Discover] Fix toggling multi fields from doc view table (elastic#91121) [ML] Data Frame Analytics: ROC Curve Chart (elastic#89991) skip flaky suite (elastic#86948) skip flaky suite (elastic#91191) Fix date histogram time zone for rollup index (elastic#90632) [Search Source] Fix retrieval of unmapped fields; Add field filters (elastic#89837) [Logs UI] Use useMlHref hook for ML links (elastic#90935) Fix values of `products.min_price` field in Kibana sample ecommerce data set (elastic#90428) [APM] Darker shade for Error group details labels (elastic#91349) [Lens] Adjust new copy for 7.12 (elastic#90413) [ML] Unskip test. Fix modelMemoryLimit value. (elastic#91280) [Lens] Fix empty display name issue in XY chart (elastic#91132) [Lens] Improves error messages when in Dashboard (elastic#90668) [Lens] Keyboard-selected items follow user traversal of drop zones (elastic#90546) [Lens] Improves ranking feature in Top values (elastic#90749) [ILM] Rollover min age tooltip and copy fixes (elastic#91110) # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts

…89837) (#91396) * [Search Source] Remove includes when retrieving fields from source * Removing unusded imports * Use exclusion filters to retrieve a list of fields * Exclude _source from fields list * Fix small check in getting the field list * Fixing faulty import * Filter logic * Adding a unit test for maps use case * Updating maps unit & functional test * Add unit test * Move logic for requesting a field list inside search_source * Remove unnecessary mock * Code cleanup as per PR comments Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

[Search Source] Remove includes when retrieving fields from source

e870118

Merge branch 'master' into inclusion-filters

a691322

Maja Grubic added 4 commits February 1, 2021 13:33

Removing unusded imports

5321c06

Use exclusion filters to retrieve a list of fields

170cb50

Exclude _source from fields list

08a25e6

Merge branch 'master' into inclusion-filters

892d18c

majagrubic changed the title ~~[Search Source] Remove includes when retrieving fields from source~~ [Search Source] Fix retrieval of unmapped fields; Add field filters Feb 3, 2021

Fix small check in getting the field list

0788854

lukeelmers reviewed Feb 3, 2021

View reviewed changes

Maja Grubic added 4 commits February 4, 2021 11:28

Fixing faulty import

736d6a7

Filter logic

0364594

Adding a unit test for maps use case

ff9cd2b

Merge master

2872c36

nreese approved these changes Feb 5, 2021

View reviewed changes

Maja Grubic added 2 commits February 7, 2021 19:58

Updating maps unit & functional test

f7c845e

Add unit test

21b011e

Merge branch 'master' into inclusion-filters

0bca218

majagrubic marked this pull request as ready for review February 8, 2021 12:14

majagrubic requested a review from a team February 8, 2021 12:14

majagrubic requested review from a team as code owners February 8, 2021 12:14

majagrubic added v7.12.0 release_note:roadmap Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Feb 8, 2021

majagrubic requested a review from ppisljar February 8, 2021 12:15

Move logic for requesting a field list inside search_source

259cf6b

majagrubic mentioned this pull request Feb 10, 2021

[Discover] Remaining work on Fields API #88426

Closed

31 tasks

kibanamachine and others added 2 commits February 10, 2021 10:03

Merge branch 'master' into inclusion-filters

4279ef8

Remove unnecessary mock

c1aef7d

majagrubic requested a review from lukeelmers February 10, 2021 17:55

lukeelmers reviewed Feb 11, 2021

View reviewed changes

Code cleanup as per PR comments

a9c9389

majagrubic requested a review from lukeelmers February 11, 2021 16:59

lukeelmers approved these changes Feb 11, 2021

View reviewed changes

Merge branch 'master' into inclusion-filters

5b94504

ppisljar approved these changes Feb 15, 2021

View reviewed changes

Merge branch 'master' into inclusion-filters

01aa170

kertal reviewed Feb 15, 2021

View reviewed changes

majagrubic merged commit 5b5f47f into elastic:master Feb 15, 2021

majagrubic deleted the inclusion-filters branch February 15, 2021 12:53

majagrubic mentioned this pull request Feb 15, 2021

[7.x] [Search Source] Fix retrieval of unmapped fields; Add field filters (#89837) #91396

Merged

sorenlouv mentioned this pull request Feb 15, 2021

[7.x] [Search Source] Fix retrieval of unmapped fields; Add field filters (#89837) #91398

Closed

majagrubic mentioned this pull request Feb 15, 2021

[Discover] Making source filters test run with fields API #91404

Merged

2 tasks

timroes mentioned this pull request Feb 19, 2021

[Discover] Fields filter doesn't work #88193

Closed

timroes added release_note:enhancement Feature:Search Querying infrastructure in Kibana and removed release_note:roadmap labels Mar 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search Source] Fix retrieval of unmapped fields; Add field filters #89837

[Search Source] Fix retrieval of unmapped fields; Add field filters #89837

majagrubic commented Feb 1, 2021 •

edited

Loading

majagrubic commented Feb 1, 2021

kibanamachine commented Feb 1, 2021

lukeelmers Feb 3, 2021

lukeelmers Feb 3, 2021

majagrubic Feb 3, 2021

lukeelmers Feb 3, 2021

lukeelmers Feb 3, 2021

majagrubic Feb 3, 2021

lukeelmers Feb 3, 2021

majagrubic Feb 8, 2021

ppisljar Feb 9, 2021

majagrubic Feb 9, 2021

spalger commented Feb 3, 2021

nreese left a comment

nreese Feb 5, 2021

majagrubic commented Feb 8, 2021

elasticmachine commented Feb 8, 2021

majagrubic commented Feb 10, 2021

lukeelmers Feb 11, 2021

lukeelmers Feb 11, 2021 •

edited

Loading

majagrubic Feb 11, 2021 •

edited

Loading

lukeelmers left a comment

majagrubic commented Feb 12, 2021

ppisljar left a comment

majagrubic commented Feb 15, 2021

kibanamachine commented Feb 15, 2021

kertal left a comment

[Search Source] Fix retrieval of unmapped fields; Add field filters #89837

[Search Source] Fix retrieval of unmapped fields; Add field filters #89837

Conversation

majagrubic commented Feb 1, 2021 • edited Loading

Summary

Checklist

For maintainers

majagrubic commented Feb 1, 2021

kibanamachine commented Feb 1, 2021

⏳ Build in-progress, with failures

Failed CI Steps

History

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Feb 3, 2021

nreese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majagrubic commented Feb 8, 2021

elasticmachine commented Feb 8, 2021

majagrubic commented Feb 10, 2021

Choose a reason for hiding this comment

lukeelmers Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

majagrubic Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

majagrubic commented Feb 12, 2021

ppisljar left a comment

Choose a reason for hiding this comment

majagrubic commented Feb 15, 2021

kibanamachine commented Feb 15, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

kertal left a comment

Choose a reason for hiding this comment

majagrubic commented Feb 1, 2021 •

edited

Loading

lukeelmers Feb 11, 2021 •

edited

Loading

majagrubic Feb 11, 2021 •

edited

Loading