[Data Explorer][Discover 2.0] Implement data fetch, index pattern, saved search for table vis #4564

ananzh · 2023-07-13T16:01:04Z

Description

Fetch data and feed to table vis by deangularizing controller methods
Transfer legacy table to data grid Table
Remove doc view links for now

Issues Resolved

#4224

Screenshot

Testing the changes

Check List

codecov · 2023-07-13T18:13:47Z

Codecov Report

Merging #4564 (84bb885) into feature/data-explorer (6e87e28) will increase coverage by 0.02%.
The diff coverage is 78.94%.

❗ Current head 84bb885 differs from pull request most recent head 859432b. Consider uploading reports for the commit 859432b to get more accurate results

@@                    Coverage Diff                    @@
##           feature/data-explorer    #4564      +/-   ##
=========================================================
+ Coverage                  66.58%   66.60%   +0.02%     
=========================================================
  Files                       3278     3282       +4     
  Lines                      62645    62732      +87     
  Branches                    9759     9780      +21     
=========================================================
+ Hits                       41710    41782      +72     
- Misses                     18569    18574       +5     
- Partials                    2366     2376      +10

Flag	Coverage Δ
Linux	`66.54% <78.94%> (+0.02%)`	⬆️
Windows	`66.54% <78.94%> (+0.02%)`	⬆️

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

Files Changed	Coverage Δ
...omponents/data_grid/data_grid_table_cell_value.tsx	`70.83% <70.83%> (ø)`
...ins/discover/public/__mock__/index_pattern_mock.ts	`75.00% <75.00%> (ø)`
...blic/application/view_components/utils/get_sort.ts	`88.88% <88.88%> (ø)`
...lic/application/components/utils/use_pagination.ts	`90.00% <90.00%> (ø)`

... and 2 files with indirect coverage changes

ashwin-pc

The PR looks good to me, just one blocking change to add the doc_viewer_links service back. Unit tests would be really appreciated since we will need them before merge to main anyways.

ashwin-pc · 2023-07-20T23:42:27Z

src/plugins/discover/public/application/components/data_grid/constants.ts

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+export const toolbarVisibility = {


Suggested change

export const toolbarVisibility = {

export const TOOLBAR_VISIBILITY = {

nit:

will change

ashwin-pc · 2023-07-20T23:51:51Z

src/plugins/discover/public/application/components/data_grid/data_grid_table.scss

do we need all this custom css?

ashwin-pc · 2023-07-20T23:56:45Z

src/plugins/discover/public/application/doc_views_links/doc_views_links_registry.ts

As we discussed earlier, lets add this back to the app. Its an api we expose that is not used in our project but is used by the community

will add it back

ashwin-pc · 2023-07-20T23:59:32Z

src/plugins/discover/public/application/components/data_grid/data_grid_table_cell_value.tsx

Can you add a unit test for this file? it looks like it wont change in future.

Will add an initial unit test for now. This data_grid_table_cell_value.tsx will need further polish to include nested fields and others.

ashwin-pc · 2023-07-21T00:00:04Z

src/plugins/discover/public/application/components/data_grid/data_grid_table_columns.tsx

The same goes for this. Unit tests will be useful here

will add a unit test

ashwin-pc · 2023-07-21T00:10:03Z

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

+  const { core, chrome, data, uiSettings: config, toastNotifications } = services;
+  const [savedSearch, setSavedSearch] = useState<SavedSearch>();
+  const [indexPattern, setIndexPattern] = useState<IndexPattern | undefined>(undefined);
+  // ToDo: get id from data explorer since it is handling the routing logic


nit: I usually have my case sensitivity turned on so i dont want to accidentally miss this

Suggested change

// ToDo: get id from data explorer since it is handling the routing logic

// TODO: get id from data explorer since it is handling the routing logic

will replace all ToDo to TODO

ashwin-pc · 2023-07-21T00:12:06Z

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

+    };
+  }, [data$, fetchState]);
+
+  // ToDo: implement columns, onAddColumn, onRemoveColumn, onMoveColumn, onSetColumns using config, indexPattern, appState


most of these are already done in my side panel PR so i'd hold off implementing this until then :)

ashwin-pc · 2023-07-21T00:13:14Z

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

+      <EuiPage className="dscCanvasAppPage">
+        <EuiPageBody className="dscCanvasAppPageBody">


Do we need EuiPage and EuiPageBody? We already have them at the app root level

will remove

ashwin-pc · 2023-07-21T00:17:21Z

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

+  indexPattern: IndexPattern;
+}
+
+export const DiscoverTableService = ({


Do we need this? Right now the React tree looks like:

Canvas -> DiscoverTable -> DiscoverTableService -> DiscoverTableApplication -> DataGrid

Cant we instead simplify this to just be

Canvas -> DiscoverTable -> DataGrid?

This DiscoverTableApplication actually need to rename. It will include DiscoverChart (HitsCount, TimeChartHeader and Histogram) and DiscoverTable.
This discover_table_service is used to have any updates on services, like data & saved search for both DiscoverChart and DiscoverTable.

We could re-evaluate and simply after canvas parts all implemented.

ashwin-pc · 2023-07-21T00:20:33Z

...gins/discover/public/application/view_components/canvas/utils/use_discover_canvas_service.ts

All files inside utils need to have unit tests. I wont hold back the PR if they arent there, but before we merge to main, we definitely need to add them

Yes and will do. Currently, use_discover_canvas_service is not done. We will add tests for every utility functions after canvas is done.

Issue Resolve opensearch-project#4442 Signed-off-by: ananzh <ananzh@amazon.com>

Issue Resolve: opensearch-project#4397 Signed-off-by: ananzh <ananzh@amazon.com>

Signed-off-by: ananzh <ananzh@amazon.com>

ananzh · 2023-07-25T17:29:01Z

@ashwin-pc fixed all your comments. Some major changes are 1) add some initial tests. I put index pattern mock into a common mock folder bc it will be used by many other comps. Currently dataRowsMock is in each test file, but if there is more duplication, we will also move it to common folder. 2) add doc_views_link back. will see how to use it better and verify these links work in later work.

ashwin-pc

Wont block on the new index pattern mock, but in a followup PR can you replace the custom mock with an existing one so that we dont have many ways to mock the same thing. Index patterns may change soon and custom mocks like this will make it annoying to update tests that rely on them.

ashwin-pc · 2023-07-26T20:19:30Z

src/plugins/discover/public/__mock__/index_pattern_mock.ts

Do we need this? Cant we use import { getStubIndexPattern } from '../../../../../../data/public/test_utils'; like the one found in src/plugins/discover/public/application/components/sidebar/lib/field_calculator.test.ts?

Yes we could. This is a great finding. I think we just need to add a new data in fixture. Let me do it in a follow up PR.

github-actions bot added the distinguished-contributor label Jul 13, 2023

ananzh force-pushed the feature/data-explorer-fetch branch 4 times, most recently from 99f3371 to f3d17ed Compare July 18, 2023 17:08

ananzh marked this pull request as ready for review July 18, 2023 17:12

ananzh requested review from kavilla, seanneumann, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, kristenTian, zhongnansu, manasvinibs, ZilongX and Flyingliuhub as code owners July 18, 2023 17:12

ananzh mentioned this pull request Jul 18, 2023

[Data Explorer] Implement table using table gird and remove doc_viewer_link #4458

Closed

ananzh assigned ashwin-pc Jul 18, 2023

ashwin-pc reviewed Jul 21, 2023

View reviewed changes

ananzh added 2 commits July 24, 2023 21:04

Implement table using table gird and remove doc_viewer_link

a7aa8a1

Issue Resolve opensearch-project#4442 Signed-off-by: ananzh <ananzh@amazon.com>

[Data Explorer] Implement data fetch logic in Discover 2.0

afa8081

Issue Resolve: opensearch-project#4397 Signed-off-by: ananzh <ananzh@amazon.com>

ashwin-pc added discover for discover reinvent de-angular de-angularize work data explorer Issues related to the Data Explorer project labels Jul 25, 2023

ananzh force-pushed the feature/data-explorer-fetch branch from f3d17ed to 84bb885 Compare July 25, 2023 17:24

fix comments

859432b

Signed-off-by: ananzh <ananzh@amazon.com>

ananzh force-pushed the feature/data-explorer-fetch branch from 84bb885 to 859432b Compare July 25, 2023 17:28

ashwin-pc approved these changes Jul 26, 2023

View reviewed changes

ananzh merged commit 146cc36 into opensearch-project:feature/data-explorer Jul 26, 2023
20 of 37 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Data Explorer][Discover 2.0] Implement data fetch, index pattern, saved search for table vis #4564

[Data Explorer][Discover 2.0] Implement data fetch, index pattern, saved search for table vis #4564

ananzh commented Jul 13, 2023 •

edited

Loading

codecov bot commented Jul 13, 2023 •

edited

Loading

ashwin-pc left a comment

ashwin-pc Jul 20, 2023

ananzh Jul 24, 2023

ashwin-pc Jul 20, 2023

ashwin-pc Jul 20, 2023

ananzh Jul 24, 2023

ashwin-pc Jul 20, 2023

ananzh Jul 25, 2023

ashwin-pc Jul 21, 2023

ananzh Jul 25, 2023

ashwin-pc Jul 21, 2023

ananzh Jul 25, 2023

ashwin-pc Jul 21, 2023

ashwin-pc Jul 21, 2023

ananzh Jul 25, 2023

ashwin-pc Jul 21, 2023

ananzh Jul 25, 2023

ashwin-pc Jul 21, 2023

ananzh Jul 25, 2023

ananzh commented Jul 25, 2023

ashwin-pc left a comment

ashwin-pc Jul 26, 2023

ananzh Jul 26, 2023

	export const toolbarVisibility = {
	export const TOOLBAR_VISIBILITY = {

	// ToDo: get id from data explorer since it is handling the routing logic
	// TODO: get id from data explorer since it is handling the routing logic

		<EuiPage className="dscCanvasAppPage">
		<EuiPageBody className="dscCanvasAppPageBody">

[Data Explorer][Discover 2.0] Implement data fetch, index pattern, saved search for table vis #4564

[Data Explorer][Discover 2.0] Implement data fetch, index pattern, saved search for table vis #4564

Conversation

ananzh commented Jul 13, 2023 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Check List

codecov bot commented Jul 13, 2023 • edited Loading

Codecov Report

ashwin-pc 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

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

ananzh commented Jul 25, 2023

ashwin-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ananzh commented Jul 13, 2023 •

edited

Loading

codecov bot commented Jul 13, 2023 •

edited

Loading