chore(sqllab): Cleanup /tables/... endpoint #21284

john-bodley · 2022-09-01T04:16:37Z

SUMMARY

The /tables/... endpoint was augmented (#1466) when SQL Lab supported:

Fetching tables/views across either a single or all schemas, i.e., the SEE TABLE SCHEMA functioned (if configured) without having a schema specified.
Backend filtering.

however the endpoint is now always called with a specified schema and filtering (substring table name matching) is never invoked.

Given these observations this PR refactors the endpoint—by removing unnecessary logic and providing a more performant ORM query given that all table/views are from a specific schema—and reduces the size of the response payload to improve performance (especially for schemas with a large number of tables/views).

Additionally as a byproduct of the refactor this PR removes:

The (now unused) get_all_table_names_in_database and get_all_view_names_in_database utility methods as well as the get_all_datasource_names DB engine spec method.
The (now unused) dbs.allow_multi_schema_metadata_fetch column
The non-functional MAX_TABLE_NAMES config key, i.e., the logic (which potentially was wrong) only capped the number of tables/views if and only if a filter was defined, however this was never invoked.
The superset update-datasources-cache CLI command.
Unnecessary keys in the response payload (to reduce the payload size).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Updated unit tests and manually tested.

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: @justinpark

ktmud

LGTM with a couple of questions

ktmud · 2022-09-01T17:44:23Z

...ions/versions/2022-08-31_19-30_291f024254b5_drop_column_allow_multi_schema_metadata_fetch.py

Should the default be false? I think enabling multi schema fetch for all data sources is a bigger risk than disabling it for all.

ktmud · 2022-09-01T17:46:06Z

superset/views/core.py

Since we are here, can we change force_refresh to a search params, too?

codecov · 2022-09-05T05:22:31Z

Codecov Report

Merging #21284 (f44908c) into master (094400c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #21284   +/-   ##
=======================================
  Coverage   66.55%   66.56%           
=======================================
  Files        1791     1791           
  Lines       68591    68497   -94     
  Branches     7319     7319           
=======================================
- Hits        45651    45595   -56     
+ Misses      21049    21011   -38     
  Partials     1891     1891

Flag	Coverage Δ
hive	`53.03% <35.71%> (+0.09%)`	⬆️
javascript	`52.73% <100.00%> (ø)`
mysql	`80.84% <100.00%> (+0.06%)`	⬆️
postgres	`80.89% <100.00%> (+0.06%)`	⬆️
presto	`52.93% <35.71%> (+0.09%)`	⬆️
python	`81.36% <100.00%> (+0.06%)`	⬆️
sqlite	`79.43% <100.00%> (+0.05%)`	⬆️
unit	`51.02% <35.71%> (+0.06%)`	⬆️

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

Impacted Files	Coverage Δ
...frontend/src/components/DatabaseSelector/index.tsx	`95.23% <ø> (ø)`
.../CRUD/data/database/DatabaseModal/ExtraOptions.tsx	`83.33% <ø> (ø)`
superset/cli/update.py	`0.00% <ø> (ø)`
superset/config.py	`91.58% <ø> (-0.03%)`	⬇️
superset/dashboards/schemas.py	`99.46% <ø> (-0.01%)`	⬇️
superset/databases/api.py	`95.49% <ø> (ø)`
superset/db_engine_specs/base.py	`89.56% <ø> (+1.23%)`	⬆️
superset/db_engine_specs/hive.py	`87.35% <ø> (+0.24%)`	⬆️
superset/db_engine_specs/presto.py	`87.78% <ø> (-0.18%)`	⬇️
superset/db_engine_specs/sqlite.py	`96.42% <ø> (-0.87%)`	⬇️
... and 6 more

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

justinpark · 2022-09-06T17:39:57Z

@john-bodley please mark Removes existing feature or API since this commit removes allow_multi_schema_metadata_fetch (Allow Multi Schema Metadata Fetch) from Database option.

justinpark

LGTM

EugeneTorap · 2022-09-14T12:17:51Z

Hi @john-bodley, the PR reproduces this tables loading issue in master branch. Can you fix it in next PR?
Can we also cover the case with a unit test?

This reverts commit eac6fdc.

betodealmeida · 2022-09-16T19:20:44Z

~~@john-bodley It looks like this broke loading tables in SQL Lab... I'm getting 404 now when it tries to load the list.~~

Just saw your fix.

Co-authored-by: Elizabeth Thompson <[email protected]>

Co-authored-by: Elizabeth Thompson <[email protected]> (cherry picked from commit 8c16806)

)" This reverts commit 8c16806.

john-bodley · 2022-11-10T23:44:40Z

Apologies this PR did not adhere to SIP-15, i.e., the database migration should have been added in a future PR and included in the subsequent major release.

rusackas · 2023-05-30T16:30:51Z

@john-bodley someone was just pointing out that this PR broke a sync job. I think technically since this changes the endpoints/URIs of the API, this would be considered a breaking change, namely as things move from

@expose("/tables/<int:db_id>/<schema>/<substr>/")
@expose("/tables/<int:db_id>/<schema>/<substr>/<force_refresh>/")
@expose("/tables/<int:db_id>/<schema>/<substr>/<force_refresh>/<exact_match>")

to

@expose("/tables/<int:db_id>/<schema>/")
@expose("/tables/<int:db_id>/<schema>/<force_refresh>/")
@deprecated(new_target="api/v1/database/<int:pk>/tables/")

I'm not sure if there's a way to patch this to be non-breaking at this point, but maybe adding a breaking change notice to UPDATING.md would at least raise awareness.

pull-request-size bot added the size/XL label Sep 1, 2022

john-bodley force-pushed the john-bodley--refactor-tables-views branch 2 times, most recently from 880dcb9 to 0baba51 Compare September 1, 2022 04:18

john-bodley marked this pull request as ready for review September 1, 2022 04:26

john-bodley requested a review from a team as a code owner September 1, 2022 04:26

john-bodley requested review from bkyryliuk and ktmud September 1, 2022 04:27

john-bodley force-pushed the john-bodley--refactor-tables-views branch 2 times, most recently from b7be637 to 88367ff Compare September 1, 2022 16:27

ktmud approved these changes Sep 1, 2022

View reviewed changes

john-bodley force-pushed the john-bodley--refactor-tables-views branch 3 times, most recently from f13fd45 to 15f0179 Compare September 5, 2022 04:47

chore(sqllab): Cleanup /tables/... endpoint

3dd8fc8

john-bodley force-pushed the john-bodley--refactor-tables-views branch from 15f0179 to 3dd8fc8 Compare September 5, 2022 05:54

Update index.test.jsx

0769a95

justinpark approved these changes Sep 7, 2022

View reviewed changes

Merge branch 'master' into john-bodley--refactor-tables-views

f44908c

john-bodley merged commit eac6fdc into apache:master Sep 13, 2022

john-bodley deleted the john-bodley--refactor-tables-views branch September 13, 2022 15:22

john-bodley mentioned this pull request Sep 14, 2022

fix: Address regression introduced in #21284 #21470

Merged

9 tasks

john-bodley added a commit to john-bodley/superset that referenced this pull request Sep 14, 2022

fix: Address regression introduced in apache#21284

32a4850

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

Revert "chore(sqllab): Cleanup /tables/... endpoint (apache#21284)"

fef5146

This reverts commit eac6fdc.

eschutho added a commit that referenced this pull request Sep 16, 2022

fix: Address regression introduced in #21284 (#21470)

8c16806

Co-authored-by: Elizabeth Thompson <[email protected]>

jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Sep 19, 2022

fix: Address regression introduced in apache#21284 (apache#21470)

00b9e3d

Co-authored-by: Elizabeth Thompson <[email protected]> (cherry picked from commit 8c16806)

mayurnewase added a commit to mayurnewase/incubator-superset that referenced this pull request Sep 22, 2022

Revert "fix: Address regression introduced in apache#21284 (apache#21470

219048f

)" This reverts commit 8c16806.

john-bodley mentioned this pull request Oct 13, 2022

fix(presto/trino): Ensure get_table_names only returns real tables #21794

Merged

9 tasks

john-bodley mentioned this pull request Nov 15, 2022

docs: Add potential downtime line item to UPDATING.md #22130

Merged

9 tasks

mistercrunch added the 🚢 2.1.3 First shipped in 2.1.3 label Feb 18, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore(sqllab): Cleanup /tables/... endpoint #21284

chore(sqllab): Cleanup /tables/... endpoint #21284

Uh oh!

john-bodley commented Sep 1, 2022 •

edited

Loading

Uh oh!

ktmud left a comment

Uh oh!

ktmud Sep 1, 2022

Uh oh!

ktmud Sep 1, 2022

Uh oh!

codecov bot commented Sep 5, 2022 •

edited

Loading

Uh oh!

justinpark commented Sep 6, 2022

Uh oh!

justinpark left a comment

Uh oh!

EugeneTorap commented Sep 14, 2022 •

edited

Loading

Uh oh!

betodealmeida commented Sep 16, 2022 •

edited

Loading

Uh oh!

john-bodley commented Nov 10, 2022

Uh oh!

rusackas commented May 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

chore(sqllab): Cleanup /tables/... endpoint #21284

chore(sqllab): Cleanup /tables/... endpoint #21284

Uh oh!

Conversation

john-bodley commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

ktmud left a comment

Choose a reason for hiding this comment

Uh oh!

ktmud Sep 1, 2022

Choose a reason for hiding this comment

Uh oh!

ktmud Sep 1, 2022

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

justinpark commented Sep 6, 2022

Uh oh!

justinpark left a comment

Choose a reason for hiding this comment

Uh oh!

EugeneTorap commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

betodealmeida commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

john-bodley commented Nov 10, 2022

Uh oh!

rusackas commented May 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

john-bodley commented Sep 1, 2022 •

edited

Loading

codecov bot commented Sep 5, 2022 •

edited

Loading

EugeneTorap commented Sep 14, 2022 •

edited

Loading

betodealmeida commented Sep 16, 2022 •

edited

Loading