-
Notifications
You must be signed in to change notification settings - Fork 13.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(sqllab): Cleanup /tables/... endpoint #21284
chore(sqllab): Cleanup /tables/... endpoint #21284
Conversation
880dcb9
to
0baba51
Compare
b7be637
to
88367ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a couple of questions
"allow_multi_schema_metadata_fetch", | ||
sa.Boolean(), | ||
nullable=True, | ||
default=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the default be false
? I think enabling multi schema fetch for all data sources is a bigger risk than disabling it for all.
@expose("/tables/<int:db_id>/<schema>/<substr>/<force_refresh>/<exact_match>") | ||
def tables( # pylint: disable=too-many-locals,no-self-use,too-many-arguments | ||
@expose("/tables/<int:db_id>/<schema>/") | ||
@expose("/tables/<int:db_id>/<schema>/<force_refresh>/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are here, can we change force_refresh
to a search params, too?
f13fd45
to
15f0179
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
15f0179
to
3dd8fc8
Compare
@john-bodley please mark |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hi @john-bodley, the PR reproduces this |
This reverts commit eac6fdc.
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.
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. |
@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
to
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 |
SUMMARY
The
/tables/...
endpoint was augmented (#1466) when SQL Lab supported:SEE TABLE SCHEMA
functioned (if configured) without having a schema specified.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:
get_all_table_names_in_database
andget_all_view_names_in_database
utility methods as well as theget_all_datasource_names
DB engine spec method.dbs.allow_multi_schema_metadata_fetch
columnMAX_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.superset update-datasources-cache
CLI command.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Updated unit tests and manually tested.
ADDITIONAL INFORMATION
cc: @justinpark