fix(sqllab): perf regression on #21532 refactor #21632

justinpark · 2022-09-28T21:49:48Z

SUMMARY

This commit fixes the rendering perf regression from #21532

Since SqlLab consists of heavy rendering components, accessing the entire current queryEditor object can occur re-rendering on unnecessary children components.

https://github.com/apache/superset/pull/21532/files#diff-dac1ba995e65dfae7c9aa7a0a794036ace1eb83814e2101d354752da2bb41dfdR166-R172

This commit removes the currentQueryEditor selector and moved into AceEditorWrapper where the actual values are consumed.
This commit also introduces useQueryEditor hook which supports a syntax validation that helps to optimize the queryEditor value access.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

type.validation.mov

TESTING INSTRUCTIONS

npm run test

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

cc: @EugeneTorap @zhaoyongjie

codecov · 2022-09-28T22:48:42Z

Codecov Report

Merging #21632 (9ef8499) into master (5ea9249) will decrease coverage by 0.00%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##           master   #21632      +/-   ##
==========================================
- Coverage   65.55%   65.54%   -0.01%     
==========================================
  Files        1798     1799       +1     
  Lines       68793    68814      +21     
  Branches     7326     7332       +6     
==========================================
+ Hits        45095    45106      +11     
- Misses      21823    21828       +5     
- Partials     1875     1880       +5

Flag	Coverage Δ
javascript	`53.15% <90.00%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
superset-frontend/src/SqlLab/reducers/sqlLab.js	`35.22% <ø> (ø)`
...d/src/SqlLab/components/AceEditorWrapper/index.tsx	`58.22% <50.00%> (-0.75%)`	⬇️
...frontend/src/SqlLab/components/SaveQuery/index.tsx	`76.74% <80.00%> (ø)`
...d/src/SqlLab/components/QueryLimitSelect/index.tsx	`100.00% <100.00%> (ø)`
...c/SqlLab/components/RunQueryActionButton/index.tsx	`72.72% <100.00%> (-0.81%)`	⬇️
...d/src/SqlLab/components/ShareSqlLabQuery/index.tsx	`80.76% <100.00%> (-1.99%)`	⬇️
...frontend/src/SqlLab/components/SqlEditor/index.jsx	`53.84% <100.00%> (-0.51%)`	⬇️
...d/src/SqlLab/components/SqlEditorLeftBar/index.tsx	`52.23% <100.00%> (-2.05%)`	⬇️
...-frontend/src/SqlLab/hooks/useQueryEditor/index.ts	`100.00% <100.00%> (ø)`
...ugins/plugin-chart-echarts/src/Gauge/buildQuery.ts	`66.66% <0.00%> (-8.34%)`	⬇️
... and 15 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

EugeneTorap · 2022-09-29T16:19:00Z

LGTM! Good fix to prevent re-rendering AceEditorWrapper.
I've prepared a #21650 PR for others SqlEditor child components to use queryEditorId.

michael-s-molina · 2022-09-30T11:41:19Z

superset-frontend/src/SqlLab/hooks/useQueryEditor/useQueryEditor.test.ts

+const middlewares = [thunk];
+const mockStore = configureStore(middlewares);
+
+describe('useQueryEditor', () => {


Can you avoid nesting? You can just use test for each test case in the file.

michael-s-molina · 2022-09-30T11:56:00Z

superset-frontend/src/SqlLab/hooks/useQueryEditor/index.ts

+import { shallowEqual, useSelector } from 'react-redux';
+import { SqlLabRootState, QueryEditor } from 'src/SqlLab/types';
+
+export default function useQueryEditor<T extends keyof QueryEditor>(


I love this hook! Great improvement to code readability.

zhaoyongjie

LGTM, work as expected. One suggestion for this part is that should disable the Run button when the selection area is 1) empty or 2) beginning a comment(--) of SQL and only 1 line.

selection.in.SQLLab.mov

- set disable for comments only

justinpark · 2022-09-30T17:05:37Z

LGTM, work as expected. One suggestion for this part is that should disable the Run button when the selection area is 1) empty or 2) beginning a comment(--) of SQL and only 1 line.

selection.in.SQLLab.mov

@zhaoyongjie I made changes for this improvement

run.selection.mov

zhaoyongjie · 2022-10-03T03:00:48Z

LGTM, work as expected. One suggestion for this part is that should disable the Run button when the selection area is 1) empty or 2) beginning a comment(--) of SQL and only 1 line.
selection.in.SQLLab.mov

@zhaoyongjie I made changes for this improvement
run.selection.mov

Thanks for tackling the long-term UX issue!

…#21632)" This reverts commit 8d1b7ec.

pull-request-size bot added the size/M label Sep 28, 2022

fix(sqllab): perf regression on apache#21532 refactor

6986c32

justinpark force-pushed the fix--perf-regression-on-sqllab-editor branch from afb6c4d to 0d6638e Compare September 29, 2022 20:36

pull-request-size bot added size/L and removed size/M labels Sep 29, 2022

justinpark added 2 commits September 29, 2022 20:35

add useQueryEditor hook to simplify accessing updated queryEditor values

c7dd91a

minor bug fix on SET_ACTIVE_QUERY_EDITOR

b8d18f2

justinpark force-pushed the fix--perf-regression-on-sqllab-editor branch from cb4100b to b8d18f2 Compare September 30, 2022 03:35

michael-s-molina reviewed Sep 30, 2022

View reviewed changes

michael-s-molina requested a review from zhaoyongjie September 30, 2022 11:54

michael-s-molina reviewed Sep 30, 2022

View reviewed changes

zhaoyongjie approved these changes Sep 30, 2022

View reviewed changes

justinpark added 3 commits September 30, 2022 09:19

update type on SaveQuery

7e90729

avoid nested scope in test

e694b97

- migrate RunQuery button using useQueryEditor

9ef8499

- set disable for comments only

pull-request-size bot added size/XL and removed size/L labels Sep 30, 2022

zhaoyongjie merged commit 8d1b7ec into apache:master Oct 3, 2022

sadpandajoe added a commit to preset-io/superset that referenced this pull request Oct 17, 2022

Revert "fix(sqllab): perf regression on apache#21532 refactor (apache…

6a153fe

…#21632)" This reverts commit 8d1b7ec.

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sqllab): perf regression on #21532 refactor #21632

fix(sqllab): perf regression on #21532 refactor #21632

justinpark commented Sep 28, 2022 •

edited

Loading

codecov bot commented Sep 28, 2022 •

edited

Loading

EugeneTorap commented Sep 29, 2022

michael-s-molina Sep 30, 2022

justinpark Sep 30, 2022

michael-s-molina Sep 30, 2022

zhaoyongjie left a comment •

edited

Loading

justinpark commented Sep 30, 2022

zhaoyongjie commented Oct 3, 2022

fix(sqllab): perf regression on #21532 refactor #21632

fix(sqllab): perf regression on #21532 refactor #21632

Conversation

justinpark commented Sep 28, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Sep 28, 2022 • edited Loading

Codecov Report

EugeneTorap commented Sep 29, 2022

michael-s-molina Sep 30, 2022

Choose a reason for hiding this comment

justinpark Sep 30, 2022

Choose a reason for hiding this comment

michael-s-molina Sep 30, 2022

Choose a reason for hiding this comment

zhaoyongjie left a comment • edited Loading

Choose a reason for hiding this comment

justinpark commented Sep 30, 2022

zhaoyongjie commented Oct 3, 2022

justinpark commented Sep 28, 2022 •

edited

Loading

codecov bot commented Sep 28, 2022 •

edited

Loading

zhaoyongjie left a comment •

edited

Loading