[Vis Builder] Add metric to metric, bucket to bucket aggregation persistence #3495

abbyhu2000 · 2023-02-24T20:04:13Z

Description

This PR implements the aggregation persistence feature in Vis Builder when switching vis type.

Detailed research doc can be found here: #2900
The proposal, mapping rules and examples can be found here: #3482

Issues Resolved

#3482

Check List

codecov-commenter · 2023-03-01T00:31:17Z

Codecov Report

Merging #3495 (42a7684) into main (69bcbfe) will decrease coverage by 0.01%.
The diff coverage is 70.58%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3495      +/-   ##
==========================================
- Coverage   66.45%   66.45%   -0.01%     
==========================================
  Files        3205     3206       +1     
  Lines       61562    61596      +34     
  Branches     9497     9503       +6     
==========================================
+ Hits        40914    40936      +22     
- Misses      18377    18387      +10     
- Partials     2271     2273       +2

Flag	Coverage Δ
Linux	`66.40% <70.58%> (-0.01%)`	⬇️
Windows	`66.40% <70.58%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...uilder/public/application/components/right_nav.tsx	`11.11% <0.00%> (-2.23%)`	⬇️
...plication/utils/state_management/shared_actions.ts	`100.00% <ø> (ø)`
...tion/utils/state_management/visualization_slice.ts	`33.33% <ø> (ø)`
.../application/utils/use/use_persisted_agg_params.ts	`77.41% <77.41%> (ø)`
...s/osd-optimizer/src/node/node_auto_tranpilation.ts	`83.67% <0.00%> (-4.09%)`	⬇️
packages/osd-optimizer/src/node/cache.ts	`51.31% <0.00%> (-2.64%)`	⬇️
...ared/static/forms/hook_form_lib/hooks/use_field.ts	`66.66% <0.00%> (+0.96%)`	⬆️

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

CHANGELOG.md

src/plugins/vis_builder/public/application/components/right_nav.tsx

src/plugins/vis_builder/public/application/utils/state_management/visualization_slice.ts

ashwin-pc

Love how surgical and clean the implementation is.

src/plugins/vis_builder/public/application/utils/state_management/shared_actions.ts

src/plugins/vis_builder/public/application/components/right_nav.tsx

src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.ts

ashwin-pc

These tests are in the right direction but not exactly what i was looking for. The reason i wanted the unit tests was to ensure that the logic you described in the parent issue works as expected, and if that changes, these tests should catch it. I dont see a test here that validates that logic. There are just 3 tests here that i am really looking for:

A test for usePersistedAggParams that given a list of appParams, an old and new vis type can successfully map the params to the new type.
A test to show how the hook would drop values if it did not find a way to map the params successfully. e.g. goes over the max count
Any edge cases that we need to cover. e.g. no schema

The rest of the functions dont really need tests since they are helper functions that help the hook function as expected.

src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.test.tsx

Signed-off-by: abbyhu2000 <[email protected]>

abbyhu2000 · 2023-03-08T21:06:16Z

These tests are in the right direction but not exactly what i was looking for. The reason i wanted the unit tests was to ensure that the logic you described in the parent issue works as expected, and if that changes, these tests should catch it. I dont see a test here that validates that logic. There are just 3 tests here that i am really looking for:

A test for usePersistedAggParams that given a list of appParams, an old and new vis type can successfully map the params to the new type.

A test to show how the hook would drop values if it did not find a way to map the params successfully. e.g. goes over the max count

Any edge cases that we need to cover. e.g. no schema

The rest of the functions dont really need tests since they are helper functions that help the hook function as expected.

@ashwin-pc Rewrote the unit test to test the parent function as a whole under different scenarios.

ashwin-pc

Love the new tests and the feature!

ashwin-pc · 2023-03-08T21:13:34Z

src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.test.tsx

+    };
+  });
+
+  test('return the correct metric-to-metric, bucket-to-bucket mapping and correct persisted aggregations', () => {


ashwin-pc · 2023-03-08T21:19:23Z

src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.test.tsx

+    `);
+  });
+
+  test('aggregations with undefined schema remain undefined; schema will be set to undefined if aggregations that exceeds the max amount', () => {


Nice combining two tests in one

…istence (#3495) * Add metric to metric, bucket to bucket aggregation persistance Signed-off-by: abbyhu2000 <[email protected]> * Add logics to avoid exceeding the max count for each schema field Signed-off-by: abbyhu2000 <[email protected]> * Add changelog entry Signed-off-by: abbyhu2000 <[email protected]> * addressing comments Signed-off-by: abbyhu2000 <[email protected]> * Add unit tests for functions in use_persisted_agg_params Signed-off-by: abbyhu2000 <[email protected]> * rewrite unit tests to test the entire functionality Signed-off-by: abbyhu2000 <[email protected]> --------- Signed-off-by: abbyhu2000 <[email protected]> Co-authored-by: Anan Zhuang <[email protected]> (cherry picked from commit 525b033) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…istence (#3495) (#3594)

…istence (opensearch-project#3495) * Add metric to metric, bucket to bucket aggregation persistance Signed-off-by: abbyhu2000 <[email protected]> * Add logics to avoid exceeding the max count for each schema field Signed-off-by: abbyhu2000 <[email protected]> * Add changelog entry Signed-off-by: abbyhu2000 <[email protected]> * addressing comments Signed-off-by: abbyhu2000 <[email protected]> * Add unit tests for functions in use_persisted_agg_params Signed-off-by: abbyhu2000 <[email protected]> * rewrite unit tests to test the entire functionality Signed-off-by: abbyhu2000 <[email protected]> --------- Signed-off-by: abbyhu2000 <[email protected]> Co-authored-by: Anan Zhuang <[email protected]> Signed-off-by: David Sinclair <[email protected]>

abbyhu2000 added the vis builder label Feb 24, 2023

abbyhu2000 self-assigned this Feb 24, 2023

abbyhu2000 force-pushed the aggregation_persistence branch from d239f2d to 2fdfbdf Compare February 28, 2023 23:18

abbyhu2000 marked this pull request as ready for review February 28, 2023 23:20

abbyhu2000 requested a review from a team as a code owner February 28, 2023 23:20

AMoo-Miki reviewed Mar 1, 2023

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

src/plugins/vis_builder/public/application/components/right_nav.tsx Outdated Show resolved Hide resolved

src/plugins/vis_builder/public/application/utils/state_management/visualization_slice.ts Outdated Show resolved Hide resolved

ashwin-pc reviewed Mar 2, 2023

View reviewed changes

abbyhu2000 force-pushed the aggregation_persistence branch from a0a3e6b to 67feda9 Compare March 2, 2023 20:23

abbyhu2000 mentioned this pull request Mar 3, 2023

[Vis Builder] Persist agg when switching among area, line, histogram vis type #3158

Closed

8 tasks

abbyhu2000 requested a review from ashwin-pc March 6, 2023 17:24

ashwin-pc reviewed Mar 6, 2023

View reviewed changes

abbyhu2000 added 6 commits March 7, 2023 23:58

Add metric to metric, bucket to bucket aggregation persistance

9e047a9

Signed-off-by: abbyhu2000 <[email protected]>

Add logics to avoid exceeding the max count for each schema field

a70d9b8

Signed-off-by: abbyhu2000 <[email protected]>

Add changelog entry

77ea970

Signed-off-by: abbyhu2000 <[email protected]>

addressing comments

1fa0c1f

Signed-off-by: abbyhu2000 <[email protected]>

Add unit tests for functions in use_persisted_agg_params

f4199e4

Signed-off-by: abbyhu2000 <[email protected]>

rewrite unit tests to test the entire functionality

1d32f58

Signed-off-by: abbyhu2000 <[email protected]>

abbyhu2000 force-pushed the aggregation_persistence branch from 42a7684 to 1d32f58 Compare March 8, 2023 21:05

ashwin-pc approved these changes Mar 8, 2023

View reviewed changes

ananzh mentioned this pull request Mar 10, 2023

[Maintenance] PR review tracking @ananzh #3589

Open

Merge branch 'main' into aggregation_persistence

7a0b86d

ananzh approved these changes Mar 10, 2023

View reviewed changes

abbyhu2000 merged commit 525b033 into opensearch-project:main Mar 10, 2023

abbyhu2000 added v2.7.0 backport 2.x labels Mar 10, 2023

opensearch-trigger-bot bot mentioned this pull request Mar 10, 2023

[Backport 2.x] [Vis Builder] Add metric to metric, bucket to bucket aggregation persistence #3594

Merged

abbyhu2000 pushed a commit that referenced this pull request Mar 11, 2023

[Vis Builder] Add metric to metric, bucket to bucket aggregation pers…

27e9fe0

…istence (#3495) (#3594)

abbyhu2000 mentioned this pull request Mar 13, 2023

[Vis builder] Persist data when switching visualization types #2763

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vis Builder] Add metric to metric, bucket to bucket aggregation persistence #3495

[Vis Builder] Add metric to metric, bucket to bucket aggregation persistence #3495

abbyhu2000 commented Feb 24, 2023

codecov-commenter commented Mar 1, 2023 •

edited

Loading

ashwin-pc left a comment

ashwin-pc left a comment

abbyhu2000 commented Mar 8, 2023

ashwin-pc left a comment

ashwin-pc Mar 8, 2023

ashwin-pc Mar 8, 2023

[Vis Builder] Add metric to metric, bucket to bucket aggregation persistence #3495

[Vis Builder] Add metric to metric, bucket to bucket aggregation persistence #3495

Conversation

abbyhu2000 commented Feb 24, 2023

Description

Issues Resolved

Check List

codecov-commenter commented Mar 1, 2023 • edited Loading

Codecov Report

ashwin-pc left a comment

Choose a reason for hiding this comment

ashwin-pc left a comment

Choose a reason for hiding this comment

abbyhu2000 commented Mar 8, 2023

ashwin-pc left a comment

Choose a reason for hiding this comment

ashwin-pc Mar 8, 2023

Choose a reason for hiding this comment

ashwin-pc Mar 8, 2023

Choose a reason for hiding this comment

codecov-commenter commented Mar 1, 2023 •

edited

Loading