Add framework to show related indexed views inside dataset selector and show banner in the Discover canvas for the selected dataset #8818

riysaxen-amzn · 2024-11-06T02:23:42Z

Description

This has all the changes from Base PR: #8701


In this PR we add support for

Selecting indexed view inside dataset selector. The indexed view is an Opensearch index which can be created for an external source like S3. When user selects an indexed view, we use this index for querying the data
Showing a banner at the top of Discover results canvas. This banner can be used to show actions related to the selected dataset. The first use we will add for this banner would be to show a callout to create an indexed view while searching an external source, or when an indexed view is selected, we can show an entry point to select other indexed view

and UTs for Configurator


 PASS  src/plugins/data/public/ui/dataset_selector/configurator.test.tsx (7.025 s)
  Configurator Component
    ✓ should render the component with the correct title and description (153 ms)
    ✓ should call onCancel when cancel button is clicked (83 ms)
    ✓ should call onPrevious when previous button is clicked (72 ms)
    ✓ should disable the submit button when conditions are not met (70 ms)
    ✓ should update state correctly when language is selected (68 ms)
    ✓ should fetch indexed views on mount and display them (53 ms)
    ✓ should initialize selectedLanguage with the current language from queryString (46 ms)
    ✓ should default selectedLanguage to the first language if currentLanguage is not supported (39 ms)
    ✓ should display the supported language dropdown correctly (41 ms)
    ✓ should disable the confirm button when submit is disabled (151 ms)

Test Suites: 1 passed, 1 total
Tests:       10 passed, 10 total
Snapshots:   0 total
Time:        7.964 s, estimated 8 s
Ran all test suites matching /src\/plugins\/data\/public\/ui\/dataset_selector\/configurator.test.tsx/i.

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

…s in dataset selector Signed-off-by: Amardeepsingh Siglani <[email protected]>

Signed-off-by: Amardeepsingh Siglani <[email protected]>

Signed-off-by: Riya Saxena <[email protected]>

virajsanghvi · 2024-11-07T01:07:02Z

changelogs/fragments/8818.yml

@@ -0,0 +1,3 @@
+feat:
+- Add framework to show banner at the top in discover results canvas ([#8701](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8818))


think you also want to update 8701

Signed-off-by: Riya Saxena <[email protected]>

virajsanghvi

Re-running ci to see if some checks will pass (different set than last time) but approving to unblock.

Will want to follow up and correct the pr number in the changelog fragment

Signed-off-by: Riya Saxena <[email protected]>

virajsanghvi

Checked screenshot for cigroup6, refresh button was present so likely a delay in showing up

kavilla · 2024-11-08T00:01:23Z

src/plugins/data/common/datasets/types.ts

+  | {
+      componentType: DatasetCanvasBannerComponentType.CALLOUT;
+      title: string;
+      iconType?: string;


how come iconType vs the actual icon following the current pattern: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data/common/datasets/types.ts#L155

kavilla · 2024-11-08T00:04:03Z

src/plugins/data/common/datasets/types.ts

+
+export type DatasetCanvasBannerProps =
+  | {
+      componentType: DatasetCanvasBannerComponentType.CALLOUT;


if my understanding of this is correct. This should just be type as it is redundant if this component is already a component and developers will understand that the type is associated to the type of component.

kavilla · 2024-11-08T00:07:14Z

src/plugins/data/common/datasets/types.ts

@@ -256,6 +258,29 @@ export interface DatasetField {
  // TODO:  osdFieldType?
 }

+export enum DatasetCanvasBannerComponentType {
+  CALLOUT = 'callout',


thanks for making this a string. but could we follow current pattern could we make this all caps: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data/common/datasets/types.ts#L136

might be a nitpick but it's easier to write code expecting strings to be in a certain casing

src/plugins/data/common/datasets/types.ts

kavilla · 2024-11-08T00:10:21Z

src/plugins/data/common/datasets/types.ts

@@ -256,6 +258,29 @@ export interface DatasetField {
  // TODO:  osdFieldType?
 }

+export enum DatasetCanvasBannerComponentType {
+  CALLOUT = 'callout',
+  CUSTOM = 'custom',


Suggested change

CUSTOM = 'custom',

CUSTOM = 'CUSTOM',

kavilla · 2024-11-08T00:10:39Z

src/plugins/data/common/datasets/types.ts

+
+export type DatasetCanvasBannerProps =
+  | {
+      componentType: DatasetCanvasBannerComponentType.CALLOUT;


Suggested change

componentType: DatasetCanvasBannerComponentType.CALLOUT;

type: DatasetCanvasBannerComponentType.CALLOUT;

AMoo-Miki · 2024-11-08T00:11:15Z

src/plugins/data/public/index.ts

@@ -493,6 +493,7 @@ export {
  QueryStart,
  PersistedLog,
  LanguageReference,
+  IndexedViewsService,


Didn't we rename this to DatasetIndexedViewsService?

AMoo-Miki · 2024-11-08T00:12:09Z

src/plugins/data/public/query/query_string/index.ts

@@ -34,6 +34,7 @@ export {
  DatasetService,
  DatasetServiceContract,
  DatasetTypeConfig,
+  DatasetIndexedViewsService as IndexedViewsService,


The whole point of prefixing is to avoid name collision. Exporting it without a prefix out of here doesn't mitigate that.

kavilla · 2024-11-08T00:25:56Z

src/plugins/data/common/datasets/types.ts

+  CUSTOM = 'custom',
+}
+
+export type DatasetCanvasBannerProps =


i think we should have a BaseDatasetCanvasBannerProps or DatasetCanvasBannerProps that has required props to add a new banner. Then create a type called DatasetCanvasCalloutBannerProps and DatasetCanvasCustomBannerProps or something.

if developing these union types become quiet difficult to understand sometimes and caused the linter to complain because the type isn't strict enough. easier to work with explicit types.

as far as the base props i think we should have at least

export type DatasetCanvasBannerProps = { // required. unique id, this will be used to associate a dataset to a canvas props. this is different than title because title is something that could be displayed but if i have two devs trying to add a new banner called `Index Callout Banner` (haven't looked at the code yet) but if the component doesn't change in an ID then maybe we won't render an update by accident. id: string; title: string; // required. going from optional to required would be a breaking change. and haven't looked at how this is used but i can imagine we don't really want to have a case where title is undefined.

that or title to be name similar to the flyout stuff.

we should also consider maybe key but we should def include

className?: string; 'data-test-subj'?: string;

writing tests on this abstract types would make this so much easier and if we have to style them the className will help

kavilla · 2024-11-08T00:28:41Z

src/plugins/data/common/datasets/types.ts

@@ -256,6 +258,29 @@ export interface DatasetField {
  // TODO:  osdFieldType?
 }

+export enum DatasetCanvasBannerComponentType {


Suggested change

export enum DatasetCanvasBannerComponentType {

export enum DATASET_CANVAS_BANNER_COMPONENT_TYPES {

kavilla · 2024-11-08T00:37:15Z

src/plugins/data/common/datasets/types.ts

@@ -247,6 +247,8 @@ export interface Dataset extends BaseDataset {
  timeFieldName?: string;
  /** Optional language to default to from the language selector */
  language?: string;
+  /** Optional name of the indexed view to search data from */
+  indexedView?: string;


could you add more insight on what this is?
is this to be the id of the indexedView component. is this a unique id generated from something?

without much context this for me this seems something like a data view is that correct @riysaxen-amzn ? if you can update to include an example above that would help with this insight

can any developer add an indexed view or is it only related to a specific data type? the dataset interface was derived from the index pattern interface so that we can leverage the current system setup. the current system setup assumes the title is the thing it will query. if this is changing that assumption it might be worth considering if this is a different interface like DataViewset extends BaseDataset

cc: @virajsanghvi, @AMoo-Miki , @ashwin-pc curious your opinion here

kavilla · 2024-11-08T00:49:54Z

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

@@ -40,10 +41,21 @@ export const Configurator = ({
  const queryString = queryService.queryString;
  const languageService = queryService.queryString.getLanguageService();
  const indexPatternsService = getIndexPatterns();
-  const type = queryString.getDatasetService().getType(baseDataset.type);
+  const type = queryString?.getDatasetService().getType(baseDataset.type);


queryString manager shouldn't be optional how come? the type might be undefined but the app won't work if queryString is undefined

Co-authored-by: Kawika Avilla <[email protected]> Signed-off-by: Riya <[email protected]>

ruanyl · 2024-11-08T01:13:10Z

@riysaxen-amzn FYI, noticed there is a conflict :)

amsiglan and others added 9 commits November 1, 2024 14:45

show create indexed view banner in discover canvas; show indexed view…

3c47e6e

…s in dataset selector Signed-off-by: Amardeepsingh Siglani <[email protected]>

Changeset file for PR opensearch-project#8701 created/updated

8f69898

minor update

f1220ac

Signed-off-by: Amardeepsingh Siglani <[email protected]>

fixed logic

1678d6f

Signed-off-by: Amardeepsingh Siglani <[email protected]>

Changeset file for PR opensearch-project#8701 created/updated

f16bad3

addressed PR comments

d76e42c

Signed-off-by: Amardeepsingh Siglani <[email protected]>

addressed PR comments

03fbf03

Signed-off-by: Amardeepsingh Siglani <[email protected]>

fixed logic

db13dee

Signed-off-by: Amardeepsingh Siglani <[email protected]>

added some UTs for configurator

033acee

Signed-off-by: Riya Saxena <[email protected]>

riysaxen-amzn requested a review from virajsanghvi November 6, 2024 22:17

riysaxen-amzn added 2 commits November 6, 2024 16:58

changelogs added

def4472

Signed-off-by: Riya Saxena <[email protected]>

changelogs added

b3dba93

Signed-off-by: Riya Saxena <[email protected]>

virajsanghvi reviewed Nov 7, 2024

View reviewed changes

fix getSearchBannerProps, pass dataset

8361034

Signed-off-by: Riya Saxena <[email protected]>

virajsanghvi previously approved these changes Nov 7, 2024

View reviewed changes

changelog update

3119e04

Signed-off-by: Riya Saxena <[email protected]>

riysaxen-amzn dismissed virajsanghvi’s stale review via 3119e04 November 7, 2024 02:48

riysaxen-amzn requested a review from virajsanghvi November 7, 2024 02:48

virajsanghvi previously approved these changes Nov 7, 2024

View reviewed changes

github-actions bot removed the failed changeset label Nov 7, 2024

kavilla added discover for discover reinvent discover-next labels Nov 7, 2024

kavilla reviewed Nov 8, 2024

View reviewed changes

src/plugins/data/common/datasets/types.ts Outdated Show resolved Hide resolved

kavilla reviewed Nov 8, 2024

View reviewed changes

AMoo-Miki reviewed Nov 8, 2024

View reviewed changes

kavilla reviewed Nov 8, 2024

View reviewed changes

Update src/plugins/data/common/datasets/types.ts

0098bcb

Co-authored-by: Kawika Avilla <[email protected]> Signed-off-by: Riya <[email protected]>

riysaxen-amzn dismissed virajsanghvi’s stale review via 0098bcb November 8, 2024 00:59

virajsanghvi mentioned this pull request Nov 12, 2024

Indexed views framework #8851

Merged

7 tasks

amsiglan closed this Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add framework to show related indexed views inside dataset selector and show banner in the Discover canvas for the selected dataset #8818

Add framework to show related indexed views inside dataset selector and show banner in the Discover canvas for the selected dataset #8818

riysaxen-amzn commented Nov 6, 2024 •

edited

Loading

virajsanghvi Nov 7, 2024

riysaxen-amzn Nov 7, 2024

virajsanghvi left a comment

virajsanghvi left a comment

kavilla Nov 8, 2024

kavilla Nov 8, 2024

kavilla Nov 8, 2024

kavilla Nov 8, 2024

kavilla Nov 8, 2024

AMoo-Miki Nov 8, 2024

AMoo-Miki Nov 8, 2024

kavilla Nov 8, 2024

kavilla Nov 8, 2024

kavilla Nov 8, 2024

kavilla Nov 8, 2024 •

edited

Loading

kavilla Nov 8, 2024

ruanyl commented Nov 8, 2024

		@@ -0,0 +1,3 @@
		feat:
		- Add framework to show banner at the top in discover results canvas ([#8701](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8818))

	componentType: DatasetCanvasBannerComponentType.CALLOUT;
	type: DatasetCanvasBannerComponentType.CALLOUT;

	export enum DatasetCanvasBannerComponentType {
	export enum DATASET_CANVAS_BANNER_COMPONENT_TYPES {

Add framework to show related indexed views inside dataset selector and show banner in the Discover canvas for the selected dataset #8818

Add framework to show related indexed views inside dataset selector and show banner in the Discover canvas for the selected dataset #8818

Conversation

riysaxen-amzn commented Nov 6, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virajsanghvi left a comment

Choose a reason for hiding this comment

virajsanghvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kavilla Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanyl commented Nov 8, 2024

riysaxen-amzn commented Nov 6, 2024 •

edited

Loading

kavilla Nov 8, 2024 •

edited

Loading