Skip to content

Commit

Permalink
[Discover] Rely on dataSource for ES|QL mode (#183108)
Browse files Browse the repository at this point in the history
## Summary

This PR updates Discover to rely on `dataSource` for determining if
ES|QL mode is enabled instead of relying on `query` directly. This
creates a clearer separation between the modes and moves Discover toward
supporting multiple data source types. It also includes a number of
cleanups around ES|QL mode that make the intent of the code clearer and
removes tech debt / code duplication.

Changes included in the PR:
- Add a new `useIsEsqlMode` hook that relies on `dataSource` for
checking if Discover is in ES|QL mode.
- Remove "raw record type" concept in Discover and replace it with ES|QL
`dataSource` checks.
- Remove references to `isPlainRecord` and replace them with ES|QL
`dataSource` checks.
- Remove `isTextBasedQuery` utility function and replace it with ES|QL
`dataSource` checks or `isOfAggregateQueryType` where casting is
necessary.
- Replace references to `isOfAggregateQueryType` with ES|QL `dataSource`
checks except where casting is necessary.
- Replace other references to "text based" with "ES|QL" for clarity and
consistency.

Closes #181963.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
  • Loading branch information
davismcphee authored May 18, 2024
1 parent e2ccaf2 commit 808a89c
Show file tree
Hide file tree
Showing 91 changed files with 667 additions and 899 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ import { DOC_TABLE_LEGACY } from '../constants';

export function isLegacyTableEnabled({
uiSettings,
isTextBasedQueryMode,
isEsqlMode,
}: {
uiSettings: IUiSettingsClient;
isTextBasedQueryMode: boolean;
isEsqlMode: boolean;
}): boolean {
if (isTextBasedQueryMode) {
if (isEsqlMode) {
return false; // only show the new data grid
}

Expand Down
8 changes: 4 additions & 4 deletions src/plugins/discover/common/utils/sorting/get_default_sort.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,21 @@ import { isSortable } from './get_sort';

/**
* use in case the user didn't manually sort.
* the default sort is returned depending on the data view or non for text based queries
* the default sort is returned depending on the data view or non for ES|QL queries
*/
export function getDefaultSort(
dataView: DataView | undefined,
defaultSortOrder: string = 'desc',
hidingTimeColumn: boolean = false,
isTextBasedQueryMode: boolean
isEsqlMode: boolean
): SortOrder[] {
if (isTextBasedQueryMode) {
if (isEsqlMode) {
return [];
}

if (
dataView?.timeFieldName &&
isSortable(dataView.timeFieldName, dataView, isTextBasedQueryMode) &&
isSortable(dataView.timeFieldName, dataView, isEsqlMode) &&
!hidingTimeColumn
) {
return [[dataView.timeFieldName, defaultSortOrder]];
Expand Down
28 changes: 12 additions & 16 deletions src/plugins/discover/common/utils/sorting/get_sort.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,10 @@ export type SortPairObj = Record<string, string>;
export type SortPair = SortOrder | SortPairObj;
export type SortInput = SortPair | SortPair[];

export function isSortable(
fieldName: string,
dataView: DataView,
isTextBasedQueryMode: boolean
): boolean {
if (isTextBasedQueryMode) {
// in-memory sorting is used for text-based queries
// would be great to have a way to determine if a text-based column is sortable
export function isSortable(fieldName: string, dataView: DataView, isEsqlMode: boolean): boolean {
if (isEsqlMode) {
// in-memory sorting is used for ES|QL queries
// would be great to have a way to determine if a ES|QL column is sortable
return fieldName !== '_source';
}
const field = dataView.getFieldByName(fieldName);
Expand All @@ -31,18 +27,18 @@ export function isSortable(
function createSortObject(
sortPair: SortInput,
dataView: DataView,
isTextBasedQueryMode: boolean
isEsqlMode: boolean
): SortPairObj | undefined {
if (
Array.isArray(sortPair) &&
sortPair.length === 2 &&
isSortable(String(sortPair[0]), dataView, isTextBasedQueryMode)
isSortable(String(sortPair[0]), dataView, isEsqlMode)
) {
const [field, direction] = sortPair as SortOrder;
return { [field]: direction };
} else if (
isPlainObject(sortPair) &&
isSortable(Object.keys(sortPair)[0], dataView, isTextBasedQueryMode)
isSortable(Object.keys(sortPair)[0], dataView, isEsqlMode)
) {
return sortPair as SortPairObj;
}
Expand All @@ -59,21 +55,21 @@ export function isLegacySort(sort: SortPair[] | SortPair): sort is SortPair {
* @param {array} sort two dimensional array [[fieldToSort, directionToSort]]
* or an array of objects [{fieldToSort: directionToSort}]
* @param {object} dataView used for determining default sort
* @param {boolean} isTextBasedQueryMode
* @param {boolean} isEsqlMode
* @returns Array<{object}> an array of sort objects
*/
export function getSort(
sort: SortPair[] | SortPair,
dataView: DataView,
isTextBasedQueryMode: boolean
isEsqlMode: boolean
): SortPairObj[] {
if (Array.isArray(sort)) {
if (isLegacySort(sort)) {
// To stay compatible with legacy sort, which just supported a single sort field
return [{ [sort[0]]: sort[1] }];
}
return sort
.map((sortPair: SortPair) => createSortObject(sortPair, dataView, isTextBasedQueryMode))
.map((sortPair: SortPair) => createSortObject(sortPair, dataView, isEsqlMode))
.filter((sortPairObj) => typeof sortPairObj === 'object') as SortPairObj[];
}
return [];
Expand All @@ -86,9 +82,9 @@ export function getSort(
export function getSortArray(
sort: SortInput,
dataView: DataView,
isTextBasedQueryMode: boolean
isEsqlMode: boolean
): SortOrder[] {
return getSort(sort, dataView, isTextBasedQueryMode).reduce((acc: SortOrder[], sortPair) => {
return getSort(sort, dataView, isEsqlMode).reduce((acc: SortOrder[], sortPair) => {
const entries = Object.entries(sortPair);
if (entries && entries[0]) {
acc.push(entries[0]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export function getSortForSearchSource({
}

const { timeFieldName } = dataView;
const sortPairs = getSort(sort, dataView, false); // text based request is not using search source
const sortPairs = getSort(sort, dataView, false); // ES|QL request is not using search source

const sortForSearchSource = sortPairs.map((sortPair: Record<string, string>) => {
if (timeFieldName && sortPair[timeFieldName]) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import { LocalStorageMock } from '../../../../__mocks__/local_storage_mock';
import { DiscoverServices } from '../../../../build_services';
import { discoverServiceMock } from '../../../../__mocks__/services';
import { DiscoverTourProvider } from '../../../../components/discover_tour';
import { getDiscoverStateMock } from '../../../../__mocks__/discover_state.mock';
import { DiscoverMainProvider } from '../../state_management/discover_state_provider';

const defaultServices = {
...discoverServiceMock,
Expand Down Expand Up @@ -62,9 +64,11 @@ describe('Document Explorer Update callout', () => {
it('should start a tour when the button is clicked', () => {
const result = mountWithIntl(
<KibanaContextProvider services={defaultServices}>
<DiscoverTourProvider isPlainRecord={false}>
<DocumentExplorerUpdateCallout />
</DiscoverTourProvider>
<DiscoverMainProvider value={getDiscoverStateMock({})}>
<DiscoverTourProvider>
<DocumentExplorerUpdateCallout />
</DiscoverTourProvider>
</DiscoverMainProvider>
</KibanaContextProvider>
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ import { useInternalStateSelector } from '../../state_management/discover_intern
import { useAppStateSelector } from '../../state_management/discover_app_state_container';
import { useDiscoverServices } from '../../../../hooks/use_discover_services';
import { FetchStatus } from '../../../types';
import { RecordRawType } from '../../state_management/discover_data_state_container';
import { DiscoverStateContainer } from '../../state_management/discover_state';
import { useDataState } from '../../hooks/use_data_state';
import { DocTableInfinite } from '../../../../components/doc_table/doc_table_infinite';
Expand All @@ -56,7 +55,6 @@ import {
DISCOVER_TOUR_STEP_ANCHOR_IDS,
DiscoverTourProvider,
} from '../../../../components/discover_tour';
import { getRawRecordType } from '../../utils/get_raw_record_type';
import {
getMaxAllowedSampleSize,
getAllowedSampleSize,
Expand All @@ -68,6 +66,7 @@ import { SelectedVSAvailableCallout } from './selected_vs_available_callout';
import { useDiscoverCustomization } from '../../../../customizations';
import { onResizeGridColumn } from '../../../../utils/on_resize_grid_column';
import { useContextualGridCustomisations } from '../../hooks/grid_customisations';
import { useIsEsqlMode } from '../../hooks/use_is_esql_mode';

const containerStyles = css`
position: relative;
Expand Down Expand Up @@ -123,20 +122,20 @@ function DiscoverDocumentsComponent({
];
});
const expandedDoc = useInternalStateSelector((state) => state.expandedDoc);
const isTextBasedQuery = useMemo(() => getRawRecordType(query) === RecordRawType.PLAIN, [query]);
const isEsqlMode = useIsEsqlMode();
const useNewFieldsApi = useMemo(() => !uiSettings.get(SEARCH_FIELDS_FROM_SOURCE), [uiSettings]);
const hideAnnouncements = useMemo(() => uiSettings.get(HIDE_ANNOUNCEMENTS), [uiSettings]);
const isLegacy = useMemo(
() => isLegacyTableEnabled({ uiSettings, isTextBasedQueryMode: isTextBasedQuery }),
[uiSettings, isTextBasedQuery]
() => isLegacyTableEnabled({ uiSettings, isEsqlMode }),
[uiSettings, isEsqlMode]
);
const documentState = useDataState(documents$);
const isDataLoading =
documentState.fetchStatus === FetchStatus.LOADING ||
documentState.fetchStatus === FetchStatus.PARTIAL;

// This is needed to prevent EuiDataGrid pushing onSort because the data view has been switched.
// It's just necessary for non-text-based query lang requests since they don't have a partial result state, that's
// It's just necessary for non ES|QL requests since they don't have a partial result state, that's
// considered as loading state in the Component.
// 1. When switching the data view, the sorting in the URL is reset to the default sorting of the selected data view.
// 2. The new sort param is already available in this component and propagated to the EuiDataGrid.
Expand All @@ -145,13 +144,12 @@ function DiscoverDocumentsComponent({
// 5. this is propagated to Discover's URL and causes an unwanted change of state to an unsorted state
// This solution switches to the loading state in this component when the URL index doesn't match the dataView.id
const isDataViewLoading =
useInternalStateSelector((state) => state.isDataViewLoading) && !isTextBasedQuery;
useInternalStateSelector((state) => state.isDataViewLoading) && !isEsqlMode;
const isEmptyDataResult =
isTextBasedQuery || !documentState.result || documentState.result.length === 0;
isEsqlMode || !documentState.result || documentState.result.length === 0;
const rows = useMemo(() => documentState.result || [], [documentState.result]);

const { isMoreDataLoading, totalHits, onFetchMoreRecords } = useFetchMoreRecords({
isTextBasedQuery,
stateContainer,
});

Expand Down Expand Up @@ -227,10 +225,10 @@ function DiscoverDocumentsComponent({

const columnsMeta: DataTableColumnsMeta | undefined = useMemo(
() =>
documentState.textBasedQueryColumns
? getTextBasedColumnsMeta(documentState.textBasedQueryColumns)
documentState.esqlQueryColumns
? getTextBasedColumnsMeta(documentState.esqlQueryColumns)
: undefined,
[documentState.textBasedQueryColumns]
[documentState.esqlQueryColumns]
);

const renderDocumentView = useCallback(
Expand Down Expand Up @@ -269,32 +267,26 @@ function DiscoverDocumentsComponent({
() => (
<>
<SelectedVSAvailableCallout
isPlainRecord={isTextBasedQuery}
textBasedQueryColumns={documents?.textBasedQueryColumns}
esqlQueryColumns={documents?.esqlQueryColumns}
selectedColumns={currentColumns}
/>
<SearchResponseWarningsCallout warnings={documentState.interceptedWarnings ?? []} />
</>
),
[
isTextBasedQuery,
currentColumns,
documents?.textBasedQueryColumns,
documentState.interceptedWarnings,
]
[currentColumns, documents?.esqlQueryColumns, documentState.interceptedWarnings]
);

const gridAnnouncementCallout = useMemo(() => {
if (hideAnnouncements || isLegacy) {
return null;
}

return !isTextBasedQuery ? (
<DiscoverTourProvider isPlainRecord={isTextBasedQuery}>
return !isEsqlMode ? (
<DiscoverTourProvider>
<DocumentExplorerUpdateCallout />
</DiscoverTourProvider>
) : null;
}, [hideAnnouncements, isLegacy, isTextBasedQuery]);
}, [hideAnnouncements, isLegacy, isEsqlMode]);

const loadingIndicator = useMemo(
() =>
Expand Down Expand Up @@ -364,12 +356,12 @@ function DiscoverDocumentsComponent({
isLoading={isDataLoading}
searchDescription={savedSearch.description}
sharedItemTitle={savedSearch.title}
isPlainRecord={isTextBasedQuery}
isEsqlMode={isEsqlMode}
onAddColumn={onAddColumn}
onFilter={onAddFilter as DocViewFilterFn}
onMoveColumn={onMoveColumn}
onRemoveColumn={onRemoveColumn}
onSort={!isTextBasedQuery ? onSort : undefined}
onSort={!isEsqlMode ? onSort : undefined}
useNewFieldsApi={useNewFieldsApi}
dataTestSubj="discoverDocTable"
/>
Expand Down Expand Up @@ -415,12 +407,12 @@ function DiscoverDocumentsComponent({
rowHeightState={rowHeight}
onUpdateRowHeight={onUpdateRowHeight}
isSortEnabled={true}
isPlainRecord={isTextBasedQuery}
isPlainRecord={isEsqlMode}
rowsPerPageState={rowsPerPage ?? getDefaultRowsPerPage(services.uiSettings)}
onUpdateRowsPerPage={onUpdateRowsPerPage}
maxAllowedSampleSize={getMaxAllowedSampleSize(services.uiSettings)}
sampleSizeState={getAllowedSampleSize(sampleSizeState, services.uiSettings)}
onUpdateSampleSize={!isTextBasedQuery ? onUpdateSampleSize : undefined}
onUpdateSampleSize={!isEsqlMode ? onUpdateSampleSize : undefined}
onFieldEdited={onFieldEdited}
configRowHeight={uiSettings.get(ROW_HEIGHT_OPTION)}
showMultiFields={uiSettings.get(SHOW_MULTIFIELDS)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {
DataDocuments$,
DataMain$,
DataTotalHits$,
RecordRawType,
} from '../../state_management/discover_data_state_container';
import { discoverServiceMock } from '../../../../__mocks__/services';
import { FetchStatus, SidebarToggleState } from '../../../types';
Expand Down Expand Up @@ -52,12 +51,12 @@ function getStateContainer(savedSearch?: SavedSearch) {
}

const mountComponent = async ({
isPlainRecord = false,
isEsqlMode = false,
storage,
savedSearch = savedSearchMockWithTimeField,
searchSessionId = '123',
}: {
isPlainRecord?: boolean;
isEsqlMode?: boolean;
isTimeBased?: boolean;
storage?: Storage;
savedSearch?: SavedSearch;
Expand Down Expand Up @@ -86,7 +85,6 @@ const mountComponent = async ({

const main$ = new BehaviorSubject({
fetchStatus: FetchStatus.COMPLETE,
recordRawType: isPlainRecord ? RecordRawType.PLAIN : RecordRawType.DOCUMENT,
foundDocuments: true,
}) as DataMain$;

Expand Down Expand Up @@ -121,7 +119,6 @@ const mountComponent = async ({
stateContainer.actions.undoSavedSearchChanges = jest.fn();

const props: DiscoverHistogramLayoutProps = {
isPlainRecord,
dataView,
stateContainer,
onFieldEdited: jest.fn(),
Expand Down Expand Up @@ -176,8 +173,8 @@ describe('Discover histogram layout component', () => {
expect(component.isEmptyRender()).toBe(false);
}, 10000);

it('should not render null if there is no search session, but isPlainRecord is true', async () => {
const { component } = await mountComponent({ isPlainRecord: true });
it('should not render null if there is no search session, but isEsqlMode is true', async () => {
const { component } = await mountComponent({ isEsqlMode: true });
expect(component.isEmptyRender()).toBe(false);
});

Expand Down
Loading

0 comments on commit 808a89c

Please sign in to comment.