-
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
Add force_ctas_schema to query model when enabled #1825
Add force_ctas_schema to query model when enabled #1825
Conversation
@@ -2383,12 +2383,16 @@ def table_accessible(database, full_table_name, schema_name=None): | |||
get_datasource_access_error_msg('{}'.format(rejected_tables))) | |||
session.commit() | |||
|
|||
select_as_cta = request.form.get('select_as_cta') == 'true' | |||
if select_as_cta and mydb.force_ctas_schema: | |||
schema = mydb.force_ctas_schema |
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 am afraid it could break sql lab.
schema for this endpoint should be one selected in the drop down menu.
mydb.force_ctas_schema needs to be passed to the select * query that is created to get the results of the cta query I believe.
@vera-liu - please add a test plan to features |
Added @bkyryliuk |
After discussing with Max, I add schema to temp_table_name before adding query, so that the actual schema used is shown in output column of Query History. PTAL @bkyryliuk |
45db306
to
cbe58ef
Compare
@@ -105,7 +103,7 @@ def handle_error(msg): | |||
query.user_id, | |||
start_dttm.strftime('%Y_%m_%d_%H_%M_%S')) | |||
executed_sql = create_table_as( | |||
executed_sql, query.tmp_table_name, database.force_ctas_schema) | |||
executed_sql, query.tmp_table_name) |
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.
please add a comment that tmp_table_name includes schema here
if select_as_cta and mydb.force_ctas_schema: | ||
tmp_table_name = '{}.{}'.format( | ||
mydb.force_ctas_schema, | ||
tmp_table_name |
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.
could u please add a unit test to the celery tests?
LGTM |
Before:
After:
Tested in staging:
Todo:
needs-review @bkyryliuk @mistercrunch