You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2019/12/19 01:51:49 UTC
[GitHub] [incubator-superset] bkyryliuk opened a new pull request #8867:
[WIP] Customize schema name for the CTA queries
bkyryliuk opened a new pull request #8867: [WIP] Customize schema name for the CTA queries
URL: https://github.com/apache/incubator-superset/pull/8867
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on issue #8867: Make
schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on issue #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#issuecomment-586066118
@villebro & @willbarrett - resolved the comments & rebased
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8867:
[WIP] Customize schema name for the CTA queries
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8867: [WIP] Customize schema name for the CTA queries
URL: https://github.com/apache/incubator-superset/pull/8867#issuecomment-569331306
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=h1) Report
> Merging [#8867](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/191aca1fb01b797dae21657021d8fcd1e836c40a?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8867 +/- ##
=======================================
Coverage 58.97% 58.97%
=======================================
Files 359 359
Lines 11333 11333
Branches 2787 2787
=======================================
Hits 6684 6684
Misses 4471 4471
Partials 178 178
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=footer). Last update [191aca1...1f8593b](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull
request #8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r382111244
##########
File path: superset/views/core.py
##########
@@ -2305,10 +2317,23 @@ def sql_json_exec(
return json_error_response(f"Database with id {database_id} is missing.")
# Set tmp_table_name for CTA
+ # Quote function quotes optionally, where as quote_identifier does it always.
+ quote = mydb.get_sqla_engine().dialect.identifier_preparer.quote_identifier
if select_as_cta and mydb.force_ctas_schema:
- tmp_table_name = f"{mydb.force_ctas_schema}.{tmp_table_name}"
+ # TODO(bkyryliuk): find a better way to pass the schema to the celery workers.
+ # There are 2 schemas, 1 for the cta table creation and another one that is the
+ # execution context. Those are not always equal.
+ tmp_table_name = f"{quote(mydb.force_ctas_schema)}.{quote(tmp_table_name)}"
+ elif select_as_cta:
+ dest_schema_name = get_cta_schema_name(mydb, g.user, schema, sql)
+ tmp_table_name = (
+ f"{quote(dest_schema_name)}.{quote(tmp_table_name)}"
+ if dest_schema_name
+ else quote(tmp_table_name)
+ )
# Save current query
+ # TODO(bkyryliuk): consider quoting schema and tmp_table_name as well.
Review comment:
you are right, my old TODO
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8867:
Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#issuecomment-569331306
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=h1) Report
> Merging [#8867](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/191aca1fb01b797dae21657021d8fcd1e836c40a?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8867 +/- ##
=======================================
Coverage 58.97% 58.97%
=======================================
Files 359 359
Lines 11333 11333
Branches 2787 2787
=======================================
Hits 6684 6684
Misses 4471 4471
Partials 178 178
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=footer). Last update [191aca1...626d764](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r381417443
##########
File path: tests/celery_tests.py
##########
@@ -172,11 +176,12 @@ def test_run_sync_query_cta(self):
query2 = self.get_query_by_id(result["query"]["serverId"])
# Check the data in the tmp table.
- if backend != "postgresql":
- # TODO This test won't work in Postgres
- results = self.run_sql(db_id, query2.select_sql, "sdf2134")
- self.assertEqual(results["status"], "success")
- self.assertGreater(len(results["data"]), 0)
+ results = self.run_sql(db_id, query2.select_sql, "sdf2134")
+ self.assertEqual(results["status"], "success")
+ self.assertGreater(len(results["data"]), 0)
Review comment:
👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on a change in pull
request #8867: [WIP] Customize schema name for the CTA queries
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #8867: [WIP] Customize schema name for the CTA queries
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r360617020
##########
File path: superset/config.py
##########
@@ -499,6 +499,13 @@ class CeleryConfig(object): # pylint: disable=too-few-public-methods
# timeout.
SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT = 10 # seconds
+# Flag that controls if limit should be inforced on the create table as queries.
+SQLLAB_CTA_NO_LIMIT = False
+
+# Function accepts username, schema name and sql that will be run e.g.:
Review comment:
It's not super clear here what this does or why you'd want to use this config element. I had to read a bit of code to understand it.
```
This allows you to define custom logic around the "CREATE TABLE AS" or CTAS feautre in SQL Lab that defines where the target schema should be for a given user.
```
Then add a proper example with type annotation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on a change in pull
request #8867: [WIP] Customize schema name for the CTA queries
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #8867: [WIP] Customize schema name for the CTA queries
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r360617492
##########
File path: superset/views/core.py
##########
@@ -2591,6 +2600,15 @@ def sql_json_exec(
# Set tmp_table_name for CTA
if select_as_cta and mydb.force_ctas_schema:
tmp_table_name = f"{mydb.force_ctas_schema}.{tmp_table_name}"
+ elif select_as_cta:
+ dest_schema_name = get_cta_schema_name(
+ schema, sql, g.user.username if g.user else None
Review comment:
`g.user` will only be there if you're in sync mode (inside the scope of a web request), won't work on Celery.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r381802479
##########
File path: tests/celery_tests.py
##########
@@ -199,15 +207,92 @@ def drop_table_if_exists(self, table_name, database=None):
db.session.flush()
return self.run_sql(db_id, sql)
- def test_run_async_query(self):
+ @mock.patch(
+ "superset.views.core.get_cta_schema_name", lambda d, u, s, sql: CTAS_SCHEMA_NAME
+ )
+ def test_run_sync_query_cta_config(self):
+ main_db = get_example_database()
+ db_id = main_db.id
+ if main_db.backend == "sqlite":
+ # sqlite doesn't support schemas
+ return
+ tmp_table_name = "tmp_async_22"
+ quote = main_db.get_sqla_engine().dialect.identifier_preparer.quote_identifier
+ expected_full_table_name = f"{quote(CTAS_SCHEMA_NAME)}.{quote(tmp_table_name)}"
+ self.drop_table_if_exists(expected_full_table_name, main_db)
+ name = "James"
+ sql_where = f"SELECT name FROM birth_names WHERE name='{name}'"
+ result = self.run_sql(
+ db_id, sql_where, "cid2", tmp_table=tmp_table_name, cta=True
+ )
+
+ self.assertEqual(QueryStatus.SUCCESS, result["query"]["state"])
+ self.assertEqual([], result["data"])
+ self.assertEqual([], result["columns"])
+ query = self.get_query_by_id(result["query"]["serverId"])
+ self.assertEqual(
+ f"CREATE TABLE {expected_full_table_name} AS \n"
+ "SELECT name FROM birth_names "
+ "WHERE name='James'",
+ query.executed_sql,
+ )
+ self.assertEqual(
+ "SELECT *\n" f"FROM {expected_full_table_name}", query.select_sql
+ )
+ time.sleep(2)
Review comment:
Not proposing adding unnecessary sleeping, but should we use the regular `CELERY_SLEEP_TIME` here? Or should we introduce a new `CELERY_SHORT_SLEEP_TIME = 2` for this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r379188758
##########
File path: superset/assets/version_info.json
##########
@@ -0,0 +1 @@
+{"GIT_SHA": "8ebedbbf4da40e62313420afe93bdc811d024b38", "version": "0.999.0dev"}
Review comment:
This SHA checked into source control seems questionable. How will this be kept in sync? Was this committed in error? Also note that this directory has moved to `superset-frontend`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on a change in pull
request #8867: [WIP] Customize schema name for the CTA queries
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #8867: [WIP] Customize schema name for the CTA queries
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r360615292
##########
File path: superset/sql_lab.py
##########
@@ -366,6 +368,11 @@ def execute_sql_statements(
payload = handle_query_error(msg, query, session, payload)
return payload
+ # Commit the connection so CTA queries will create the table.
+ # TODO(bk): consider if it's only needed for postgres.
+ if conn:
Review comment:
When would `conn` be falsy?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r381416295
##########
File path: superset/views/core.py
##########
@@ -2342,9 +2361,11 @@ def sql_json_exec(
f"Query {query_id}: Template rendering failed: {error_msg}"
)
- # set LIMIT after template processing
- limits = [mydb.db_engine_spec.get_limit_from_sql(rendered_query), limit]
- query.limit = min(lim for lim in limits if lim is not None)
+ # Limit is not appliced to the CTA queries if SQLLAB_CTAS_NO_LIMIT flag is set to True.
Review comment:
typo; appliced -> applied
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #8867: Make schema
name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#issuecomment-583725245
@bkyryliuk this needs a rebase
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull
request #8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r373744021
##########
File path: tests/superset_test_config_sqllab_backend_persist.py
##########
@@ -30,7 +30,6 @@
if "SUPERSET__SQLALCHEMY_DATABASE_URI" in os.environ:
SQLALCHEMY_DATABASE_URI = os.environ["SUPERSET__SQLALCHEMY_DATABASE_URI"]
-SQL_SELECT_AS_CTA = True
Review comment:
dead code
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull
request #8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r385317360
##########
File path: superset/sql_parse.py
##########
@@ -151,20 +151,28 @@ def __process_tokenlist(self, token_list: TokenList):
self._alias_names.add(token_list.tokens[0].value)
self.__extract_from_token(token_list)
- def as_create_table(self, table_name: str, overwrite: bool = False) -> str:
+ def as_create_table(
+ self,
+ table_name: str,
+ schema_name: Optional[str] = None,
+ overwrite: bool = False,
+ ) -> str:
"""Reformats the query into the create table as query.
Works only for the single select SQL statements, in all other cases
the sql query is not modified.
- :param table_name: Table that will contain the results of the query execution
+ :param table_name: table that will contain the results of the query execution
+ :param schema_name: schema name for the target table
:param overwrite: table_name will be dropped if true
:return: Create table as query
"""
exec_sql = ""
sql = self.stripped()
+ # TODO(bkyryliuk): quote full_table_name
+ full_table_name = f"{schema_name}.{table_name}" if schema_name else table_name
Review comment:
@john-bodley this would be an additional feature. I kept the logic as it was before.
It is worth to resolve this todo and it is existing bug in superset, but I think this PR is not a right place for the fix as it is already quite large & hard to review and comprehend.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull
request #8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r383426768
##########
File path: tests/celery_tests.py
##########
@@ -199,15 +208,89 @@ def drop_table_if_exists(self, table_name, database=None):
db.session.flush()
return self.run_sql(db_id, sql)
- def test_run_async_query(self):
+ @mock.patch(
+ "superset.views.core.get_cta_schema_name", lambda d, u, s, sql: CTAS_SCHEMA_NAME
+ )
+ def test_run_sync_query_cta_config(self):
+ main_db = get_example_database()
+ db_id = main_db.id
+ if main_db.backend == "sqlite":
+ # sqlite doesn't support schemas
+ return
+ tmp_table_name = "tmp_async_22"
+ expected_full_table_name = f"{CTAS_SCHEMA_NAME}.{tmp_table_name}"
Review comment:
I am open to either, quote while generating SQL or quote early on when getting the user input.
I think both approaches are good, the latter one would be a bit involved as table object creation should be quoted for SQLATable, Column and Metric. This test case just demonstrates the existing behavior, modifying it probably would be out of scope of this change, but definitely a useful improvement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on a change in pull
request #8867: [WIP] Customize schema name for the CTA queries
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #8867: [WIP] Customize schema name for the CTA queries
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r360617247
##########
File path: superset/config.py
##########
@@ -499,6 +499,13 @@ class CeleryConfig(object): # pylint: disable=too-few-public-methods
# timeout.
SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT = 10 # seconds
+# Flag that controls if limit should be inforced on the create table as queries.
+SQLLAB_CTA_NO_LIMIT = False
+
+# Function accepts username, schema name and sql that will be run e.g.:
Review comment:
I think we want the database object as a param too as the policy may differ across databases. Also could be good to pass in the user object instead of the username in case someone would want to dig into roles/perms.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r384940868
##########
File path: superset/sql_parse.py
##########
@@ -151,20 +151,28 @@ def __process_tokenlist(self, token_list: TokenList):
self._alias_names.add(token_list.tokens[0].value)
self.__extract_from_token(token_list)
- def as_create_table(self, table_name: str, overwrite: bool = False) -> str:
+ def as_create_table(
+ self,
+ table_name: str,
+ schema_name: Optional[str] = None,
+ overwrite: bool = False,
+ ) -> str:
"""Reformats the query into the create table as query.
Works only for the single select SQL statements, in all other cases
the sql query is not modified.
- :param table_name: Table that will contain the results of the query execution
+ :param table_name: table that will contain the results of the query execution
+ :param schema_name: schema name for the target table
:param overwrite: table_name will be dropped if true
:return: Create table as query
"""
exec_sql = ""
sql = self.stripped()
+ # TODO(bkyryliuk): quote full_table_name
+ full_table_name = f"{schema_name}.{table_name}" if schema_name else table_name
Review comment:
Shouldn’t we address the TODO? Note the quoter needs to be dialect specific.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull
request #8867: [WIP] Customize schema name for the CTA queries
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #8867: [WIP] Customize schema name for the CTA queries
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r361554392
##########
File path: superset/views/core.py
##########
@@ -2591,6 +2600,15 @@ def sql_json_exec(
# Set tmp_table_name for CTA
if select_as_cta and mydb.force_ctas_schema:
tmp_table_name = f"{mydb.force_ctas_schema}.{tmp_table_name}"
+ elif select_as_cta:
+ dest_schema_name = get_cta_schema_name(
+ schema, sql, g.user.username if g.user else None
Review comment:
`views/core.py` is in the scope of the web request, this happens before query object is created and passed to the celery worker - we should be safe here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on issue #8867: [WIP]
Customize schema name for the CTA queries
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on issue #8867: [WIP] Customize schema name for the CTA queries
URL: https://github.com/apache/incubator-superset/pull/8867#issuecomment-569156262
@mistercrunch big thanks for the comments. Sorry for putting this PR prematurely, I wanted to use CI to figure out how to deal with postgres & mysql schema creation and investigating some bugs. I was a bit lazy to set up it on my fork :) I've resolved all the comments and will do a bit more testing & deploy @ dropbox prior to the next review round to make sure it will be more polished & ready to be used.
Promise to publish more mature PRs going forward.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r381413741
##########
File path: superset/db_engine_specs/base.py
##########
@@ -640,7 +640,7 @@ def select_star( # pylint: disable=too-many-arguments,too-many-locals
if schema:
full_table_name = quote(schema) + "." + quote(table_name)
else:
- full_table_name = quote(table_name)
+ full_table_name = ".".join([quote(t) for t in table_name.split(".")])
Review comment:
Are we sure we want to split with `.` here? Periods are routinely used in table names in some analytical dbs (e.g. `table.csv` when the underlying data is in a csv file, for instance), and I believe this method usually gets called only after schema and table names have been properly separated. Unless this is needed for this feature, I believe we should stick to the existing logic. If not, this should be covered by a unit test that ensures that this doesn't cause a regression for table names with periods.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8867:
Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#issuecomment-569331306
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=h1) Report
> Merging [#8867](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/c1750af54aa5088184abec09d4490b47d5dc4f8c?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8867 +/- ##
=======================================
Coverage 59.16% 59.16%
=======================================
Files 372 372
Lines 11935 11935
Branches 2927 2927
=======================================
Hits 7061 7061
Misses 4694 4694
Partials 180 180
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=footer). Last update [c1750af...15b3475](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull
request #8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r376123385
##########
File path: .travis.yml
##########
@@ -92,7 +92,7 @@ jobs:
- redis-server
before_script:
- psql -U postgres -c "CREATE DATABASE superset;"
- - psql -U postgres -c "CREATE USER postgresuser WITH PASSWORD 'pguserpassword';"
+ - psql -U postgres -c "CREATE USER postgresuser WITH PASSWORD 'pguserpassword' SUPERUSER;"
Review comment:
move the schema creation to the travis setup, it should address this issue. Got a bit lazy here :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8867:
Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#issuecomment-569331306
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=h1) Report
> Merging [#8867](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/8e4dcd0509723907de1a1e8c2161a4a715989b24?src=pr&el=desc) will **decrease** coverage by `0.19%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8867 +/- ##
=========================================
- Coverage 59.13% 58.93% -0.2%
=========================================
Files 372 372
Lines 11938 11987 +49
Branches 2925 2936 +11
=========================================
+ Hits 7059 7065 +6
- Misses 4699 4742 +43
Partials 180 180
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/8867/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `63.39% <0%> (-11.1%)` | :arrow_down: |
| [...frontend/src/views/dashboardList/DashboardList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/8867/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2Rhc2hib2FyZExpc3QvRGFzaGJvYXJkTGlzdC50c3g=) | `59.34% <0%> (-10.38%)` | :arrow_down: |
| [...rset-frontend/src/components/ListView/ListView.tsx](https://codecov.io/gh/apache/incubator-superset/pull/8867/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvTGlzdFZpZXcudHN4) | `86.02% <0%> (-6.39%)` | :arrow_down: |
| [superset-frontend/src/components/ListView/utils.ts](https://codecov.io/gh/apache/incubator-superset/pull/8867/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvdXRpbHMudHM=) | `94.44% <0%> (-4.02%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=footer). Last update [8e4dcd0...5117648](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r382418605
##########
File path: tests/celery_tests.py
##########
@@ -199,15 +208,89 @@ def drop_table_if_exists(self, table_name, database=None):
db.session.flush()
return self.run_sql(db_id, sql)
- def test_run_async_query(self):
+ @mock.patch(
+ "superset.views.core.get_cta_schema_name", lambda d, u, s, sql: CTAS_SCHEMA_NAME
+ )
+ def test_run_sync_query_cta_config(self):
+ main_db = get_example_database()
+ db_id = main_db.id
+ if main_db.backend == "sqlite":
+ # sqlite doesn't support schemas
+ return
+ tmp_table_name = "tmp_async_22"
+ expected_full_table_name = f"{CTAS_SCHEMA_NAME}.{tmp_table_name}"
Review comment:
Back to the topic of quoting.. `select_star` in `BaseEngineSpec` quotes `schema` and `table_name`, and I'm wondering if we shouldn't do that here (`views/core.py:Superset.sql_json_exec`), too. If not, then we just need to assume users won't be doing CTAS into schemas/tables with periods, which seems like a reasonable assumption, for now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r374888673
##########
File path: .travis.yml
##########
@@ -92,7 +92,7 @@ jobs:
- redis-server
before_script:
- psql -U postgres -c "CREATE DATABASE superset;"
- - psql -U postgres -c "CREATE USER postgresuser WITH PASSWORD 'pguserpassword';"
+ - psql -U postgres -c "CREATE USER postgresuser WITH PASSWORD 'pguserpassword' SUPERUSER;"
Review comment:
While this may be ok for CI, I wonder if this might open up possible problems where we assume that the backend db user has more rights than it should have? A quick glance at the docs revealed the following:
https://www.postgresql.org/docs/10/role-attributes.html
> superuser status
A database superuser bypasses all permission checks, except the right to log in. This is a dangerous privilege and should not be used carelessly; it is best to do most of your work as a role that is not a superuser.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on a change in pull
request #8867: [WIP] Customize schema name for the CTA queries
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #8867: [WIP] Customize schema name for the CTA queries
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r360617619
##########
File path: superset/views/core.py
##########
@@ -2644,8 +2662,9 @@ def sql_json_exec(
)
# set LIMIT after template processing
- limits = [mydb.db_engine_spec.get_limit_from_sql(rendered_query), limit]
- query.limit = min(lim for lim in limits if lim is not None)
+ if not config.get("SQLLAB_CTA_NO_LIMIT", False):
Review comment:
We can assume that the key exists no need for default to False here, and if would be False anyways if the key doesn't exist
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r384755390
##########
File path: superset/views/core.py
##########
@@ -2334,9 +2346,14 @@ def sql_json_exec(
if not mydb:
return json_error_response(f"Database with id {database_id} is missing.")
- # Set tmp_table_name for CTA
+ # Set tmp_schema_name for CTA
+ # TODO(bkyryliuk): consider parsing, splitting tmp_schema_name from tmp_table_name if user enters
+ # <schema_name>.<table_name>
+ tmp_schema_name = schema # type: Optional[str]
Review comment:
Same here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r381799906
##########
File path: superset/views/core.py
##########
@@ -2305,10 +2317,23 @@ def sql_json_exec(
return json_error_response(f"Database with id {database_id} is missing.")
# Set tmp_table_name for CTA
+ # Quote function quotes optionally, where as quote_identifier does it always.
+ quote = mydb.get_sqla_engine().dialect.identifier_preparer.quote_identifier
if select_as_cta and mydb.force_ctas_schema:
- tmp_table_name = f"{mydb.force_ctas_schema}.{tmp_table_name}"
+ # TODO(bkyryliuk): find a better way to pass the schema to the celery workers.
+ # There are 2 schemas, 1 for the cta table creation and another one that is the
+ # execution context. Those are not always equal.
+ tmp_table_name = f"{quote(mydb.force_ctas_schema)}.{quote(tmp_table_name)}"
+ elif select_as_cta:
+ dest_schema_name = get_cta_schema_name(mydb, g.user, schema, sql)
+ tmp_table_name = (
+ f"{quote(dest_schema_name)}.{quote(tmp_table_name)}"
+ if dest_schema_name
+ else quote(tmp_table_name)
+ )
# Save current query
+ # TODO(bkyryliuk): consider quoting schema and tmp_table_name as well.
Review comment:
isn't at least `tmp_table_name` already quoted?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r374889991
##########
File path: superset/config.py
##########
@@ -517,6 +522,33 @@ class CeleryConfig: # pylint: disable=too-few-public-methods
# timeout.
SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT = 10 # seconds
+# Flag that controls if limit should be enforced on the CTA (create table as queries).
+SQLLAB_CTA_NO_LIMIT = False
+
+# This allows you to define custom logic around the "CREATE TABLE AS" or CTAS feature
+# in SQL Lab that defines where the target schema should be for a given user.
+# Database `CTAS Schema` has a precedence over this setting.
+# Example below returns a username and CTA queries will write tables into the schema
+# name `username`
+# SQLLAB_CTA_SCHEMA_NAME_FUNC = lambda database, user, schema, sql: user.username
+# This is move involved example where depending on the database you can leverage data
+# available to assign schema for the CTA query:
+# def compute_schema_name(database: Database, user: User, schema: str, sql: str) -> str:
+# if database.name == 'mysql_payments_slave':
+# return 'tmp_superset_schema'
+# if database.name == 'presto_gold':
+# return user.username
+# if database.name == 'analytics':
+# if 'analytics' in [r.name for r in user.roles]:
+# return 'analytics_cta'
+# else:
+# return f'tmp_{schema}'
+# Function accepts database object, user object, schema name and sql that will be run.
Review comment:
Nice comprehensive example 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8867:
Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#issuecomment-569331306
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=h1) Report
> Merging [#8867](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/291306392443a5a0d0e2ee0cc4a95d37c56d4589?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8867 +/- ##
======================================
Coverage 59.1% 59.1%
======================================
Files 372 372
Lines 11920 11920
Branches 2917 2917
======================================
Hits 7045 7045
Misses 4693 4693
Partials 182 182
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=footer). Last update [2913063...182b5ce](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r384942167
##########
File path: .travis.yml
##########
@@ -64,8 +64,10 @@ jobs:
- redis-server
before_script:
- mysql -u root -e "DROP DATABASE IF EXISTS superset; CREATE DATABASE superset DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci"
+ - mysql -u root -e "DROP DATABASE IF EXISTS sqllab_test_db; CREATE DATABASE sqllab_test_db DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci"
Review comment:
It’s not clear from this PR why we need these additional two databases.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r384752501
##########
File path: superset/config.py
##########
@@ -523,6 +526,33 @@ class CeleryConfig: # pylint: disable=too-few-public-methods
# timeout.
SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT = 10 # seconds
+# Flag that controls if limit should be enforced on the CTA (create table as queries).
+SQLLAB_CTAS_NO_LIMIT = False
+
+# This allows you to define custom logic around the "CREATE TABLE AS" or CTAS feature
+# in SQL Lab that defines where the target schema should be for a given user.
+# Database `CTAS Schema` has a precedence over this setting.
+# Example below returns a username and CTA queries will write tables into the schema
+# name `username`
+# SQLLAB_CTA_SCHEMA_NAME_FUNC = lambda database, user, schema, sql: user.username
+# This is move involved example where depending on the database you can leverage data
+# available to assign schema for the CTA query:
+# def compute_schema_name(database: Database, user: User, schema: str, sql: str) -> str:
+# if database.name == 'mysql_payments_slave':
+# return 'tmp_superset_schema'
+# if database.name == 'presto_gold':
+# return user.username
+# if database.name == 'analytics':
+# if 'analytics' in [r.name for r in user.roles]:
+# return 'analytics_cta'
+# else:
+# return f'tmp_{schema}'
+# Function accepts database object, user object, schema name and sql that will be run.
+SQLLAB_CTA_SCHEMA_NAME_FUNC = (
+ None
+) # type: Optional[Callable[["Database", "models.User", str, str], str]]
Review comment:
A bit of a nit, but as Superset has deprecated support for python 2.7, we prefer regular type annotations over type comments, i.e.
```python
SQLLAB_CTA_SCHEMA_NAME_FUNC: Optional[
Callable[["Database", "models.User", str, str], str]
] = None
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8867:
Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#issuecomment-569331306
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=h1) Report
> Merging [#8867](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/bbe433784d232538a9ed17edb9d7e5aaa0f39b0d?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8867 +/- ##
=======================================
Coverage 59.45% 59.45%
=======================================
Files 369 369
Lines 11747 11747
Branches 2888 2888
=======================================
Hits 6984 6984
Misses 4584 4584
Partials 179 179
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=footer). Last update [bbe4337...eb1a59a](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro merged pull request #8867: Make
schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro merged pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull
request #8867: [WIP] Customize schema name for the CTA queries
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #8867: [WIP] Customize schema name for the CTA queries
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r361554433
##########
File path: superset/views/core.py
##########
@@ -2644,8 +2662,9 @@ def sql_json_exec(
)
# set LIMIT after template processing
- limits = [mydb.db_engine_spec.get_limit_from_sql(rendered_query), limit]
- query.limit = min(lim for lim in limits if lim is not None)
+ if not config.get("SQLLAB_CTA_NO_LIMIT", False):
Review comment:
good point :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r381404768
##########
File path: superset/config.py
##########
@@ -41,6 +41,10 @@
logger = logging.getLogger(__name__)
+# Avoid circular import
Review comment:
I think we can remove this comment, as `TYPE_CHECKING` is pretty common throughout the codebase
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull
request #8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r379202353
##########
File path: superset/assets/version_info.json
##########
@@ -0,0 +1 @@
+{"GIT_SHA": "8ebedbbf4da40e62313420afe93bdc811d024b38", "version": "0.999.0dev"}
Review comment:
this is an error - will clean up and rebase again
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r384755271
##########
File path: superset/views/core.py
##########
@@ -243,6 +244,17 @@ def _deserialize_results_payload(
return json.loads(payload) # type: ignore
+def get_cta_schema_name(
+ database: Database, user: ab_models.User, schema: str, sql: str
+) -> Optional[str]:
+ func = config.get(
+ "SQLLAB_CTA_SCHEMA_NAME_FUNC"
+ ) # type: Optional[Callable[[Database, ab_models.User, str, str], str]]
Review comment:
Same as above. Also, we prefer using square brackets when reading configs, i.e. `config["SQLLAB_CTA_SCHEMA_NAME_FUNC"]` to avoid having to check for `None` (doesn't really apply in this case, but it's a convention nonetheless).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on a change in pull
request #8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r375097500
##########
File path: tests/base_tests.py
##########
@@ -222,22 +222,27 @@ def run_sql(
query_limit=None,
database_name="examples",
sql_editor_id=None,
+ select_as_cta=False,
+ tmp_table_name=None,
):
if user_name:
self.logout()
self.login(username=(user_name or "admin"))
dbid = self._get_database_by_name(database_name).id
+ json_payload = dict(
+ database_id=dbid,
+ sql=sql,
+ client_id=client_id,
+ queryLimit=query_limit,
+ sql_editor_id=sql_editor_id,
+ )
Review comment:
Personally I don't feel strongly about this. If a linter can't enforce it, I'd say either approaches are fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull
request #8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r385316553
##########
File path: .travis.yml
##########
@@ -64,8 +64,10 @@ jobs:
- redis-server
before_script:
- mysql -u root -e "DROP DATABASE IF EXISTS superset; CREATE DATABASE superset DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci"
+ - mysql -u root -e "DROP DATABASE IF EXISTS sqllab_test_db; CREATE DATABASE sqllab_test_db DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci"
Review comment:
@john-bodley this is purely for testing, e.g. there is a need to have 2 different schemas in the mysql & postgres to test the CTA behavior
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r382414155
##########
File path: superset/migrations/versions/72428d1ea401_add_tmp_schema_name_to_the_query_object.py
##########
@@ -0,0 +1,44 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Add tmp_schema_name to the query object.
+
+Revision ID: 72428d1ea401
+Revises: 3325d4caccc8
+Create Date: 2020-02-20 08:52:22.877902
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "72428d1ea401"
+down_revision = "3325d4caccc8"
+
+import sqlalchemy as sa
+from alembic import op
+
+
+def upgrade():
+ op.add_column(
+ "query", sa.Column("tmp_schema_name", sa.String(length=256), nullable=True)
Review comment:
(not to other reviewers) At first I was confused by the choice of column name here, but turns out there was already `tmp_table_name` for the CTAS target target table. A more appropriate name might be `target_table_name` or `ctas_table_name`, but no point in changing the current convention in this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on a change in pull
request #8867: [WIP] Customize schema name for the CTA queries
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #8867: [WIP] Customize schema name for the CTA queries
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r360616280
##########
File path: superset/sql_parse.py
##########
@@ -222,7 +222,10 @@ def get_query_with_new_limit(self, new_limit: int) -> str:
limit_pos = pos
break
_, limit = statement.token_next(idx=limit_pos)
- if limit.ttype == sqlparse.tokens.Literal.Number.Integer:
+ # Override the limit only when it exceeds the configured value.
+ if limit.ttype == sqlparse.tokens.Literal.Number.Integer and new_limit < int(
Review comment:
Mmmh, this here in theory changes what the function is expected to do ("returns the query with the specified limit"), so either we change the name/docstring to reflect that, or either we move the conditional logic towards where the function is called.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r374893429
##########
File path: tests/base_tests.py
##########
@@ -222,22 +222,27 @@ def run_sql(
query_limit=None,
database_name="examples",
sql_editor_id=None,
+ select_as_cta=False,
+ tmp_table_name=None,
):
if user_name:
self.logout()
self.login(username=(user_name or "admin"))
dbid = self._get_database_by_name(database_name).id
+ json_payload = dict(
+ database_id=dbid,
+ sql=sql,
+ client_id=client_id,
+ queryLimit=query_limit,
+ sql_editor_id=sql_editor_id,
+ )
Review comment:
While the old example used `dict()`, I think we prefer to use `{}` notation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #8867: [WIP]
Customize schema name for the CTA queries
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #8867: [WIP] Customize schema name for the CTA queries
URL: https://github.com/apache/incubator-superset/pull/8867#issuecomment-569331306
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=h1) Report
> Merging [#8867](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/191aca1fb01b797dae21657021d8fcd1e836c40a?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8867 +/- ##
=======================================
Coverage 58.97% 58.97%
=======================================
Files 359 359
Lines 11333 11333
Branches 2787 2787
=======================================
Hits 6684 6684
Misses 4471 4471
Partials 178 178
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=footer). Last update [191aca1...1f8593b](https://codecov.io/gh/apache/incubator-superset/pull/8867?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #8867: Make schema
name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#issuecomment-592980742
As this PR includes a db migration, it would be nice to merge soon if there are no objections. I'll leave this open for a few more days, then merging if no further comments surface.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull
request #8867: [WIP] Customize schema name for the CTA queries
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #8867: [WIP] Customize schema name for the CTA queries
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r361552876
##########
File path: superset/sql_parse.py
##########
@@ -222,7 +222,10 @@ def get_query_with_new_limit(self, new_limit: int) -> str:
limit_pos = pos
break
_, limit = statement.token_next(idx=limit_pos)
- if limit.ttype == sqlparse.tokens.Literal.Number.Integer:
+ # Override the limit only when it exceeds the configured value.
+ if limit.ttype == sqlparse.tokens.Literal.Number.Integer and new_limit < int(
Review comment:
Updated the docstring, yeah there is a mismatch. It's hard to move this condition outside of this function as it needs to get the existing limit value from the query and would require to parse query twice. I can't think about the usecase where we would want to override lower user limit with the higher configured value, e.g. I would expect to see 1 row when I query select * from bla limit 1 rather than 100.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r383505022
##########
File path: tests/celery_tests.py
##########
@@ -199,15 +208,89 @@ def drop_table_if_exists(self, table_name, database=None):
db.session.flush()
return self.run_sql(db_id, sql)
- def test_run_async_query(self):
+ @mock.patch(
+ "superset.views.core.get_cta_schema_name", lambda d, u, s, sql: CTAS_SCHEMA_NAME
+ )
+ def test_run_sync_query_cta_config(self):
+ main_db = get_example_database()
+ db_id = main_db.id
+ if main_db.backend == "sqlite":
+ # sqlite doesn't support schemas
+ return
+ tmp_table_name = "tmp_async_22"
+ expected_full_table_name = f"{CTAS_SCHEMA_NAME}.{tmp_table_name}"
Review comment:
Let's not push too much complexity into this PR, we can deal with this later.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull
request #8867: Make schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#discussion_r375973391
##########
File path: tests/base_tests.py
##########
@@ -222,22 +222,27 @@ def run_sql(
query_limit=None,
database_name="examples",
sql_editor_id=None,
+ select_as_cta=False,
+ tmp_table_name=None,
):
if user_name:
self.logout()
self.login(username=(user_name or "admin"))
dbid = self._get_database_by_name(database_name).id
+ json_payload = dict(
+ database_id=dbid,
+ sql=sql,
+ client_id=client_id,
+ queryLimit=query_limit,
+ sql_editor_id=sql_editor_id,
+ )
Review comment:
I prefer {} as well, if both are fine I'll stick with {} rather than dict, thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on issue #8867: Make
schema name for the CTA queries and limit configurable
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on issue #8867: Make schema name for the CTA queries and limit configurable
URL: https://github.com/apache/incubator-superset/pull/8867#issuecomment-584401801
@villebro - done, thanks !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org