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

Add support for s3 fields in discover #8609

Merged
merged 7 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
2 changes: 2 additions & 0 deletions changelogs/fragments/8609.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- Discover 2.0 support for S3 fields ([#8609](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8609))
2 changes: 2 additions & 0 deletions src/plugins/data/common/datasets/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export interface DataSourceMeta {
name?: string;
/** Optional session ID for faster responses when utilizing async query sources */
sessionId?: string;
/** Optional supportsTimeFilter determines if a time filter is needed */
supportsTimeFilter?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

can this be controlled based on whether timeField is present in the dataset rather than additional flag?

Copy link
Member Author

@ps48 ps48 Oct 22, 2024

Choose a reason for hiding this comment

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

Today even if timeField is present we don't have support for S3 datasources to automatic time filter injection in the query. This is surely where we want to go towards in future though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some older conversations around this: #8337 (comment), #8337 (comment)

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,25 +32,25 @@
import { SavedObjectsClientCommon } from '../..';
import { DuplicateField } from '../../../../opensearch_dashboards_utils/common';

import { SerializedFieldFormat } from '../../../../expressions/common';
import {
OPENSEARCH_FIELD_TYPES,
OSD_FIELD_TYPES,
IIndexPattern,
FieldFormatNotFoundError,
IFieldType,
IIndexPattern,
OPENSEARCH_FIELD_TYPES,
OSD_FIELD_TYPES,
} from '../../../common';
import { IndexPatternField, IIndexPatternFieldList, fieldList } from '../fields';
import { formatHitProvider } from './format_hit';
import { flattenHitWrapper } from './flatten_hit';
import { FieldFormatsStartCommon, FieldFormat } from '../../field_formats';
import { FieldFormat, FieldFormatsStartCommon } from '../../field_formats';
import { IIndexPatternFieldList, IndexPatternField, fieldList } from '../fields';
import {
IndexPatternSpec,
TypeMeta,
SourceFilter,
IndexPatternFieldMap,
IndexPatternSpec,
SavedObjectReference,
SourceFilter,
TypeMeta,
} from '../types';
import { SerializedFieldFormat } from '../../../../expressions/common';
import { flattenHitWrapper } from './flatten_hit';
import { formatHitProvider } from './format_hit';

interface IndexPatternDeps {
spec?: IndexPatternSpec;
Expand Down Expand Up @@ -94,6 +94,7 @@
public version: string | undefined;
public sourceFilters?: SourceFilter[];
public dataSourceRef?: SavedObjectReference;
public fieldsLoading?: boolean;
private originalSavedObjectBody: SavedObjectBody = {};
private shortDotsEnable: boolean = false;
private fieldFormats: FieldFormatsStartCommon;
Expand Down Expand Up @@ -137,6 +138,7 @@
return this.deserializeFieldFormatMap(mapping);
});
this.dataSourceRef = spec.dataSourceRef;
this.fieldsLoading = spec.fieldsLoading;
}

/**
Expand Down Expand Up @@ -378,6 +380,10 @@
: [];
};

setFieldsLoading = (status: boolean) => {
return (this.fieldsLoading = status);

Check warning on line 384 in src/plugins/data/common/index_patterns/index_patterns/index_pattern.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/common/index_patterns/index_patterns/index_pattern.ts#L384

Added line #L384 was not covered by tests
};

/**
* Provide a field, get its formatter
* @param field
Expand Down
7 changes: 4 additions & 3 deletions src/plugins/data/common/index_patterns/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@
* under the License.
*/

import { ToastInputFields, ErrorToastOptions } from 'src/core/public/notifications';
import { ErrorToastOptions, ToastInputFields } from 'src/core/public/notifications';
// eslint-disable-next-line
import type { SavedObject } from 'src/core/server';
import { IFieldType } from './fields';
import { FieldFormat, IndexPatternField, OSD_FIELD_TYPES } from '..';
import { SerializedFieldFormat } from '../../../expressions/common';
import { OSD_FIELD_TYPES, IndexPatternField, FieldFormat } from '..';
import { IFieldType } from './fields';

export type FieldFormatMap = Record<string, SerializedFieldFormat>;

Expand Down Expand Up @@ -201,6 +201,7 @@ export interface IndexPatternSpec {
typeMeta?: TypeMeta;
type?: string;
dataSourceRef?: SavedObjectReference;
fieldsLoading?: boolean;
}

export interface SourceFilter {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,22 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { CoreStart } from 'opensearch-dashboards/public';
import LRUCache from 'lru-cache';
import { CoreStart } from 'opensearch-dashboards/public';
import {
CachedDataStructure,
Dataset,
DataStorage,
DataStructure,
IndexPatternSpec,
DEFAULT_DATA,
IndexPatternFieldMap,
IndexPatternSpec,
UI_SETTINGS,
DataStorage,
CachedDataStructure,
} from '../../../../common';
import { DatasetTypeConfig, DataStructureFetchOptions } from './types';
import { indexPatternTypeConfig, indexTypeConfig } from './lib';
import { IndexPatternsContract } from '../../../index_patterns';
import { IDataPluginServices } from '../../../types';
import { indexPatternTypeConfig, indexTypeConfig } from './lib';
import { DatasetTypeConfig, DataStructureFetchOptions } from './types';

export class DatasetService {
private indexPatterns?: IndexPatternsContract;
Expand Down Expand Up @@ -91,29 +92,54 @@
}
}

public async cacheDataset(dataset: Dataset): Promise<void> {
const type = this.getType(dataset.type);
if (dataset && dataset.type !== DEFAULT_DATA.SET_TYPES.INDEX_PATTERN) {
const spec = {
id: dataset.id,
title: dataset.title,
timeFieldName: dataset.timeFieldName,
fields: await type?.fetchFields(dataset),
dataSourceRef: dataset.dataSource
? {
id: dataset.dataSource.id!,
name: dataset.dataSource.title,
type: dataset.dataSource.type,
}
: undefined,
} as IndexPatternSpec;
const temporaryIndexPattern = await this.indexPatterns?.create(spec, true);
if (temporaryIndexPattern) {
this.indexPatterns?.saveToCache(dataset.id, temporaryIndexPattern);
public async cacheDataset(
dataset: Dataset,
services: Partial<IDataPluginServices>
): Promise<void> {
const type = this.getType(dataset?.type);
try {
const asyncType = type && type.meta.isFieldLoadAsync;
if (dataset && dataset.type !== DEFAULT_DATA.SET_TYPES.INDEX_PATTERN) {
const fetchedFields = asyncType
? await type?.fetchFields(dataset, services)
: ({} as IndexPatternFieldMap);
const spec = {
id: dataset.id,
title: dataset.title,
timeFieldName: dataset.timeFieldName,
fields: fetchedFields,
fieldsLoading: asyncType ? true : false,
dataSourceRef: dataset.dataSource
? {
id: dataset.dataSource.id!,
name: dataset.dataSource.title,
type: dataset.dataSource.type,
}
: undefined,
} as IndexPatternSpec;
const temporaryIndexPattern = await this.indexPatterns?.create(spec, true);

// Load schema asynchronously if it's an async index pattern
if (asyncType && temporaryIndexPattern) {
Copy link
Member

Choose a reason for hiding this comment

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

does async index patterns mean the metadata like field schema is not stored in the index pattern saved object?

Copy link
Member Author

Choose a reason for hiding this comment

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

Async index patterns means that the fields are lazy loaded; i.e. the temporary index pattern is created with empty fields and when the fields are loaded in the index pattern gets hydrated with the fields.

type

Check warning on line 124 in src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts#L124

Added line #L124 was not covered by tests
.fetchFields(dataset, services)
.then((fields) => {
temporaryIndexPattern?.fields.replaceAll([...fields]);
this.indexPatterns?.saveToCache(dataset.id, temporaryIndexPattern);

Check warning on line 128 in src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts#L127-L128

Added lines #L127 - L128 were not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Handling of this request should ideally be tested. Can we follow up on this?

})
.finally(() => {
temporaryIndexPattern.setFieldsLoading(false);

Check warning on line 131 in src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts#L131

Added line #L131 was not covered by tests
});
}

if (temporaryIndexPattern) {
this.indexPatterns?.saveToCache(dataset.id, temporaryIndexPattern);
}
}
} catch (error) {
throw new Error(`Failed to load dataset: ${dataset?.id}`);

Check warning on line 140 in src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts#L140

Added line #L140 was not covered by tests
}
}

public async fetchOptions(
services: IDataPluginServices,
path: DataStructure[],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ export interface DatasetTypeConfig {
tooltip?: string;
/** Optional preference for search on page load else defaulted to true */
searchOnLoad?: boolean;
/** Optional supportsTimeFilter determines if a time filter is needed */
supportsTimeFilter?: boolean;
/** Optional isFieldLoadAsync determines if field loads are async */
isFieldLoadAsync?: boolean;
};
/**
* Converts a DataStructure to a Dataset.
Expand All @@ -55,7 +59,10 @@ export interface DatasetTypeConfig {
* Fetches fields for the dataset.
* @returns {Promise<DatasetField[]>} A promise that resolves to an array of DatasetFields.
*/
fetchFields: (dataset: Dataset) => Promise<DatasetField[]>;
fetchFields: (
dataset: Dataset,
services?: Partial<IDataPluginServices>
) => Promise<DatasetField[]>;
/**
* Retrieves the supported query languages for this dataset type.
* @returns {Promise<string[]>} A promise that resolves to an array of supported language ids.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ import {
DataStructure,
DEFAULT_DATA,
} from '../../../common';
import { DatasetExplorer } from './dataset_explorer';
import { Configurator } from './configurator';
import { getQueryService } from '../../services';
import { IDataPluginServices } from '../../types';
import { Configurator } from './configurator';
import { DatasetExplorer } from './dataset_explorer';

export const AdvancedSelector = ({
services,
Expand Down Expand Up @@ -52,6 +52,7 @@ export const AdvancedSelector = ({

return selectedDataset ? (
<Configurator
services={services}
baseDataset={selectedDataset}
onConfirm={onSelect}
onCancel={onCancel}
Expand Down
10 changes: 9 additions & 1 deletion src/plugins/data/public/ui/dataset_selector/configurator.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,16 @@
import React, { useEffect, useMemo, useState } from 'react';
import { BaseDataset, DEFAULT_DATA, Dataset, DatasetField } from '../../../common';
import { getIndexPatterns, getQueryService } from '../../services';
import { IDataPluginServices } from '../../types';

export const Configurator = ({
services,
baseDataset,
onConfirm,
onCancel,
onPrevious,
}: {
services: IDataPluginServices;
baseDataset: BaseDataset;
onConfirm: (dataset: Dataset) => void;
onCancel: () => void;
Expand Down Expand Up @@ -80,6 +83,11 @@
setTimeFields(dateFields || []);
};

if (baseDataset?.dataSource?.meta?.supportsTimeFilter === false) {
ps48 marked this conversation as resolved.
Show resolved Hide resolved
setTimeFields([]);
Copy link
Member

Choose a reason for hiding this comment

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

this [] creates a different object every time when called, and will trigger a re-render. might be better to add a guard

Suggested change
setTimeFields([]);
if (timeFields.length > 0) setTimeFields([]);

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: 53198c4

return;

Check warning on line 88 in src/plugins/data/public/ui/dataset_selector/configurator.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/ui/dataset_selector/configurator.tsx#L87-L88

Added lines #L87 - L88 were not covered by tests
}

fetchFields();
}, [baseDataset, indexPatternsService, queryString]);

Expand Down Expand Up @@ -192,7 +200,7 @@
</EuiButton>
<EuiButton
onClick={async () => {
await queryString.getDatasetService().cacheDataset(dataset);
await queryString.getDatasetService().cacheDataset(dataset, services);

Check warning on line 203 in src/plugins/data/public/ui/dataset_selector/configurator.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/ui/dataset_selector/configurator.tsx#L203

Added line #L203 was not covered by tests
onConfirm(dataset);
}}
fill
Expand Down
ps48 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -28,31 +28,31 @@
* under the License.
*/

import './discover_sidebar.scss';
import React, { useCallback, useEffect, useState, useMemo } from 'react';
import { i18n } from '@osd/i18n';
import {
EuiDragDropContext,
DropResult,
EuiDroppable,
EuiButtonEmpty,
EuiDragDropContext,
EuiDraggable,
EuiDroppable,
EuiPanel,
EuiSplitPanel,
EuiButtonEmpty,
} from '@elastic/eui';
import { i18n } from '@osd/i18n';
import { I18nProvider } from '@osd/i18n/react';
import React, { useCallback, useEffect, useMemo, useState } from 'react';
import { IndexPattern, IndexPatternField, UI_SETTINGS } from '../../../../../data/public';
import { FIELDS_LIMIT_SETTING } from '../../../../common';
import { getServices } from '../../../opensearch_dashboards_services';
import { DiscoverField } from './discover_field';
import { DiscoverFieldSearch } from './discover_field_search';
import { DiscoverFieldDataFrame } from './discover_field_data_frame';
import { FIELDS_LIMIT_SETTING } from '../../../../common';
import { groupFields } from './lib/group_fields';
import { IndexPatternField, IndexPattern, UI_SETTINGS } from '../../../../../data/public';
import { getDetails } from './lib/get_details';
import { DiscoverFieldSearch } from './discover_field_search';
import './discover_sidebar.scss';
import { displayIndexPatternCreation } from './lib/display_index_pattern_creation';
import { getDefaultFieldFilter, setFieldFilterProp } from './lib/field_filter';
import { getDetails } from './lib/get_details';
import { getIndexPatternFieldList } from './lib/get_index_pattern_field_list';
import { getServices } from '../../../opensearch_dashboards_services';
import { groupFields } from './lib/group_fields';
import { FieldDetails } from './types';
import { displayIndexPatternCreation } from './lib/display_index_pattern_creation';

export interface DiscoverSidebarProps {
/**
Expand Down Expand Up @@ -231,11 +231,12 @@ export function DiscoverSidebar(props: DiscoverSidebarProps) {
/>
</EuiSplitPanel.Inner>
) : null}

<EuiSplitPanel.Inner
className="eui-yScroll dscSideBar_fieldListContainer"
paddingSize="none"
>
{fields.length > 0 && (
{(fields.length > 0 || selectedIndexPattern.fieldsLoading) && (
Copy link
Member

Choose a reason for hiding this comment

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

if loading and fields list is empty, what does UI show? should it indicate that it's loading?

Copy link
Member Author

Choose a reason for hiding this comment

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

If loading and fields list is empty -> The UI shows the sidebar in the loading state.

<>
<FieldList
category="selected"
Expand Down Expand Up @@ -312,6 +313,7 @@ const FieldList = ({
size="xs"
className="dscSideBar_fieldGroup"
aria-label={title}
isLoading={selectedIndexPattern.fieldsLoading ?? false}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
isLoading={selectedIndexPattern.fieldsLoading ?? false}
isLoading={selectedIndexPattern.fieldsLoading}

or coerce to boolean !!selectedIndexPattern.fieldsLoading?

Copy link
Member Author

Choose a reason for hiding this comment

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

Coerce makes sense: 53198c4

>
{title}
</EuiButtonEmpty>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { useCallback, useEffect, useMemo, useState } from 'react';
import { i18n } from '@osd/i18n';
import { useCallback, useEffect, useMemo, useState } from 'react';
import { IndexPattern, useQueryStringManager } from '../../../../../data/public';
import { useSelector, updateIndexPattern } from '../../utils/state_management';
import { QUERY_ENHANCEMENT_ENABLED_SETTING } from '../../../../common';
import { DiscoverViewServices } from '../../../build_services';
import { getIndexPatternId } from '../../helpers/get_index_pattern_id';
import { QUERY_ENHANCEMENT_ENABLED_SETTING } from '../../../../common';
import { updateIndexPattern, useSelector } from '../../utils/state_management';

/**
* Custom hook to fetch and manage the index pattern based on the provided services.
Expand Down Expand Up @@ -51,7 +51,13 @@
query.dataset.type !== 'INDEX_PATTERN'
);
if (!pattern) {
await data.query.queryString.getDatasetService().cacheDataset(query.dataset);
await data.query.queryString.getDatasetService().cacheDataset(query.dataset, {

Check warning on line 54 in src/plugins/discover/public/application/view_components/utils/use_index_pattern.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/discover/public/application/view_components/utils/use_index_pattern.ts#L54

Added line #L54 was not covered by tests
uiSettings: services.uiSettings,
savedObjects: services.savedObjects,
notifications: services.notifications,
http: services.http,
data: services.data,
});
pattern = await data.indexPatterns.get(
query.dataset.id,
query.dataset.type !== 'INDEX_PATTERN'
Expand Down Expand Up @@ -98,6 +104,7 @@
isMounted = false;
};
}, [
services,
isQueryEnhancementEnabled,
indexPatternIdFromState,
fetchIndexPatternDetails,
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/query_enhancements/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,5 @@ export const UI_SETTINGS = {
};

export const ERROR_DETAILS = { GUARDRAILS_TRIGGERED: 'guardrails triggered' };

export const S3_PARTITION_INFO_COLUMN = '# Partition Information';
Loading
Loading