fix(sqllab): Replace autocomplete logic by a hook #24677

justinpark · 2023-07-13T00:45:24Z

SUMMARY

Following-up #24611, this commit adds specs for autocomplete regression by refactoring the logic into a dedicate hook.
In addition to the refactoring, this commit also fixes the unnecessary iterations for generating autocomplete items.
This performance improvement enables the autocomplete with a large-size metadata set.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After: (able to interact with autocomplete in a large set)

after--autocomplete-perf.mov

Before: (impossible to interact with autocomplete in a large set)

before--autocomplete-perf.mov

TESTING INSTRUCTIONS

Go to SQL lab and enable autocomplete

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

justinpark · 2023-07-13T00:46:39Z

cc: @sadpandajoe @EugeneTorap

codecov · 2023-07-13T03:45:19Z

Codecov Report

Merging #24677 (e29cfce) into master (1473d97) will decrease coverage by 0.08%.
The diff coverage is 80.52%.

❗ Current head e29cfce differs from pull request most recent head 3ebebf2. Consider uploading reports for the commit 3ebebf2 to get more accurate results

@@            Coverage Diff             @@
##           master   #24677      +/-   ##
==========================================
- Coverage   69.03%   68.96%   -0.08%     
==========================================
  Files        1908     1903       -5     
  Lines       74197    73947     -250     
  Branches     8186     8174      -12     
==========================================
- Hits        51219    50994     -225     
+ Misses      20857    20843      -14     
+ Partials     2121     2110      -11

Flag	Coverage Δ
javascript	`55.77% <70.63%> (+0.05%)`	⬆️

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

Impacted Files	Coverage Δ
...src/components/ColumnTypeLabel/ColumnTypeLabel.tsx	`100.00% <ø> (ø)`
...d/packages/superset-ui-chart-controls/src/index.ts	`100.00% <ø> (ø)`
...-chart-controls/src/sections/advancedAnalytics.tsx	`14.28% <ø> (ø)`
...rset-ui-chart-controls/src/sections/chartTitle.tsx	`100.00% <ø> (ø)`
...legacy-plugin-chart-partition/src/controlPanel.tsx	`25.00% <ø> (ø)`
...gins/legacy-plugin-chart-rose/src/controlPanel.tsx	`50.00% <ø> (ø)`
...gins/legacy-preset-chart-nvd3/src/NVD3Controls.tsx	`100.00% <ø> (ø)`
.../BigNumber/BigNumberWithTrendline/controlPanel.tsx	`16.66% <ø> (ø)`
...s/plugin-chart-echarts/src/Funnel/controlPanel.tsx	`66.66% <ø> (ø)`
...ns/plugin-chart-echarts/src/Gauge/controlPanel.tsx	`66.66% <ø> (ø)`
... and 90 more

... and 13 files with indirect coverage changes

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

john-bodley · 2023-07-13T16:55:42Z

@sadpandajoe can you help us test this?

john-bodley · 2023-07-13T16:55:53Z

/testenv up

github-actions · 2023-07-13T16:56:12Z

@john-bodley Ephemeral environment creation is currently limited to committers.

michael-s-molina · 2023-07-13T17:04:10Z

/testenv up

sadpandajoe · 2023-07-13T17:05:43Z

@sadpandajoe can you help us test this?

Yeah I can probably either later today or tomorrow. We should take a look at writing an e2e test for this.

github-actions · 2023-07-13T17:06:01Z

@michael-s-molina Ephemeral environment spinning up at http://52.13.18.69:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

sadpandajoe · 2023-07-14T15:56:15Z

@michael-s-molina looks like when I try to log into the ephemeral I'm stuck in an infinite log in state.

michael-s-molina · 2023-07-14T16:06:51Z

@michael-s-molina looks like when I try to log into the ephemeral I'm stuck in an infinite log in state.

@eschutho were you able to resolve this issue?

michael-s-molina

Thanks for the PR @justinpark. Left some first-pass comments.

michael-s-molina · 2023-07-19T14:41:18Z

superset-frontend/src/SqlLab/components/AceEditorWrapper/useKeywords.test.ts

+});
+
+test('returns column keywords among selected tables', async () => {
+  const expectDbId = 1;


Could you extract the constants that are reusable by the tests?

michael-s-molina · 2023-07-19T14:51:57Z

superset-frontend/src/SqlLab/components/AceEditorWrapper/useKeywords.ts

+  // skipFetch is used to prevent re-evaluating memoized keywords
+  // due to updated api results by skip flag
+  const skipFetch = hasFetchedKeywords && skip;
+  const { data: schemaOptions } = useSchemas({


We should pass the onError callback and deal with errors when fetching schemas and tables.

The schema error handling already covered at DatabaseSelector

I'm not sure if I understood. I'm assuming this hook is independent of the DatabaseSelector. It's calling useSchemas from src/hooks/apiResources which might fail right? If I use this hook in another component, shouldn't the exception be handled?

You're right. This hook is independent of the DatabaseSelector. However, it shared the query data from DatabaseSelector by using shared useSchemas/useTables hooks. If this hook also add the error handler (which shows the error toast), the same error message pops up twice because that error handler is already implemented in DatabaseSelect which is used in the left sidebar layout.

Why the error handler in DatabaseSelector? There's a benefit of covering all error situations in that component such like new dataset left menu refactor doesn't need to add extra error handling logic for that integration.

To minimize the confusion, let me change useQuery over useQueryState which means only sharing the query data without triggering fetches. It will only useQuery for database_functions since it's the only item that requires fetch for additional autocomplete data

As we discussed offline, let's improve the error handling in a follow-up PR as it may require a more detailed context.

michael-s-molina · 2023-07-19T14:53:20Z

superset-frontend/src/SqlLab/components/AceEditorWrapper/useKeywords.ts

+    }),
+  });
+
+  const { data: functionNames, isError } = useDatabaseFunctionsQuery(


It would be good if these hooks follow the same pattern for error handling. Tables and Schemas have an onError callback. Could you make useDatabaseFunctionsQuery follow the same pattern?

michael-s-molina · 2023-07-19T14:54:27Z

superset-frontend/src/SqlLab/components/AceEditorWrapper/useKeywords.ts

+    { skip: skipFetch || !dbId },
+  );
+
+  useEffect(() => {


We need to account for all resources when handling errors (tables, schemas).

michael-s-molina · 2023-07-19T14:59:53Z

superset-frontend/src/SqlLab/components/AceEditorWrapper/useKeywords.ts

+
+  hasFetchedKeywords.current = !skip;
+
+  return skip ? EMPTY_LIST : keywords;


Can you move the skip check to the beginning of the function to avoid everything else in case it should return an empty list?

It will violate the rule of hooks then.

🤦🏼‍♂️

michael-s-molina · 2023-07-19T17:56:00Z

@justinpark Given the PR description, is it fair to change the PR title from refactor: to fix:?

michael-s-molina

LGTM

github-actions · 2023-07-26T18:25:00Z

Ephemeral environment shutdown and build artifacts deleted.

(cherry picked from commit 7750517)

refactor(sqllab): Replace autocomplete logic by a hook

919ab80

pull-request-size bot added the size/XL label Jul 13, 2023

justinpark requested a review from michael-s-molina July 13, 2023 00:45

add missing fetchMock

460190f

john-bodley requested review from betodealmeida and eschutho July 13, 2023 16:54

justinpark added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jul 18, 2023

michael-s-molina reviewed Jul 19, 2023

View reviewed changes

justinpark changed the title ~~refactor(sqllab): Replace autocomplete logic by a hook~~ fix(sqllab): Replace autocomplete logic by a hook Jul 20, 2023

replace useSchemas and useTables by query state

230a5aa

justinpark force-pushed the refactor--sqllab-autocomplete-by-useKeywords branch from 8c41dde to 230a5aa Compare July 21, 2023 23:09

remove only

3ebebf2

michael-s-molina approved these changes Jul 26, 2023

View reviewed changes

justinpark merged commit 7750517 into apache:master Jul 26, 2023
26 checks passed

michael-s-molina pushed a commit that referenced this pull request Jul 26, 2023

fix(sqllab): Replace autocomplete logic by a hook (#24677)

84488b7

(cherry picked from commit 7750517)

mistercrunch added 🍒 3.0.0 🍒 3.0.1 labels Mar 8, 2024

mistercrunch added 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sqllab): Replace autocomplete logic by a hook #24677

fix(sqllab): Replace autocomplete logic by a hook #24677

justinpark commented Jul 13, 2023

justinpark commented Jul 13, 2023

codecov bot commented Jul 13, 2023 •

edited

Loading

john-bodley commented Jul 13, 2023

john-bodley commented Jul 13, 2023

github-actions bot commented Jul 13, 2023

michael-s-molina commented Jul 13, 2023

sadpandajoe commented Jul 13, 2023

github-actions bot commented Jul 13, 2023

sadpandajoe commented Jul 14, 2023

michael-s-molina commented Jul 14, 2023

michael-s-molina left a comment

michael-s-molina Jul 19, 2023 •

edited

Loading

michael-s-molina Jul 19, 2023

justinpark Jul 20, 2023

michael-s-molina Jul 20, 2023

justinpark Jul 21, 2023

justinpark Jul 21, 2023

justinpark Jul 21, 2023

michael-s-molina Jul 26, 2023

michael-s-molina Jul 19, 2023

michael-s-molina Jul 19, 2023

michael-s-molina Jul 19, 2023

justinpark Jul 20, 2023 •

edited

Loading

michael-s-molina Jul 20, 2023

michael-s-molina commented Jul 19, 2023

michael-s-molina left a comment

github-actions bot commented Jul 26, 2023


		hasFetchedKeywords.current = !skip;

		return skip ? EMPTY_LIST : keywords;

fix(sqllab): Replace autocomplete logic by a hook #24677

fix(sqllab): Replace autocomplete logic by a hook #24677

Conversation

justinpark commented Jul 13, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

justinpark commented Jul 13, 2023

codecov bot commented Jul 13, 2023 • edited Loading

Codecov Report

john-bodley commented Jul 13, 2023

john-bodley commented Jul 13, 2023

github-actions bot commented Jul 13, 2023

michael-s-molina commented Jul 13, 2023

sadpandajoe commented Jul 13, 2023

github-actions bot commented Jul 13, 2023

sadpandajoe commented Jul 14, 2023

michael-s-molina commented Jul 14, 2023

michael-s-molina left a comment

Choose a reason for hiding this comment

michael-s-molina Jul 19, 2023 • 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

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

justinpark Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-s-molina commented Jul 19, 2023

michael-s-molina left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 26, 2023

codecov bot commented Jul 13, 2023 •

edited

Loading

michael-s-molina Jul 19, 2023 •

edited

Loading

justinpark Jul 20, 2023 •

edited

Loading