[Table Visualization] Replace table visualization with React and DataGrid

[ananzh]

Description

In this PR, we add back functions to make new table usage to be consistent with the replaced one.

• total function
• percentage column
• filter in/out

Meanwhile, we also add back server. Functional tests are removed. We will add new functional test in opensearch-dashboards-functional-test repo. We also clean out some legacy codes.

Issues Resolved

#2855

Need Review

Some of the files have been reviewed here. To avoid duplicate work, I summarized all the un-reviewed PRs. There are two main places, src/plugins and test/functional.

src/plugins

data/common/field_formats

Need review.

vis_builder/public/visualizatons/public/visualizations/table

2 files need review.

• to_expression.ts: some re-name
• common/table_viz_options.tsx: clean

vis_type_table

I list all the files that requires review.

• README.md
• opensearch_dashboards.json
• public
  • index.ts
  • plugin.ts: add back Visualizations and PluginInitializerContext
  • table_vis_fn.ts: rename
  • table_vis_type.ts
  • to_ast.ts
  • types.ts: the only diff is adding back AggTypes
  • utils
    • convert_to_formatted_data.ts: add back percentage close
  • components
    • table_vis_grid_columns.tsx: filter for/filter in data features. They are the plus and minus circle when you click the data.

visualizations/public/legacy

Everything here needs review. It is to clean out legacy pipeline.

test/functional

Everything in test/functional should be reviewed. These are the table vis related functional tests and related common functions. We have create an issue to document all of them. They will be recreated in another repo.
There are four parts in the test/functional.

• Apps
  Applications have two parts, dashboard and visualize. Visualize are most pure table vis tests. Dashboard part is about use table vis and test it functions well in dashboard.

• Fixtures
  Data.json.gz is modified and table related data is all removed. (Optional. It is hard to review. But you are welcome to unzip and compare.)

• Page_objects
  Most are functions that are used in dashboard page.

• Services
  Remove all table related services.

Check List

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

[codecov-commenter]

Codecov Report

Merging #2863 (81a5c9f) into main (0ec0712) will decrease coverage by 0.28%.
The diff coverage is 76.47%.

@@            Coverage Diff             @@
##             main    #2863      +/-   ##
==========================================
- Coverage   66.67%   66.39%   -0.29%     
==========================================
  Files        3220     3199      -21     
  Lines       61580    61235     -345     
  Branches     9445     9428      -17     
==========================================
- Hits        41061    40654     -407     
- Misses      18273    18332      +59     
- Partials     2246     2249       +3     

Flag	Coverage Δ
Linux	66.39% <76.47%> (-0.24%)	⬇️
Windows	?

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

Impacted Files	Coverage Δ
.../plugins/data/common/field_formats/field_format.ts	80.64% <ø> (ø)
...ualizations/table/components/table_viz_options.tsx	0.00% <ø> (ø)
...ilder/public/visualizations/table/to_expression.ts	4.65% <ø> (ø)
.../vega_inspector/components/inspector_data_grid.tsx	5.12% <0.00%> (ø)
...ins/visualizations/public/legacy/build_pipeline.ts	46.28% <ø> (-2.64%)	⬇️
src/plugins/vis_type_table/public/to_ast.ts	91.66% <91.66%> (ø)
src/plugins/vis_type_table/public/table_vis_fn.ts	100.00% <100.00%> (ø)
...arch_dashboards_legacy/public/paginate/paginate.js	0.00% <0.00%> (-89.80%)	⬇️
...c/plugins/data/common/search/aggs/buckets/range.ts	26.31% <0.00%> (-68.43%)	⬇️
...ins/data/public/field_formats/utils/deserialize.ts	20.00% <0.00%> (-60.00%)	⬇️
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[ashwin-pc]

I think you need to rebase the changelog. This will wipe out all the old changelog information

[ananzh]

For the CHANGELOG, I specifically removed all the old terms, because it is under [Unrelease] section. I only kept the ones not being released in 2.4. I m not sure whether I should submit a different PR for just cleaning CHANGELOG?

[ananzh]

will restore old CHANGELOG to avoid confusion.

[kavilla]

Missing dco sign off

[tmarkley]

I know the functional tests are going to be addressed in a follow-up, but can we at least add unit tests?

[tmarkley]

Suggestion: isNumericalAggregationAllowed

[ananzh]

yeah it is not looking good. but the original table is using this name here and so do percent, bytes and some other format files. I think I will just use this name for now.

[joshuarrrr]

I also prefer tmarkley's suggestion, but agree that it's out of scope for this PR.

[tmarkley]

Couldn't we do something slightly simpler like:

return columns.find((col) => col.id === columnId)?.formattedTotal || null;

[ananzh]

yeah nice. will change. thx.

[tmarkley]

Do you think it'd be worthwhile to maintain a map of id->column pairs so that we don't have to iterate through the entire list of columns every time we need to find a specific column? I'm not super familiar with this component but considering how many times find is used for columns I wanted to discuss that.

[tmarkley]

Also, why not add the initial width property to the formatted column object as well?

[ananzh]

For the first question, yeah, I think @joshuarrrr also suggest the same. I have opened a research issue here. The reason is that column id might change. For example, when we add/remove a column, id will change. Therefore, when we format the column, we might fail to find the column if use id. But I agree it is pain. After done the functional and unit test, I will re-visit this enhancement issue.

[ananzh]

For the second question, not quite sure if my understand of the question is correct. We are changing the formatted col to data grid formatted col here, since EuiDataGrid component requests EuiDataGridColumn format. EuiDataGridColumn need to have the width info to display col in correct width. In EuiDataGridColumn, InitialWidth is to store the col width and formatted col doesn't need to store the info.

[tmarkley]

Minor grammar: columnWidths

[ananzh]

will change.

[tmarkley]

This function is pretty meaty.

1. Can it be simplified and/or broken up in any way to make it more readable?
2. Can we add any unit tests to cover all of these conditionals?

[ananzh]

Yeah. They are actually from the original angular code here.

We will add some functional tests for this part. I document all the functional tests here and will add these in opensearch-dashboards-functional-test repo. The split table section in the functional test issue will test whether table can be split in rows and columns and the values in the split tables.

[tmarkley]

I'm glad you are adding functional tests, but I think this needs unit tests as well.

[ananzh]

Yeah functionality is a concern for everyone to review such large PR.
Functional tests in original OSD are removed and the tests related to table vis are re-created here:
opensearch-project/opensearch-dashboards-functional-test#423

Currently, some tests are failed because we need this table vis to hook up to OSD.
Once table is added in OSD and functionalities are all good, I will start adding unit tests. I also opened a tracking issue.

[ashwin-pc]

I second @tmarkley's comment about adding a unit test for this function. Looks like a pretty straightforward function with a decent amount of logic.

[ananzh]

Yes. I have a separate unit test issue and will create all the unit tests.
Here is the issue: #2856.

[ananzh]

Missing dco sign off

added it. thx

[joshuarrrr]

OK, halfway done, everything looks good so far! Mostly just a few clarifying questions and some formatting and readability tweaks to the readme. I think I've reviewed everything outside of vis_type_table.

Thanks again for the reviewer guide in the description - it's a super helpful reference point.

[joshuarrrr]

I also prefer tmarkley's suggestion, but agree that it's out of scope for this PR.

[joshuarrrr]

huh, interesting that it only tested embedding a data table. Has this test also been migrated to the functional test repo, even though it's outside of testing the table vis itself?

[ananzh]

It hasn't yet. But I am working on it.
Currently, all the table functions and tests on visualization page are re-produced. Things left:

1. embedding a table (_embedding_chart.js)
2. some aggregation tests via table vis if there is no better way to test (_histogram_request_start.js)
3. add table to dashboard page
4. saved search from discover

[joshuarrrr]

cool, yeah, as long as we're tracking tests that still require migration somewhere.

[joshuarrrr]

were these tests also migrated to the functional test repo, even though they're not tests for the table vis itself?

[ananzh]

It hasn't yet. But I am working on it.
Currently, all the table functions and tests on visualization page are re-produced. Things left:

1. embedding a table (_embedding_chart.js)
2. some aggregation tests via table vis if there is no better way to test (_histogram_request_start.js)
3. add table to dashboard page
4. saved search from discover

[joshuarrrr]

were these tests also migrated?

[ananzh]

yeah would like to re-produce the saved search tests. It doesn't have to be table vis. For example, we could use metrics vis. First of all, I see there are a lot of detailed concepts been tested and I don't think metrics can reproduce all of them. Secondly, we are trying to ship all the tests to cypress. I think I could reproduce more of these tests. Of course, another way to find another vis type, re-design some tests and use this one as a reference.

[joshuarrrr]

Is it necessary to delete this test? It seems we still have a table_vis_fn.ts file, so updating it would likely be preferable.

[ananzh]

Are we going to re-create all unit tests via react-test-library?
Yah, I think before I work on the new unit tests, I could add back and modify this one.

[joshuarrrr]

Yeah, while modernizing unit tests is a good goal, we should still just update existing tests as needed.

[joshuarrrr]

I'm curious - why did this change?

[ananzh]

I separate TableVisConfig from TableVisParams and define TableVisConfig as

export interface TableVisConfig extends TableVisParams {
  title: string;
  metrics: SchemaConfig[];
  buckets: SchemaConfig[];
  splitRow?: SchemaConfig[];
  splitColumn?: SchemaConfig[];
}

I think it is easier than using nested params.

[joshuarrrr]

This is probably fine for now, but in general we'll want to standardize these configs. Agreed that the way dimensions are handled in general could use some streamlining. So happy to fix that as part of #2840 .

[joshuarrrr]

What does this do?

[ananzh]

I don't see any specific usage. Maybe there is something that I missed. It seems to me just an alias of data returned when use expression to fetch data from data plugin.

data: 
  as: "table_vis"
  params: {listenOnChange: true}
  type: "render"
  value: {visData: {…}, visType: 'table', visConfig: {…}}

The reason I changed it is just to make it consistent with metrics (metric_vis)and markdown (markdown_vis)

[joshuarrrr]

Oh - it took me a little while to figure it out, but the reason you needed to change this is that you introduced and register a new table-specific rendering expression function, instead of using the visualization rendering function. I think I missed it because https://github.com/opensearch-project/OpenSearch-Dashboards/blob/81a5c9f47e71bbac37dbafa7016357282f8cbf3e/src/plugins/vis_type_table/public/table_vis_renderer.tsx was marked as simply renamed (but was introduced as part of the new table vis.

[joshuarrrr]

Unit test coverage for this fn would be useful.

[ananzh]

Yes. I have a separate unit test issue and will create all the unit tests.
Here is the issue: #2856.

[ananzh]

will add a unit test for this file

[joshuarrrr]

this .slice could benefit from an explanatory statement.

[ananzh]

will add some comments here

[joshuarrrr]

Ideally we'd also have a test case in the unit test file (now that it exists) to validate this behavior 😁

[ashwin-pc]

The changes look good! I dont have many additional comments over what @tmarkley and @joshuarrrr have already said. I've also pulled this down and played with it a bit and the feature looks good! Great job with the port @ananzh!

[ashwin-pc]

Can you write a unit test for this function too? You can reference src/plugins/vis_type_metric/public/to_ast.test.ts since they are quite similar

[ananzh]

Yes. I have a separate unit test issue and will create all the unit tests.
Here is the issue: #2856.

[ananzh]

I will add a unit test for this to_ast.ts

[ashwin-pc]

This file seems to have some edits made to it while it was moved. I also dont see any unit tests for this. Can we add one for this and the other files under utils?

[ananzh]

Yes. I have a separate unit test issue and will create all the unit tests.
Here is the issue: #2856.

[joshuarrrr]

@ananzh Does this structural change make sense to you? where params has moved up a level?

[ananzh]

@joshuarrrr thanks for checking this. I removed listenOnChange in the updated commit. I used UiState initially and listenOnChange is used to force re-render. Then to hookup to visBuilder, I switched to global state. test snap is also updated.

[joshuarrrr]

Ideally we'd also have a test case in the unit test file (now that it exists) to validate this behavior 😁

[joshuarrrr]

looks good overall - just awaiting automation to complete.

[joshuarrrr]

I'd still prefer to restore and update table_vis_fn.test.ts, but not a blocker.

[joshuarrrr]

Oh - it took me a little while to figure it out, but the reason you needed to change this is that you introduced and register a new table-specific rendering expression function, instead of using the visualization rendering function. I think I missed it because https://github.com/opensearch-project/OpenSearch-Dashboards/blob/81a5c9f47e71bbac37dbafa7016357282f8cbf3e/src/plugins/vis_type_table/public/table_vis_renderer.tsx was marked as simply renamed (but was introduced as part of the new table vis.

[joshuarrrr]

Just double checking I'm understanding this change correctly...

Because the previous table relied on the visualization render function, this param was necessary, because eventually that render function uses the VisualizationChart component, where the listenOnChange is used to enable re-renders based on updates to uiState:

OpenSearch-Dashboards/src/plugins/visualizations/public/components/visualization_chart.tsx

Lines 118 to 120 in 0ec0712

	if (this.props.listenOnChange) {
	this.props.uiState.on('change', this.onUiStateChanged);
	}

It's not needed in the new implementation, because your new TableVisComponent is reactive to changes in uiState by default.

[joshuarrrr]

There are a lot of non-null assertions here: vis.data.indexPattern, vis.data.indexPattern.id, vis.data.aggs. Where do we actually catch, handle or enforce that these will always be defined?

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2863-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 9eedb8ee5f5ed5195dab43198be2d519adba55fb
# Push it to GitHub
git push --set-upstream origin backport/backport-2863-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2863-to-2.x.

[joshuarrrr]

manually backported: #3198