-
Notifications
You must be signed in to change notification settings - Fork 14k
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
feat(postgresql): dynamic schema #23401
Conversation
0ba2db6
to
05b1e7e
Compare
Codecov Report
@@ Coverage Diff @@
## master #23401 +/- ##
==========================================
+ Coverage 65.96% 67.61% +1.64%
==========================================
Files 1907 1907
Lines 73590 73611 +21
Branches 7982 7982
==========================================
+ Hits 48546 49772 +1226
+ Misses 22996 21791 -1205
Partials 2048 2048
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 81 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
05b1e7e
to
c807664
Compare
c807664
to
c4c7820
Compare
9a3b2f7
to
0835da1
Compare
@@ -1057,30 +1057,33 @@ def extract_errors( | |||
] | |||
|
|||
@classmethod | |||
def adjust_database_uri( # pylint: disable=unused-argument | |||
def adjust_engine_params( # pylint: disable=unused-argument |
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.
@betodealmeida what do you think will be the impact of this change on any custom db engine specs that have been added to Superset instances?
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.
oh, nm, I see how you covered this below.
superset/db_engine_specs/base.py
Outdated
) -> URL: | ||
connect_args: Dict[str, Any], | ||
catalog: Optional[str], | ||
schema: Optional[str], |
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.
I noticed that in some of the class overrides, catalog and schema have kwargs with defaults instead of positional arguments. Do we want to be consistent?
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.
Ah, good point, let me add defaults here.
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.
Looks great! I just had the one question about args vs kwargs, but not a blocker.
SUMMARY
Builds on #23385. Renames the
adjust_database_uri
method toadjust_engine_params
and makes it acceptconnect_args
as well, so we can implement dynamic schema in Postgres. In addition, the method also takes an optional catalog, so in the future we can implement proper catalog support.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before, when using Postgres the SQL Lab schema dropdown is not used to run the query. Note that in the example below
information_schema
is selected, but the query actually runs in thepublic
schema:With this PR:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION