[Discover] Optimize checking for multifields during grid rendering (#…

…145698) A follow up for #144672 (issue #144673) ## Summary This PR reduces number of iterations through fields array by simplifying the logic and switching to `Map` and `.get()` instead of using `.includes()` on a visible fields array for each rendered field. Also `shouldShowFieldHandler` can be further improved in the future without the need to refactor how it's being used. Before: <img width="1490" alt="Screenshot 2022-11-21 at 19 36 17" src="https://user-images.githubusercontent.com/1415710/203134749-190df445-f0dc-4a66-8797-21fedc1345d2.png"> <img width="430" alt="Screenshot 2022-11-21 at 19 36 43" src="https://user-images.githubusercontent.com/1415710/203134792-06b659cd-b4e0-4248-b1b2-63767528738b.png"> <img width="406" alt="Screenshot 2022-11-21 at 19 39 05" src="https://user-images.githubusercontent.com/1415710/203134836-2881e4bf-a9be-4a12-bcd5-76074d3ae0d3.png"> After: <img width="1493" alt="Screenshot 2022-11-21 at 19 42 48" src="https://user-images.githubusercontent.com/1415710/203134864-9d686705-891f-4640-96e6-29e7af5ae9d9.png"> <img width="496" alt="Screenshot 2022-11-21 at 19 44 54" src="https://user-images.githubusercontent.com/1415710/203135114-fda20375-fd3f-4ab4-b3e5-cdf7758ca3c8.png"> <img width="370" alt="Screenshot 2022-11-21 at 19 44 32" src="https://user-images.githubusercontent.com/1415710/203135038-c4fd40b7-1c8a-43a3-b08c-c4c19c74743f.png">
elastic · Nov 22, 2022 · 05dbc91 · 05dbc91
1 parent 62a90f8
commit 05dbc91
Show file tree

Hide file tree

Showing 14 changed files with 149 additions and 108 deletions.
diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid.tsx
@@ -45,7 +45,7 @@ import {
   SHOW_MULTIFIELDS,
 } from '../../../common';
 import { DiscoverGridDocumentToolbarBtn } from './discover_grid_document_selection';
-import { getFieldsToShow } from '../../utils/get_fields_to_show';
+import { getShouldShowFieldHandler } from '../../utils/get_should_show_field_handler';
 import type { DataTableRecord, ValueToStringConverter } from '../../types';
 import { useRowHeightsOptions } from '../../hooks/use_row_heights_options';
 import { useDiscoverServices } from '../../hooks/use_discover_services';
@@ -341,9 +341,9 @@ export const DiscoverGrid = ({
 
   const showMultiFields = services.uiSettings.get(SHOW_MULTIFIELDS);
 
-  const fieldsToShow = useMemo(() => {
+  const shouldShowFieldHandler = useMemo(() => {
     const dataViewFields = dataView.fields.getAll().map((fld) => fld.name);
-    return getFieldsToShow(dataViewFields, dataView, showMultiFields);
+    return getShouldShowFieldHandler(dataViewFields, dataView, showMultiFields);
   }, [dataView, showMultiFields]);
 
   /**
@@ -355,11 +355,11 @@ export const DiscoverGrid = ({
         dataView,
         displayedRows,
         useNewFieldsApi,
-        fieldsToShow,
+        shouldShowFieldHandler,
         services.uiSettings.get(MAX_DOC_FIELDS_DISPLAYED),
         () => dataGridRef.current?.closeCellPopover()
       ),
-    [dataView, displayedRows, useNewFieldsApi, fieldsToShow, services.uiSettings]
+    [dataView, displayedRows, useNewFieldsApi, shouldShowFieldHandler, services.uiSettings]
   );
 
   /**

diff --git a/src/plugins/discover/public/components/discover_grid/get_render_cell_value.test.tsx b/src/plugins/discover/public/components/discover_grid/get_render_cell_value.test.tsx
@@ -79,7 +79,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsSource.map(build),
       false,
-      [],
+      () => false,
       100,
       jest.fn()
     );
@@ -104,7 +104,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsSource.map(build),
       false,
-      [],
+      () => false,
       100,
       jest.fn()
     );
@@ -130,7 +130,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsFields.map(build),
       false,
-      [],
+      () => false,
       100,
       closePopoverMockFn
     );
@@ -157,7 +157,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsSource.map(build),
       false,
-      ['extension', 'bytes'],
+      (fieldName) => ['extension', 'bytes'].includes(fieldName),
       100,
       jest.fn()
     );
@@ -231,7 +231,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsSource.map(build),
       false,
-      [],
+      () => false,
       100,
       jest.fn()
     );
@@ -304,7 +304,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsFields.map(build),
       true,
-      ['extension', 'bytes'],
+      (fieldName) => ['extension', 'bytes'].includes(fieldName),
       100,
       jest.fn()
     );
@@ -382,7 +382,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsFields.map(build),
       true,
-      ['extension', 'bytes'],
+      (fieldName) => ['extension', 'bytes'].includes(fieldName),
       // this is the number of rendered items
       1,
       jest.fn()
@@ -461,7 +461,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsFields.map(build),
       true,
-      [],
+      (fieldName) => false,
       100,
       jest.fn()
     );
@@ -539,7 +539,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsFieldsWithTopLevelObject.map(build),
       true,
-      ['object.value', 'extension', 'bytes'],
+      (fieldName) => ['object.value', 'extension', 'bytes'].includes(fieldName),
       100,
       jest.fn()
     );
@@ -581,7 +581,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsFieldsWithTopLevelObject.map(build),
       true,
-      ['extension', 'bytes', 'object.value'],
+      (fieldName) => ['extension', 'bytes', 'object.value'].includes(fieldName),
       100,
       jest.fn()
     );
@@ -623,7 +623,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsFieldsWithTopLevelObject.map(build),
       true,
-      [],
+      () => false,
       100,
       closePopoverMockFn
     );
@@ -688,7 +688,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsFieldsWithTopLevelObject.map(build),
       true,
-      [],
+      () => false,
       100,
       closePopoverMockFn
     );
@@ -716,7 +716,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsFieldsWithTopLevelObject.map(build),
       true,
-      [],
+      () => false,
       100,
       jest.fn()
     );
@@ -750,7 +750,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsSource.map(build),
       false,
-      [],
+      () => false,
       100,
       jest.fn()
     );
@@ -775,7 +775,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsSource.map(build),
       false,
-      [],
+      () => false,
       100,
       jest.fn()
     );
@@ -813,7 +813,7 @@ describe('Discover grid cell rendering', function () {
       dataViewMock,
       rowsFieldsUnmapped.map(build),
       true,
-      ['unmapped'],
+      (fieldName) => ['unmapped'].includes(fieldName),
       100,
       jest.fn()
     );

diff --git a/src/plugins/discover/public/components/discover_grid/get_render_cell_value.tsx b/src/plugins/discover/public/components/discover_grid/get_render_cell_value.tsx
@@ -29,6 +29,7 @@ import { formatHit } from '../../utils/format_hit';
 import { DataTableRecord, EsHitRecord } from '../../types';
 import { useDiscoverServices } from '../../hooks/use_discover_services';
 import { MAX_DOC_FIELDS_DISPLAYED } from '../../../common';
+import { type ShouldShowFieldInTableHandler } from '../../utils/get_should_show_field_handler';
 
 const CELL_CLASS = 'dscDiscoverGrid__cellValue';
 
@@ -37,7 +38,7 @@ export const getRenderCellValueFn =
     dataView: DataView,
     rows: DataTableRecord[] | undefined,
     useNewFieldsApi: boolean,
-    fieldsToShow: string[],
+    shouldShowFieldHandler: ShouldShowFieldInTableHandler,
     maxDocFieldsDisplayed: number,
     closePopover: () => void
   ) =>
@@ -98,11 +99,11 @@ export const getRenderCellValueFn =
 
     if (field?.type === '_source' || useTopLevelObjectColumns) {
       const pairs = useTopLevelObjectColumns
-        ? getTopLevelObjectPairs(row.raw, columnId, dataView, fieldsToShow).slice(
+        ? getTopLevelObjectPairs(row.raw, columnId, dataView, shouldShowFieldHandler).slice(
             0,
             maxDocFieldsDisplayed
           )
-        : formatHit(row, dataView, fieldsToShow, maxEntries, fieldFormats);
+        : formatHit(row, dataView, shouldShowFieldHandler, maxEntries, fieldFormats);
 
       return (
         <EuiDescriptionList
@@ -235,7 +236,7 @@ function getTopLevelObjectPairs(
   row: EsHitRecord,
   columnId: string,
   dataView: DataView,
-  fieldsToShow: string[]
+  shouldShowFieldHandler: ShouldShowFieldInTableHandler
 ) {
   const innerColumns = getInnerColumns(row.fields as Record<string, unknown[]>, columnId);
   // Put the most important fields first
@@ -260,7 +261,7 @@ function getTopLevelObjectPairs(
       .join(', ');
     const pairs = highlights[key] ? highlightPairs : sourcePairs;
     if (displayKey) {
-      if (fieldsToShow.includes(displayKey)) {
+      if (shouldShowFieldHandler(displayKey)) {
         pairs.push([displayKey, formatted]);
       }
     } else {

diff --git a/src/plugins/discover/public/components/doc_table/components/table_row.tsx b/src/plugins/discover/public/components/doc_table/components/table_row.tsx
@@ -21,6 +21,7 @@ import { DataTableRecord, EsHitRecord } from '../../../types';
 import { TableRowDetails } from './table_row_details';
 import { useDiscoverServices } from '../../../hooks/use_discover_services';
 import { DOC_HIDE_TIME_COLUMN_SETTING, MAX_DOC_FIELDS_DISPLAYED } from '../../../../common';
+import { type ShouldShowFieldInTableHandler } from '../../../utils/get_should_show_field_handler';
 
 export type DocTableRow = EsHitRecord & {
   isAnchor?: boolean;
@@ -34,7 +35,7 @@ export interface TableRowProps {
   row: DataTableRecord;
   dataView: DataView;
   useNewFieldsApi: boolean;
-  fieldsToShow: string[];
+  shouldShowFieldHandler: ShouldShowFieldInTableHandler;
   onAddColumn?: (column: string) => void;
   onRemoveColumn?: (column: string) => void;
 }
@@ -47,7 +48,7 @@ export const TableRow = ({
   row,
   dataView,
   useNewFieldsApi,
-  fieldsToShow,
+  shouldShowFieldHandler,
   onAddColumn,
   onRemoveColumn,
 }: TableRowProps) => {
@@ -77,7 +78,7 @@ export const TableRow = ({
     // If we're formatting the _source column, don't use the regular field formatter,
     // but our Discover mechanism to format a hit in a better human-readable way.
     if (fieldName === '_source') {
-      return formatRow(row, dataView, fieldsToShow, maxEntries, fieldFormats);
+      return formatRow(row, dataView, shouldShowFieldHandler, maxEntries, fieldFormats);
     }
 
     const formattedField = formatFieldValue(
@@ -136,7 +137,7 @@ export const TableRow = ({
   }
 
   if (columns.length === 0 && useNewFieldsApi) {
-    const formatted = formatRow(row, dataView, fieldsToShow, maxEntries, fieldFormats);
+    const formatted = formatRow(row, dataView, shouldShowFieldHandler, maxEntries, fieldFormats);
 
     rowCells.push(
       <TableCell

diff --git a/src/plugins/discover/public/components/doc_table/doc_table_wrapper.tsx b/src/plugins/discover/public/components/doc_table/doc_table_wrapper.tsx
@@ -16,7 +16,7 @@ import { TableHeader } from './components/table_header/table_header';
 import { SHOW_MULTIFIELDS } from '../../../common';
 import { TableRow } from './components/table_row';
 import { DocViewFilterFn } from '../../services/doc_views/doc_views_types';
-import { getFieldsToShow } from '../../utils/get_fields_to_show';
+import { getShouldShowFieldHandler } from '../../utils/get_should_show_field_handler';
 import { useDiscoverServices } from '../../hooks/use_discover_services';
 import type { DataTableRecord } from '../../types';
 
@@ -142,9 +142,9 @@ export const DocTableWrapper = forwardRef(
       bottomMarker!.blur();
     }, [rows]);
 
-    const fieldsToShow = useMemo(
+    const shouldShowFieldHandler = useMemo(
       () =>
-        getFieldsToShow(
+        getShouldShowFieldHandler(
           dataView.fields.map((field: DataViewField) => field.name),
           dataView,
           showMultiFields
@@ -178,7 +178,7 @@ export const DocTableWrapper = forwardRef(
             dataView={dataView}
             row={current}
             useNewFieldsApi={useNewFieldsApi}
-            fieldsToShow={fieldsToShow}
+            shouldShowFieldHandler={shouldShowFieldHandler}
             onAddColumn={onAddColumn}
             onRemoveColumn={onRemoveColumn}
           />
@@ -191,7 +191,7 @@ export const DocTableWrapper = forwardRef(
         onFilter,
         dataView,
         useNewFieldsApi,
-        fieldsToShow,
+        shouldShowFieldHandler,
         onAddColumn,
         onRemoveColumn,
       ]

diff --git a/src/plugins/discover/public/components/doc_table/utils/row_formatter.test.ts b/src/plugins/discover/public/components/doc_table/utils/row_formatter.test.ts
@@ -47,7 +47,8 @@ describe('Row formatter', () => {
   };
   const hit = buildDataTableRecord(rawHit, dataView);
 
-  const fieldsToShow = dataView.fields.getAll().map((fld) => fld.name);
+  const shouldShowField = (fieldName: string) =>
+    dataView.fields.getAll().some((fld) => fld.name === fieldName);
 
   beforeEach(() => {
     services = {
@@ -59,7 +60,7 @@ describe('Row formatter', () => {
   });
 
   it('formats document properly', () => {
-    expect(formatRow(hit, dataView, fieldsToShow, 100, services.fieldFormats))
+    expect(formatRow(hit, dataView, shouldShowField, 100, services.fieldFormats))
       .toMatchInlineSnapshot(`
       <TemplateComponent
         defPairs={
@@ -104,7 +105,7 @@ describe('Row formatter', () => {
         getFormatterForField: jest.fn(() => ({ convert: (value: unknown) => value })),
       },
     } as unknown as DiscoverServices;
-    expect(formatRow(hit, dataView, [], 1, services.fieldFormats)).toMatchInlineSnapshot(`
+    expect(formatRow(hit, dataView, () => false, 1, services.fieldFormats)).toMatchInlineSnapshot(`
       <TemplateComponent
         defPairs={
           Array [
@@ -144,7 +145,7 @@ describe('Row formatter', () => {
       dataView
     );
 
-    expect(formatRow(highLightHit, dataView, fieldsToShow, 100, services.fieldFormats))
+    expect(formatRow(highLightHit, dataView, shouldShowField, 100, services.fieldFormats))
       .toMatchInlineSnapshot(`
       <TemplateComponent
         defPairs={

diff --git a/src/plugins/discover/public/components/doc_table/utils/row_formatter.tsx b/src/plugins/discover/public/components/doc_table/utils/row_formatter.tsx
@@ -13,6 +13,7 @@ import { formatHit } from '../../../utils/format_hit';
 import type { DataTableRecord } from '../../../types';
 
 import './row_formatter.scss';
+import { type ShouldShowFieldInTableHandler } from '../../../utils/get_should_show_field_handler';
 
 interface Props {
   defPairs: Array<readonly [string, string]>;
@@ -40,11 +41,11 @@ const TemplateComponent = ({ defPairs }: Props) => {
 export const formatRow = (
   hit: DataTableRecord,
   dataView: DataView,
-  fieldsToShow: string[],
+  shouldShowFieldHandler: ShouldShowFieldInTableHandler,
   maxEntries: number,
   fieldFormats: FieldFormatsStart
 ) => {
-  const pairs = formatHit(hit, dataView, fieldsToShow, maxEntries, fieldFormats);
+  const pairs = formatHit(hit, dataView, shouldShowFieldHandler, maxEntries, fieldFormats);
   return <TemplateComponent defPairs={pairs} />;
 };
 

diff --git a/src/plugins/discover/public/services/doc_views/components/doc_viewer_table/legacy/table.tsx b/src/plugins/discover/public/services/doc_views/components/doc_viewer_table/legacy/table.tsx
@@ -14,7 +14,7 @@ import { useDiscoverServices } from '../../../../../hooks/use_discover_services'
 import { SHOW_MULTIFIELDS } from '../../../../../../common';
 import { DocViewRenderProps, FieldRecordLegacy } from '../../../doc_views_types';
 import { ACTIONS_COLUMN, MAIN_COLUMNS } from './table_columns';
-import { getFieldsToShow } from '../../../../../utils/get_fields_to_show';
+import { getShouldShowFieldHandler } from '../../../../../utils/get_should_show_field_handler';
 import { getIgnoredReason } from '../../../../../utils/get_ignored_reason';
 import { formatFieldValue } from '../../../../../utils/format_value';
 import { isNestedFieldParent } from '../../../../../application/main/utils/nested_fields';
@@ -56,12 +56,13 @@ export const DocViewerLegacyTable = ({
     };
   }, []);
 
-  const fieldsToShow = getFieldsToShow(Object.keys(hit.flattened), dataView, showMultiFields);
+  const shouldShowFieldHandler = useMemo(
+    () => getShouldShowFieldHandler(Object.keys(hit.flattened), dataView, showMultiFields),
+    [hit.flattened, dataView, showMultiFields]
+  );
 
   const items: FieldRecordLegacy[] = Object.keys(hit.flattened)
-    .filter((fieldName) => {
-      return fieldsToShow.includes(fieldName);
-    })
+    .filter(shouldShowFieldHandler)
     .sort((fieldA, fieldB) => {
       const mappingA = mapping(fieldA);
       const mappingB = mapping(fieldB);