Indexed views framework #8851

amsiglan · 2024-11-12T16:25:09Z

Description

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

Issues Resolved

New feature

Screenshot

Note: Below screenshots were produced by making changes to the S3 type config for testing and are not part of this PR since this PR is only adding the framework.

Callout for creating indexed view

Action from callout can open the create flyout (this is all handled by the type config provider and not in this PR's code)

Create indexed view banner:

Testing the changes

Set up workspace with query enhancements enabled

For the banner in Discover canvas

[For testing only]: Update the S3 config type to return the banner props based on the fetch status
Discover will render the banner based on provided props

For showing indexed views in dataset selector

Update the S3 config type to return implemented instance of IndexedViewsService
Dataset selector will show the indexed views in the second step

Changelog

feat: Add framework to show banner at the top in discover results canvas
feat: Show indexed views in dataset selector

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

codecov · 2024-11-12T16:54:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.39%. Comparing base (25d3de7) to head (d9bbd23).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8851      +/-   ##
==========================================
- Coverage   60.90%   56.39%   -4.52%     
==========================================
  Files        3800     1239    -2561     
  Lines       90845    25913   -64932     
  Branches    14316     4437    -9879     
==========================================
- Hits        55331    14613   -40718     
+ Misses      31994    10559   -21435     
+ Partials     3520      741    -2779

Flag	Coverage Δ
Linux_1	`?`
Linux_2	`56.39% <ø> (ø)`
Linux_3	`?`
Linux_4	`?`
Windows_1	`?`
Windows_2	`?`
Windows_3	`?`
Windows_4	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

virajsanghvi

Callout that there are multiple ci items failing - please take a look (looks like lint and snapshot tests, and patch coverage). Also, given patch coverage % and the fact that this is a framework for adding additional functionality, would expect unit test coverage to be higher.

virajsanghvi

Does this incorporate all feedback addressed in #8818?

Can you point out the major new changes from previous PRs?

virajsanghvi · 2024-11-12T18:07:50Z

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

+  /** Optional reference to the source dataset. Example usage is for indexed views to store the
+   * reference to the table dataset 
+   */
+  ref?: {


Shouldn't we name this something less generic?

@kavilla Is this the right place for this? Isn't the source an attribute of the index that was created? Would expect that to be part of DataSource definition I guess, although if this is a two-way door seems fine for now (I think it would be as not having source in the right place wouldn't impact functionality for saved query/recent dataset?).

that is a good point. for example we do store the session id wtih the meta info about the data source since it's the data source that decides that where as the data source would also include the index that is pointing to. originally i think we though it should go in the data source meta but didn't have too much of hard buy in on the data source. but reflecting it makes more sense there.

agreeed as well this is a two way door. as i believe we can still be able to query the info i think the ref now is used for ui purposes (for now) but def will need to update it.

Can we create an issue to track the rename and move?

virajsanghvi · 2024-11-12T18:15:41Z

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

+                      }
+                    )}
+                  </EuiFormLabel>  }
+                onChange={(e) => setSelectIndexedView(e.target.checked)}


Didn't @riysaxen-amzn add tests for a bunch of these? Looks like they're all here, but not sure why coverage report looks different

src/plugins/discover/public/application/view_components/canvas/index.tsx

...public/application/view_components/canvas/query_result_extensions/query_result_extension.tsx

ashwin-pc

Didnt get a chance to review the configurator changes but had a look at the rest. Added my comments below. I agree with Viraj, can we add unit tests for the new code introduced?

src/plugins/data/public/query/query_string/dataset_service/types.ts

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

src/plugins/vis_builder/public/application/components/data_tab/field_selector.tsx

src/plugins/data/public/query/query_string/query_results_service/query_result_service.ts

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

src/plugins/data/public/ui/query_editor/_query_editor.scss

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

kavilla · 2024-11-13T23:54:56Z

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

+  /** Optional reference to the source dataset. Example usage is for indexed views to store the
+   * reference to the table dataset
+   */
+  sourceDatasetRef?: {


nit:
where you able to comment on @virajsanghvi previous feedback?
def could be follow up but just some insight url length is limited:

Application and global state are stored in URL parameters

Although we're far from URL length limits, field name length matters

Should follow existing naming conventions (e.g. 'timeFieldName' from index patterns)

Consider shorter property names when possible

I suggested this name - ideally we aren't coupling our interfaces to urls so they aren't impacted by url limits.

Do you have a suggested name that accurately reflects what this is?

Do you have a suggested name that accurately reflects what this is?

nope :). i didn't have a good suggestion and was being pedantic and something to keep track of.

src/plugins/data/public/ui/query_editor/query_editor_extensions/query_editor_extensions.tsx

virajsanghvi

Can we resolve @kavilla's concern around the connected data source?

src/plugins/data/public/ui/query_editor/_query_editor.scss

src/plugins/data/public/ui/query_editor/query_editor_extensions/query_editor_extension.tsx

virajsanghvi · 2024-11-13T23:57:51Z

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

+  /** Optional reference to the source dataset. Example usage is for indexed views to store the
+   * reference to the table dataset
+   */
+  sourceDatasetRef?: {


I suggested this name - ideally we aren't coupling our interfaces to urls so they aren't impacted by url limits.

Do you have a suggested name that accurately reflects what this is?

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

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

virajsanghvi · 2024-11-14T01:05:06Z

src/plugins/data/public/ui/query_editor/_query_editor.scss

@@ -141,7 +141,7 @@
 }

 .osdQueryEditor__bottomPanel {
-  margin: 5px 0;
+  margin: $euiSizeXS 0;


Just a call out - this isn't equivalent to 5px

Removed it from here and yes 4px would also be fine in this case

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

kavilla · 2024-11-14T01:29:30Z

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

+      return dataset;
+    }
+
+    let connectedDataSource;


can be a fast follow but i think the indexed view service could handle fetching the datasource and creating and returning an indexed view vs the configurator doing this work

kavilla · 2024-11-14T01:31:31Z

src/plugins/data/public/ui/query_editor/query_editor_extensions/query_editor_extension.tsx

+  /**
+   * Currently set Query
+   */
+  query: Query;


nit: what are we using this for? if it's for accessing the current query since it's with the same component it should already have access to the same query. therefore we could just access this query directly

This is used to access the dataset on the query, passed in query to be future safe

virajsanghvi

Approving assuming ci passes

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

virajsanghvi

Restarted failing ci, did pass on linux so assuming it'll pass on windows

ashwin-pc · 2024-11-14T17:15:05Z

Skipping CI group 10 since the failed test case seems to be flakey. The rest of the tests in the suite pass

queries.spec.js.mp4

* indexed-view-working Signed-off-by: Amardeepsingh Siglani <[email protected]> * working skeleton Signed-off-by: Amardeepsingh Siglani <[email protected]> * Changeset file for PR opensearch-project#8851 created/updated * removed unwanted changes Signed-off-by: Amardeepsingh Siglani <[email protected]> * fixed linter errors; failing UT Signed-off-by: Amardeepsingh Siglani <[email protected]> * added some UTs Signed-off-by: Amardeepsingh Siglani <[email protected]> * added more UTs Signed-off-by: Amardeepsingh Siglani <[email protected]> * refactored code to address comments Signed-off-by: Amardeepsingh Siglani <[email protected]> * minor updates Signed-off-by: Amardeepsingh Siglani <[email protected]> * minor updates Signed-off-by: Amardeepsingh Siglani <[email protected]> * updated tests Signed-off-by: Amardeepsingh Siglani <[email protected]> * fixed UT Signed-off-by: Amardeepsingh Siglani <[email protected]> --------- Signed-off-by: Amardeepsingh Siglani <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

github-actions bot added the valued-contributor label Nov 12, 2024

opensearch-changeset-bot bot added a commit to amsiglan/OpenSearch-Dashboards that referenced this pull request Nov 12, 2024

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

985dadb

virajsanghvi reviewed Nov 12, 2024

View reviewed changes

ashwin-pc reviewed Nov 12, 2024

View reviewed changes

amsiglan added 5 commits November 13, 2024 14:49

removed unwanted changes

95dde3e

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

fixed linter errors; failing UT

1cc8693

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

added some UTs

98a405f

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

added more UTs

99d9633

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

refactored code to address comments

502cdab

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

amsiglan force-pushed the indexed-views-redesigned branch from 488dd43 to 502cdab Compare November 13, 2024 22:49

kavilla reviewed Nov 13, 2024

View reviewed changes

src/plugins/data/public/ui/query_editor/_query_editor.scss Outdated Show resolved Hide resolved

kavilla reviewed Nov 13, 2024

View reviewed changes

src/plugins/data/public/ui/dataset_selector/configurator.tsx Show resolved Hide resolved

kavilla reviewed Nov 13, 2024

View reviewed changes

src/plugins/data/public/ui/query_editor/query_editor_extensions/query_editor_extensions.tsx Show resolved Hide resolved

virajsanghvi reviewed Nov 14, 2024

View reviewed changes

kavilla reviewed Nov 14, 2024

View reviewed changes

src/plugins/data/public/ui/dataset_selector/configurator.tsx Show resolved Hide resolved

kavilla reviewed Nov 14, 2024

View reviewed changes

src/plugins/data/public/ui/dataset_selector/configurator.tsx Outdated Show resolved Hide resolved

minor updates

1d73abd

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

virajsanghvi reviewed Nov 14, 2024

View reviewed changes

amsiglan added 2 commits November 13, 2024 17:18

minor updates

98545d7

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

updated tests

d9bbd23

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

kavilla reviewed Nov 14, 2024

View reviewed changes

kavilla previously approved these changes Nov 14, 2024

View reviewed changes

kavilla reviewed Nov 14, 2024

View reviewed changes

virajsanghvi previously approved these changes Nov 14, 2024

View reviewed changes

fixed UT

1057f9e

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

amsiglan dismissed stale reviews from virajsanghvi and kavilla via 1057f9e November 14, 2024 02:53

virajsanghvi approved these changes Nov 14, 2024

View reviewed changes

ashwin-pc approved these changes Nov 14, 2024

View reviewed changes

ashwin-pc merged commit 413697d into opensearch-project:main Nov 14, 2024
66 of 67 checks passed

amsiglan mentioned this pull request Nov 20, 2024

Backport 8896 and 8851 to 2.x #8901

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Indexed views framework #8851

Indexed views framework #8851

amsiglan commented Nov 12, 2024

codecov bot commented Nov 12, 2024 •

edited

Loading

virajsanghvi left a comment

virajsanghvi left a comment

virajsanghvi Nov 12, 2024

kavilla Nov 12, 2024

virajsanghvi Nov 13, 2024

virajsanghvi Nov 12, 2024

ashwin-pc left a comment

kavilla Nov 13, 2024

virajsanghvi Nov 13, 2024 •

edited

Loading

kavilla Nov 14, 2024

virajsanghvi left a comment

virajsanghvi Nov 13, 2024 •

edited

Loading

virajsanghvi Nov 14, 2024

amsiglan Nov 14, 2024

kavilla Nov 14, 2024

kavilla Nov 14, 2024

amsiglan Nov 14, 2024

virajsanghvi left a comment

virajsanghvi left a comment

ashwin-pc commented Nov 14, 2024

Indexed views framework #8851

Indexed views framework #8851

Conversation

amsiglan commented Nov 12, 2024

Description

Issues Resolved

Screenshot

Note: Below screenshots were produced by making changes to the S3 type config for testing and are not part of this PR since this PR is only adding the framework.

Testing the changes

Changelog

Check List

codecov bot commented Nov 12, 2024 • edited Loading

Codecov Report

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

ashwin-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virajsanghvi Nov 13, 2024 • edited Loading

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 Nov 13, 2024 • edited Loading

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

virajsanghvi left a comment

Choose a reason for hiding this comment

virajsanghvi left a comment

Choose a reason for hiding this comment

ashwin-pc commented Nov 14, 2024

codecov bot commented Nov 12, 2024 •

edited

Loading

virajsanghvi Nov 13, 2024 •

edited

Loading

virajsanghvi Nov 13, 2024 •

edited

Loading