Skip to content
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

[ES|QL] Hide field statistics tab and Dashboard when ES|QL is in use, disable Index data visualizer for MATCH and QSRT functions #197538

Merged
merged 36 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
f5fa6c6
Add getFieldStatsWarningMsgForQuery
qn895 Oct 23, 2024
8736c72
Add messaging for embeddables
qn895 Oct 23, 2024
b35e69c
Update messaging and add callout
qn895 Oct 24, 2024
2957796
Update callout style with icon
qn895 Oct 24, 2024
aff1b6f
Update styling
qn895 Oct 24, 2024
a6145ce
Make it so preview won't happen if not supported
qn895 Oct 24, 2024
ae086fc
Update messaging
qn895 Oct 24, 2024
1f9cef4
Exit early
qn895 Oct 28, 2024
38d47b9
Add new util to esql-utils
qn895 Oct 28, 2024
fafcc26
Merge remote-tracking branch 'upstream/main' into esql-match-disable
qn895 Oct 28, 2024
db63c8b
Merge branch 'main' into esql-match-disable
stratoula Oct 29, 2024
b2b7222
Refactor to have separate messaging for field stats, change from call…
qn895 Oct 30, 2024
7321cf5
Remove unused constants
qn895 Oct 31, 2024
4215c9c
Refactor
qn895 Oct 31, 2024
347e1ec
:Merge remote-tracking branch 'upstream/main' into esql-match-disable
qn895 Oct 31, 2024
989a523
Fix test
qn895 Oct 31, 2024
2dd74e9
Fix logic for detecting function
qn895 Oct 31, 2024
3ec8263
Update tests
qn895 Nov 4, 2024
cd06f88
Merge remote-tracking branch 'upstream/main' into esql-match-disable
qn895 Nov 4, 2024
2c4f740
Fix undefined
qn895 Nov 4, 2024
0dfc116
Remove field stats tab in ES|QL mode for Discover & dashboard, redire…
qn895 Nov 4, 2024
881e57b
[CI] Auto-commit changed files from 'node scripts/yarn_deduplicate'
kibanamachine Nov 4, 2024
7ea2efb
Update tests
qn895 Nov 4, 2024
b3c356e
Revert field list changes
qn895 Nov 5, 2024
bafff15
Add view mode
qn895 Nov 5, 2024
a8cd5be
Update tests
qn895 Nov 6, 2024
6b7ee45
Consolidate messaging style
qn895 Nov 6, 2024
dc99c68
Consolidate messaging
qn895 Nov 6, 2024
9d4f0f1
Update types
qn895 Nov 6, 2024
eabc053
Update logic for toggle when opening saved search
qn895 Nov 6, 2024
62c5496
Merge remote-tracking branch 'upstream/main' into esql-match-disable
qn895 Nov 6, 2024
5ccbdf7
More test updates
qn895 Nov 6, 2024
47f6210
Update jest test
qn895 Nov 11, 2024
ed826d0
Fix data visualizer page
qn895 Nov 11, 2024
5cf259a
Merge remote-tracking branch 'upstream/main' into esql-match-disable
qn895 Nov 11, 2024
2f2b2fb
Merge branch 'main' into esql-match-disable
elasticmachine Nov 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/kbn-esql-utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export {
isESQLColumnSortable,
isESQLColumnGroupable,
TextBasedLanguages,
queryCannotBeSampled,
} from './src';

export { ENABLE_ESQL, FEEDBACK_LINK } from './constants';
1 change: 1 addition & 0 deletions packages/kbn-esql-utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export {
retrieveMetadataColumns,
getQueryColumnsFromESQLQuery,
} from './utils/query_parsing_helpers';
export { queryCannotBeSampled } from './utils/query_contains_function';
export { appendToESQLQuery, appendWhereClauseToESQLQuery } from './utils/append_to_query';
export {
getESQLQueryColumns,
Expand Down
41 changes: 41 additions & 0 deletions packages/kbn-esql-utils/src/utils/query_contains_function.ts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some unit tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated here b2b7222 (#197538)

Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/
import type { AggregateQuery, Query } from '@kbn/es-query';
import { Walker } from '@kbn/esql-ast';
import { parse } from '@kbn/esql-ast';
import { isOfAggregateQueryType } from '@kbn/es-query';

/**
* Check if the query contains any of the function names being passed in
* @param query
* @param functions list of function names to check for
* @returns
*/
export const queryContainsFunction = (
query: AggregateQuery | Query | { [key: string]: any } | undefined | null,
functions: string[]
): boolean => {
if (query && isOfAggregateQueryType(query)) {
const { root } = parse(query.esql);
return functions.some((f) => Walker.hasFunction(root, f));
}
return false;
};

const UNSAMPLABLE_FUNCTIONS = ['match', 'qstr'];
/**
* Check if the query contains any function that cannot be used after LIMIT clause
* @param query
* @returns
*/
export const queryCannotBeSampled = (
query: AggregateQuery | Query | { [key: string]: any } | undefined | null
): boolean => {
return queryContainsFunction(query, UNSAMPLABLE_FUNCTIONS);
};
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ import {
import { FieldSummaryMessage } from './field_summary_message';
import { FieldNumberSummary, isNumberSummaryValid } from './field_number_summary';
import { ErrorBoundary } from '../error_boundary';
import {
FIELD_DATA_LABEL,
getReasonIfFieldStatsUnavailableForQuery,
} from '../../utils/get_warning_message';

export interface FieldStatsState {
isLoading: boolean;
Expand Down Expand Up @@ -187,6 +191,18 @@ const FieldStatsComponent: React.FC<FieldStatsProps> = ({
abortControllerRef.current?.abort();
abortControllerRef.current = new AbortController();

/**
* If the ES|QL query is unsupported, we can exit early
*/
const unsupportedReasonForQuery = isTextBased
? getReasonIfFieldStatsUnavailableForQuery(query, FIELD_DATA_LABEL)
: undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is quite large already. Let's move this logic to fetchAndCalculateFieldStats handler in

export async function fetchAndCalculateFieldStats(params: FetchAndCalculateFieldStatsParams) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up refactoring it to loadFieldStatsTextBased instead here 4215c9c (#197538)


if (unsupportedReasonForQuery) {
setState((s) => ({ ...s, isLoading: false }));
return;
}

const results = isTextBased
? await loadFieldStatsTextBased({
services: { data },
Expand Down Expand Up @@ -336,6 +352,20 @@ const FieldStatsComponent: React.FC<FieldStatsProps> = ({
: messageNoAnalysis;
}

const unsupportedReasonForQuery = getReasonIfFieldStatsUnavailableForQuery(
query,
FIELD_DATA_LABEL
);
if (unsupportedReasonForQuery) {
const messageUnsupportedReason = <FieldSummaryMessage message={unsupportedReasonForQuery} />;

return overrideMissingContent
? overrideMissingContent({
reason: 'unsupported',
element: messageUnsupportedReason,
})
: messageUnsupportedReason;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QQ: should we also skip the request to ES in this case?

Copy link
Member Author

@qn895 qn895 Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Definitely. Thanks for catching that. I'll make the change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

if (canProvideNumberSummaryForField(field, isTextBased) && isNumberSummaryValid(numberSummary)) {
title = (
<EuiTitle size="xxxs">
Expand Down
31 changes: 31 additions & 0 deletions packages/kbn-unified-field-list/src/utils/get_warning_message.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/
import type { AggregateQuery, Query } from '@kbn/es-query';
import { i18n } from '@kbn/i18n';
import { queryCannotBeSampled } from '@kbn/esql-utils';

const FIELD_STATISTICS_LABEL = i18n.translate('unifiedFieldList.fieldStats.fieldStatisticsLabel', {
defaultMessage: `Field statistics are`,
});

export const FIELD_DATA_LABEL = i18n.translate('unifiedFieldList.fieldStats.fieldDataLabel', {
defaultMessage: `Field data is`,
});

export const getReasonIfFieldStatsUnavailableForQuery = (
query?: AggregateQuery | Query | { [key: string]: any },
label: string = FIELD_STATISTICS_LABEL
): string | undefined => {
if (queryCannotBeSampled(query)) {
return i18n.translate('unifiedFieldList.fieldStats.notAvailableForMatchESQLQueryDescription', {
defaultMessage: `{label} not supported for ES|QL queries with 'MATCH' or 'QSTR' functions.`,
values: { label },
});
}
};
Copy link
Contributor

@jughosta jughosta Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would not work for all translations. I think we better define 2 full messages (without concatenation).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated here b2b7222 (#197538)

Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@
*/

import React, { useMemo, useEffect, useState, type ReactElement, useCallback } from 'react';
import { EuiFlexGroup, EuiFlexItem, EuiTab, EuiTabs, useEuiTheme } from '@elastic/eui';
import { EuiFlexGroup, EuiFlexItem, EuiTab, EuiTabs, EuiToolTip, useEuiTheme } from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n-react';
import { css } from '@emotion/react';
import { isLegacyTableEnabled, SHOW_FIELD_STATISTICS } from '@kbn/discover-utils';
import type { DataView } from '@kbn/data-views-plugin/common';
import useMountedState from 'react-use/lib/useMountedState';
import { getReasonIfFieldStatsUnavailableForQuery } from '@kbn/unified-field-list/src/utils/get_warning_message';
import { VIEW_MODE } from '../../../common/constants';
import { useDiscoverServices } from '../../hooks/use_discover_services';
import type { DiscoverStateContainer } from '../../application/main/state_management/discover_state';
Expand Down Expand Up @@ -46,6 +47,11 @@ export const DocumentViewModeToggle = ({
() => isLegacyTableEnabled({ uiSettings, isEsqlMode }),
[uiSettings, isEsqlMode]
);
const state = stateContainer.appState.getState();
const fieldStatsWarningMsgForQuery = useMemo(() => {
return getReasonIfFieldStatsUnavailableForQuery(state.query);
}, [state.query]);

const [showPatternAnalysisTab, setShowPatternAnalysisTab] = useState<boolean | null>(null);
const showFieldStatisticsTab = useMemo(
() => uiSettings.get(SHOW_FIELD_STATISTICS) && dataVisualizerService !== undefined,
Expand Down Expand Up @@ -157,14 +163,17 @@ export const DocumentViewModeToggle = ({

{showFieldStatisticsTab ? (
<EuiTab
disabled={fieldStatsWarningMsgForQuery !== undefined}
isSelected={viewMode === VIEW_MODE.AGGREGATED_LEVEL}
onClick={() => setDiscoverViewMode(VIEW_MODE.AGGREGATED_LEVEL)}
data-test-subj="dscViewModeFieldStatsButton"
>
<FormattedMessage
id="discover.viewModes.fieldStatistics.label"
defaultMessage="Field statistics"
/>
<EuiToolTip content={fieldStatsWarningMsgForQuery}>
<FormattedMessage
id="discover.viewModes.fieldStatistics.label"
defaultMessage="Field statistics"
/>
</EuiToolTip>
</EuiTab>
) : null}
</EuiTabs>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
EuiFlexItem,
EuiSuperDatePicker,
type EuiSuperDatePickerProps,
EuiToolTip,
} from '@elastic/eui';

import type { TimeRange } from '@kbn/es-query';
Expand Down Expand Up @@ -103,6 +104,10 @@ interface DatePickerWrapperProps {
* when EuiSuperDatePicker's 'Refresh'|'Update' button is clicked
*/
onRefresh?: () => void;
/**
* Tooltip message for the update button
*/
tooltipMessage?: string;
}

/**
Expand All @@ -122,6 +127,7 @@ export const DatePickerWrapper: FC<DatePickerWrapperProps> = (props) => {
isDisabled = false,
needsUpdate,
onRefresh,
tooltipMessage,
} = props;
const {
data,
Expand Down Expand Up @@ -318,34 +324,40 @@ export const DatePickerWrapper: FC<DatePickerWrapperProps> = (props) => {
recentlyUsedRanges={recentlyUsedRanges}
dateFormat={dateFormat}
commonlyUsedRanges={commonlyUsedRanges}
isDisabled={isDisabled}
updateButtonProps={{
iconOnly: isWithinLBreakpoint,
fill: false,
...(needsUpdate ? { needsUpdate } : {}),
}}
width={width}
isDisabled={isDisabled}
/>
</EuiFlexItem>
{showRefresh === true || !isTimeRangeSelectorEnabled ? (
<EuiFlexItem grow={false}>
<EuiButton
fill={false}
color={needsUpdate ? 'success' : 'primary'}
iconType={needsUpdate ? 'kqlFunction' : 'refresh'}
onClick={handleRefresh}
data-test-subj={`mlDatePickerRefreshPageButton${isLoading ? ' loading' : ' loaded'}`}
isLoading={isLoading}
>
{needsUpdate ? (
<FormattedMessage id="xpack.ml.datePicker.pageUpdateButton" defaultMessage="Update" />
) : (
<FormattedMessage
id="xpack.ml.datePicker.pageRefreshButton"
defaultMessage="Refresh"
/>
)}
</EuiButton>
<EuiToolTip content={tooltipMessage}>
<EuiButton
fill={false}
color={needsUpdate ? 'success' : 'primary'}
iconType={needsUpdate ? 'kqlFunction' : 'refresh'}
onClick={handleRefresh}
data-test-subj={`mlDatePickerRefreshPageButton${isLoading ? ' loading' : ' loaded'}`}
isLoading={isLoading}
isDisabled={isDisabled}
>
{needsUpdate ? (
<FormattedMessage
id="xpack.ml.datePicker.pageUpdateButton"
defaultMessage="Update"
/>
) : (
<FormattedMessage
id="xpack.ml.datePicker.pageRefreshButton"
defaultMessage="Refresh"
/>
)}
</EuiButton>
</EuiToolTip>
</EuiFlexItem>
) : null}
</>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ interface DataVisualizerTableProps<T extends object> {
error?: Error | string;
}

export const DataVisualizerTable = <T extends DataVisualizerTableItem>({
const UnmemoizedDataVisualizerTable = <T extends DataVisualizerTableItem>({
items,
pageState,
updatePageState,
Expand Down Expand Up @@ -506,3 +506,7 @@ export const DataVisualizerTable = <T extends DataVisualizerTableItem>({
</EuiResizeObserver>
);
};

export const DataVisualizerTable = React.memo(
UnmemoizedDataVisualizerTable
) as typeof UnmemoizedDataVisualizerTable;
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,13 @@ import {
EuiPanel,
EuiProgress,
EuiSpacer,
EuiCallOut,
} from '@elastic/eui';
import type { DataView } from '@kbn/data-views-plugin/common';
import { getIndexPatternFromESQLQuery } from '@kbn/esql-utils';
import { getReasonIfFieldStatsUnavailableForQuery } from '@kbn/unified-field-list/src/utils/get_warning_message';
import { getOrCreateDataViewByIndexPattern } from '../../search_strategy/requests/get_data_view_by_index_pattern';
import { useCurrentEuiTheme } from '../../../common/hooks/use_current_eui_theme';
import type { FieldVisConfig } from '../../../common/components/stats_table/types';
import { DATA_VISUALIZER_INDEX_VIEWER } from '../../constants/index_data_visualizer_viewer';
import { useDataVisualizerKibana } from '../../../kibana_context';
import type { GetAdditionalLinks } from '../../../common/components/results_links';
Expand Down Expand Up @@ -64,6 +65,8 @@ export const IndexDataVisualizerESQL: FC<IndexDataVisualizerESQLProps> = (dataVi
const [query, setQuery] = useState<ESQLQuery>(DEFAULT_ESQL_QUERY);
const [currentDataView, setCurrentDataView] = useState<DataView | undefined>();

const unsupportedReasonForQuery = getReasonIfFieldStatsUnavailableForQuery(localQuery);

const toggleShowEmptyFields = () => {
setDataVisualizerListState({
...dataVisualizerListState,
Expand Down Expand Up @@ -202,8 +205,11 @@ export const IndexDataVisualizerESQL: FC<IndexDataVisualizerESQLProps> = (dataVi
const onTextLangQuerySubmit = useCallback(
async (q: AggregateQuery | undefined) => {
if (isESQLQuery(q)) {
resetData();
setQuery(q);
const isUnsupported = getReasonIfFieldStatsUnavailableForQuery(q) !== undefined;
if (!isUnsupported) {
resetData();
setQuery(q);
}
}
},
[resetData]
Expand All @@ -224,8 +230,19 @@ export const IndexDataVisualizerESQL: FC<IndexDataVisualizerESQLProps> = (dataVi
data-test-subj="dataViewTitleHeader"
direction="row"
alignItems="center"
css={{ padding: `${euiTheme.euiSizeS} 0`, marginRight: `${euiTheme.euiSize}` }}
/>
css={{ padding: 0, marginRight: 0 }}
>
{unsupportedReasonForQuery ? (
<EuiFlexItem grow={true}>
<EuiCallOut
size="s"
iconType="warning"
color="warning"
title={unsupportedReasonForQuery}
/>
</EuiFlexItem>
) : null}
</EuiFlexGroup>

{isWithinLargeBreakpoint ? <EuiSpacer size="m" /> : null}
<EuiFlexGroup
Expand Down Expand Up @@ -253,7 +270,8 @@ export const IndexDataVisualizerESQL: FC<IndexDataVisualizerESQLProps> = (dataVi
width="full"
needsUpdate={queryNeedsUpdate}
onRefresh={handleRefresh}
isDisabled={!hasValidTimeField}
isDisabled={unsupportedReasonForQuery !== undefined}
tooltipMessage={unsupportedReasonForQuery}
/>
</EuiFlexItem>
</EuiFlexGroup>
Expand All @@ -276,6 +294,7 @@ export const IndexDataVisualizerESQL: FC<IndexDataVisualizerESQLProps> = (dataVi
hideRunQueryText={false}
isLoading={queryHistoryStatus ?? false}
displayDocumentationAsFlyout
disableSubmitAction={unsupportedReasonForQuery !== undefined}
/>
</EuiFlexItem>

Expand Down Expand Up @@ -312,7 +331,7 @@ export const IndexDataVisualizerESQL: FC<IndexDataVisualizerESQLProps> = (dataVi
<EuiSpacer size="s" />

<EuiProgress value={combinedProgress} max={100} size="xs" />
<DataVisualizerTable<FieldVisConfig>
<DataVisualizerTable
items={configs}
pageState={dataVisualizerListState}
updatePageState={setDataVisualizerListState}
Expand Down
Loading