[Discover] Show correct data for top level object columns #91954

wylieconlon · 2021-02-18T23:01:50Z

This can be tested with the kibana_sample_data_ecommerce data set. I recommend adding a KQL search for products.product_name: Boots to test the highlighting feature.

The easiest way to test is to modify the URL by looking for columns:!() or columns:!(_source) and replace this with columns:!(products). Then save the search. This is because on master, it's not possible to select the products column from the list of fields: this is working as expected, but users may have previously saved searches on top level objects.

Before this, we are trying to basically improve on the behavior from 7.11:

Here are the scenarios that I have tested:

// closest to 7.11
discover:searchFieldsFromSource: true
doc_table:legacy: true

// potential default for 7.12
discover:searchFieldsFromSource: true
doc_table:legacy: true

// with data grid but legacy fetching
discover:searchFieldsFromSource: false
doc_table:legacy: false

// with data grid and new data
discover:searchFieldsFromSource: false
doc_table:legacy: false

cc @mattkime because I've implemented some logic custom to Discover that is maybe better suited to the index pattern API

Closes #91953

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2021-02-18T23:14:44Z

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

src/plugins/discover/public/application/components/discover_grid/get_render_cell_value.tsx

flash1293

Breaks properties of enabled: false object fields (shown as empty)
Breaks properties of enabled: false object fields, enabled: false object root fields and nested fields for data grid (all shown as empty - these work fine in the legacy table)

flash1293 · 2021-02-19T14:57:07Z

src/plugins/discover/public/application/components/discover_grid/get_render_cell_value.tsx

+    const highlightPairs: Array<[string, string]> = [];
+    const sourcePairs: Array<[string, string]> = [];
+    Object.entries(innerColumns).forEach(([key, values]) => {
+      const subField = indexPattern.getFieldByName(key)!;


This subField can be undefined if the object is enabled: false, crashing discover.

…fields

wylieconlon · 2021-02-19T18:04:49Z

@flash1293 fixed and added unit tests. Here are the two scenarios when dealing with unmapped fields:

kertal · 2021-02-22T08:36:08Z

@elasticmachine merge upstream

flash1293

Tried around and wasn't able to break it, LGTM

We can think about formatting nested objects the same way (with highlighted keys), but it's probably not that easy to achieve. Definitely not necessary:

kertal · 2021-02-22T11:17:02Z

src/plugins/discover/public/application/components/discover_grid/get_render_cell_value.tsx

+      const subField = indexPattern.getFieldByName(key);
+      const formatter = subField
+        ? indexPattern.getFormatterForField(subField)
+        : { convert: (v: string, ...rest: unknown[]) => v.toString() };


Even if I can't think of a situation where null comes around, String(v) would be prepared for this impossible event.

kertal

Great work dear @wylieconlon , thanks a lot for fixing highlighting for the data grid as a side effect. Tested locally in Chrome, Firefox, Safari, MacOs ... works as expected

…fields

wylieconlon · 2021-02-22T17:20:25Z

Thanks @kertal updated from your suggestion. @flash1293 I opened an issue for that: #92223

kibanamachine · 2021-02-22T19:23:02Z

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id	before	after	diff
`discover`	397.3KB	400.2KB	+3.0KB

History

💛 Build #108272 was flaky 4fb232a
💚 Build #108117 succeeded 10cef85
💚 Build #107871 succeeded 1df3b9d

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

) * [Discover] Show correct data for top level object columns * Fix bug with missing fields * Fix bug in data grid * Fix remaining bug in datagrid * Change use of API to work with any type Co-authored-by: Kibana Machine <[email protected]>

…92268) * [Discover] Show correct data for top level object columns * Fix bug with missing fields * Fix bug in data grid * Fix remaining bug in datagrid * Change use of API to work with any type Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

…92269) * [Discover] Show correct data for top level object columns * Fix bug with missing fields * Fix bug in data grid * Fix remaining bug in datagrid * Change use of API to work with any type Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

[Discover] Show correct data for top level object columns

1df3b9d

wylieconlon added Feature:Discover Discover Application v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v7.13.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Feb 18, 2021

wylieconlon requested review from kertal, majagrubic and mattkime February 18, 2021 23:13

wylieconlon marked this pull request as ready for review February 18, 2021 23:14

wylieconlon requested a review from a team February 18, 2021 23:14

wylieconlon requested a review from timroes February 18, 2021 23:15

wylieconlon commented Feb 18, 2021

View reviewed changes

src/plugins/discover/public/application/components/discover_grid/get_render_cell_value.tsx Show resolved Hide resolved

flash1293 mentioned this pull request Feb 19, 2021

[Discover] Remaining work on Fields API #88426

Closed

31 tasks

flash1293 requested changes Feb 19, 2021

View reviewed changes

wylieconlon added 3 commits February 19, 2021 11:52

Merge remote-tracking branch 'origin/master' into discover-top-level-…

ce3a8e6

…fields

Fix bug with missing fields

a9b4ce9

Fix bug in data grid

b0e228c

Fix remaining bug in datagrid

10cef85

wylieconlon requested a review from flash1293 February 19, 2021 19:08

Merge branch 'master' into discover-top-level-fields

4fb232a

flash1293 approved these changes Feb 22, 2021

View reviewed changes

kertal reviewed Feb 22, 2021

View reviewed changes

kertal approved these changes Feb 22, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into discover-top-level-…

8a1b786

…fields

Change use of API to work with any type

ddafc28

wylieconlon merged commit 4614202 into elastic:master Feb 22, 2021

wylieconlon deleted the discover-top-level-fields branch February 22, 2021 19:24

This was referenced Feb 22, 2021

[7.x] [Discover] Show correct data for top level object columns (#91954) #92268

Merged

[7.12] [Discover] Show correct data for top level object columns (#91954) #92269

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Show correct data for top level object columns #91954

[Discover] Show correct data for top level object columns #91954

wylieconlon commented Feb 18, 2021 •

edited

Loading

elasticmachine commented Feb 18, 2021

flash1293 left a comment

flash1293 Feb 19, 2021

wylieconlon commented Feb 19, 2021 •

edited

Loading

kertal commented Feb 22, 2021

flash1293 left a comment

kertal Feb 22, 2021

kertal left a comment

wylieconlon commented Feb 22, 2021

kibanamachine commented Feb 22, 2021

[Discover] Show correct data for top level object columns #91954

[Discover] Show correct data for top level object columns #91954

Conversation

wylieconlon commented Feb 18, 2021 • edited Loading

Checklist

elasticmachine commented Feb 18, 2021

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Feb 19, 2021

Choose a reason for hiding this comment

wylieconlon commented Feb 19, 2021 • edited Loading

kertal commented Feb 22, 2021

flash1293 left a comment

Choose a reason for hiding this comment

kertal Feb 22, 2021

Choose a reason for hiding this comment

kertal left a comment

Choose a reason for hiding this comment

wylieconlon commented Feb 22, 2021

kibanamachine commented Feb 22, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

wylieconlon commented Feb 18, 2021 •

edited

Loading

wylieconlon commented Feb 19, 2021 •

edited

Loading