feat(sqllab) Add a configuration option to disable data preview #14768

akedrou · 2021-05-22T23:21:53Z

SUMMARY

This PR adds a new per-database configuration option to disable data preview queries in SQL Lab. This setting is exposed in the database API as preview_data. I decided to add the feature to the database API and pass the database as a parameter to addTable rather than adding a database setting into a table API response (the proposed solution in #14726). As a result the data preview query was moved up to addTable as well. To my eye that looks cleaner since I don't think that previewing data should be a part of "table metadata" anyway.

There is no UI change, though the UX changes when data preview is disabled to not show the data preview pane (or results) when getting table metadata.

TESTING INSTRUCTIONS

In the database configuration settings, add "preview_data": false to the "Extra" JSON object.
In SQL Editor, select the database you have configured, and then any schema or table.
- Alternately, trigger metadata to load from the editor textbox itself.
Table metadata should load as normal, but no data preview query should run.
When preview_data is true (or the key does not exist), a data preview query should run when selecting a table from the dropdown or triggering metadata to be loaded from the editor.

ADDITIONAL INFORMATION

Has associated issue: Resolves [sqllab] Add configuration option to disable data preview query in SQL editor #14726
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

This setting is per-database in the "extra" JSON data.

eschutho · 2021-06-15T00:40:03Z

Thanks for the enhancement @akedrou.. I'm running tests now and added some reviewers.

eschutho · 2021-06-15T00:40:24Z

@yousoph

cccs-rc · 2022-02-10T17:01:30Z

@villebro This is the PR I was hoping we could get merged! :)

villebro · 2022-02-15T16:25:30Z

@akedrou sorry about this PR falling through the cracks 🙁 This PR would need to be rebased. Is there any chance you could go through the trouble of doing that? I'd be happy to review, test and do my best to push this through (FYI @cccs-rc)

akedrou · 2022-02-15T23:16:54Z

@villebro No problem! I figured it just slipped by and I didn't have the dedication to keep nagging - I should have some time to rebase within the next few days.

Thanks for coming back to the PR!

… sure the code is compatible with master

…le with master

…ling

…kboxes and tooltips but there are now 8

codecov · 2022-03-03T08:19:55Z

Codecov Report

Merging #14768 (8459ad6) into master (00c99c9) will decrease coverage by 0.30%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master   #14768      +/-   ##
==========================================
- Coverage   66.35%   66.04%   -0.31%     
==========================================
  Files        1640     1643       +3     
  Lines       63486    65575    +2089     
  Branches     6409     6503      +94     
==========================================
+ Hits        42124    43309    +1185     
- Misses      19702    20574     +872     
- Partials     1660     1692      +32

Flag	Coverage Δ
hive	`52.65% <100.00%> (+0.17%)`	⬆️
javascript	`51.51% <80.00%> (+0.49%)`	⬆️
mysql	`?`
postgres	`81.88% <100.00%> (+0.12%)`	⬆️
presto	`52.50% <100.00%> (+0.18%)`	⬆️
python	`80.33% <100.00%> (-1.85%)`	⬇️
sqlite	`?`

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

Impacted Files	Coverage Δ
...d/src/SqlLab/components/AceEditorWrapper/index.tsx	`43.85% <ø> (ø)`
...frontend/src/SqlLab/components/SqlEditor/index.jsx	`51.44% <ø> (+0.29%)`	⬆️
...d/src/SqlLab/components/SqlEditorLeftBar/index.tsx	`58.82% <0.00%> (+1.68%)`	⬆️
.../CRUD/data/database/DatabaseModal/ExtraOptions.tsx	`83.33% <ø> (ø)`
superset/databases/api.py	`93.99% <ø> (ø)`
superset/databases/schemas.py	`98.47% <ø> (ø)`
superset/views/core.py	`65.81% <ø> (-11.70%)`	⬇️
superset/views/database/mixins.py	`81.03% <ø> (ø)`
superset-frontend/src/SqlLab/actions/sqlLab.js	`60.73% <100.00%> (+0.22%)`	⬆️
superset/dashboards/schemas.py	`99.41% <100.00%> (+<0.01%)`	⬆️
... and 94 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00c99c9...8459ad6. Read the comment docs.

villebro

Great improvement! Something I've often been annoyed by. Some comments. Also, it seems there's currently a regression on master that breaks fetching table names (see #19008), so that probably needs to be fixed before we can properly test this

villebro · 2022-03-03T08:56:52Z

superset-frontend/src/SqlLab/actions/sqlLab.test.js

+      });
+
+      it('updates and runs data preview query when configured', () => {
+        expect.assertions(2);


I think this should be

Suggested change

expect.assertions(2);

expect.assertions(5);

villebro · 2022-03-03T08:58:02Z

superset-frontend/src/SqlLab/actions/sqlLab.test.js

-            expect(fetchMock.calls(updateTableSchemaEndpoint)).toHaveLength(1);
-            expect(fetchMock.calls(getTableMetadataEndpoint)).toHaveLength(1);
-            expect(fetchMock.calls(getExtraTableMetadataEndpoint)).toHaveLength(
-              1,
-            );


..and these lines should not be removed (I tested locally, and the test passes with these changes)

villebro · 2022-03-03T09:08:57Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/ExtraOptions.tsx

+            <StyledInputContainer>
+              <div className="input-container">
+                <IndeterminateCheckbox
+                  id="allows_preview_data"
+                  indeterminate={false}
+                  checked={!!db?.extra_json?.allows_preview_data}
+                  onChange={onExtraInputChange}
+                  labelText={t('Allow SQL Lab data preview queries')}
+                />
+                <InfoTooltip
+                  tooltip={t(
+                    'The allows_preview_data field is a boolean specifying whether or not data ' +
+                      'preview queries will be run when fetching table metadata in SQL Lab.',
+                  )}
+                />
+              </div>
+            </StyledInputContainer>


I believe we may need to reverse this flag to disable_preview_data, as otherwise this will disable data preview for existing databases (breaking change)

villebro · 2022-03-03T11:14:57Z

FYI @akedrou - @cccs-Dustin was kind enough to rebase this PR (I pushed the changes here). It still appears to need some lint cleanup + I added a few review comments. Let me know if you need help finishing this PR!

villebro · 2022-03-17T14:19:49Z

Closing in favor of #19104 which finished this work

feat(sqllab) Add a configuration option to disable data preview

6cc44c9

This setting is per-database in the "extra" JSON data.

pull-request-size bot added the size/L label May 22, 2021

akedrou changed the title ~~[WIP] feat(sqllab) Add a configuration option to disable data preview~~ feat(sqllab) Add a configuration option to disable data preview May 24, 2021

akedrou mentioned this pull request Jun 9, 2021

[sqllab] Add configuration option to disable data preview query in SQL editor #14726

Closed

eschutho requested review from betodealmeida and hughhhh June 15, 2021 00:39

cccs-Dustin mentioned this pull request Feb 16, 2022

Feature/cldn 1254 CybercentreCanada/superset#92

Merged

8 tasks

cccs-Dustin added 4 commits February 24, 2022 14:22

Initial commit to fix merge issues, still need to go through and make…

2cfe589

… sure the code is compatible with master

Added in modifications to allow for the old PR feature to be compatib…

ad29be6

…le with master

Fixed some issues that were caused by the merge conficts

bf2174b

Modified the code to fix the problem where some of the tests were fai…

b748285

…ling

cccs-Dustin mentioned this pull request Feb 25, 2022

feat(sqllab): Add a configuration option to disable data preview - Rabase of original PR #18946

Closed

8 tasks

cccs-Dustin added 4 commits February 25, 2022 08:37

Ran pre-commit hook to remove any issues it would pick up

8eb76a9

Fixed a pylint error where a line was too long

fb1343b

Fixed the test so it no longer fails, since it was looking for 7 chec…

f91c691

…kboxes and tooltips but there are now 8

Fixed the test so it no longer fails, since it was looking for 7 chec…

8459ad6

…kboxes and tooltips but there are now 8

villebro reviewed Mar 3, 2022

View reviewed changes

cccs-Dustin mentioned this pull request Mar 10, 2022

feat(sqllab): Add a configuration option to disable data preview #19104

Merged

8 tasks

villebro closed this Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sqllab) Add a configuration option to disable data preview #14768

feat(sqllab) Add a configuration option to disable data preview #14768

akedrou commented May 22, 2021

eschutho commented Jun 15, 2021

eschutho commented Jun 15, 2021

cccs-rc commented Feb 10, 2022

villebro commented Feb 15, 2022

akedrou commented Feb 15, 2022

codecov bot commented Mar 3, 2022 •

edited

Loading

villebro left a comment •

edited

Loading

villebro Mar 3, 2022

villebro Mar 3, 2022

villebro Mar 3, 2022

villebro commented Mar 3, 2022

villebro commented Mar 17, 2022

feat(sqllab) Add a configuration option to disable data preview #14768

feat(sqllab) Add a configuration option to disable data preview #14768

Conversation

akedrou commented May 22, 2021

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

eschutho commented Jun 15, 2021

eschutho commented Jun 15, 2021

cccs-rc commented Feb 10, 2022

villebro commented Feb 15, 2022

akedrou commented Feb 15, 2022

codecov bot commented Mar 3, 2022 • edited Loading

Codecov Report

villebro left a comment • edited Loading

Choose a reason for hiding this comment

villebro Mar 3, 2022

Choose a reason for hiding this comment

villebro Mar 3, 2022

Choose a reason for hiding this comment

villebro Mar 3, 2022

Choose a reason for hiding this comment

villebro commented Mar 3, 2022

villebro commented Mar 17, 2022

codecov bot commented Mar 3, 2022 •

edited

Loading

villebro left a comment •

edited

Loading