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 2022/04/25 17:24:12 UTC
[GitHub] [superset] AkashN7 opened a new pull request, #19837: feat(charts): allow query mutator to update queries after splitting original sql
AkashN7 opened a new pull request, #19837:
URL: https://github.com/apache/superset/pull/19837
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
The changes allow a user to use the SQL_QUERY_MUTATOR function to add statements such as SET ROLE without breaking functionality. Currently, adding a statemen like this works fine in the SQL Editor, but Charts run individual statements by splitting a passed query and throw errors since SET ROLE does not return any data on its own. With the new MUTATE_AFTER_SPLIT variable, users can choose to apply the mutator function after the chart's query execution function splits a given query, allowing users to apply a change to all statements in a query and have Chart functionality not be interrupted.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
In the config file, update the SQL_QUERY_MUTATOR function to add a statement which does not return any results on its own and is something which should be run alongside all other statements in a given query. An example of this is adding a SET ROLE statement to the beginning of a query. Create a new Chart and see if queries return errors due to no data being fetched. Now in the config file, add a MUTATE_AFTER_SPLIT variable and set it equal to True. Create a new Chart and make a query, and verify that the desired results are showing.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #19837: feat(charts): allow query mutator to update queries after splitting original sql
Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #19837:
URL: https://github.com/apache/superset/pull/19837#discussion_r878931997
##########
superset/models/core.py:
##########
@@ -415,7 +415,11 @@ def get_df( # pylint: disable=too-many-locals
mutator: Optional[Callable[[pd.DataFrame], None]] = None,
) -> pd.DataFrame:
sqls = self.db_engine_spec.parse_sql(sql)
- engine = self.get_sqla_engine(schema)
+
+ engine = self.get_sqla_engine(schema=schema, user_name=username)
Review Comment:
`user_name` is deprecated in `get_sqla_engine`.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] merkinrob commented on pull request #19837: feat(charts): allow query mutator to update queries after splitting original sql
Posted by GitBox <gi...@apache.org>.
merkinrob commented on PR #19837:
URL: https://github.com/apache/superset/pull/19837#issuecomment-1129486280
@AkashN7 there are conflicts also
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro closed pull request #19837: feat(charts): allow query mutator to update queries after splitting original sql
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro closed pull request #19837: feat(charts): allow query mutator to update queries after splitting original sql
URL: https://github.com/apache/superset/pull/19837
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #19837: feat(charts): allow query mutator to update queries after splitting original sql
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #19837:
URL: https://github.com/apache/superset/pull/19837#discussion_r970323718
##########
superset/connectors/sqla/models.py:
##########
@@ -922,7 +922,9 @@ def mutate_query_from_config(self, sql: str) -> str:
Typically adds comments to the query with context"""
sql_query_mutator = config["SQL_QUERY_MUTATOR"]
- if sql_query_mutator:
+ mutate_after_split = config["MUTATE_AFTER_SPLIT"]
+ if sql_query_mutator and not mutate_after_split:
+ username = utils.get_username()
Review Comment:
This variable appears to be redundant as it's not referenced anywhere below
```suggestion
```
##########
superset/models/core.py:
##########
@@ -438,12 +442,29 @@ def _log_query(sql: str) -> None:
with closing(engine.raw_connection()) as conn:
cursor = conn.cursor()
for sql_ in sqls[:-1]:
+ if mutate_after_split:
+ sql_ = sql_query_mutator(
+ sql_,
+ user_name=username,
+ security_manager=security_manager,
+ database=None
+ )
_log_query(sql_)
self.db_engine_spec.execute(cursor, sql_)
cursor.fetchall()
-
- _log_query(sqls[-1])
- self.db_engine_spec.execute(cursor, sqls[-1])
+
+ if mutate_after_split:
+ last_sql = sql_query_mutator(
+ sqls[-1],
+ user_name=username,
+ security_manager=security_manager,
+ database=None
+ )
+ _log_query(last_sql)
+ self.db_engine_spec.execute(cursor, last_sql)
+ else:
+ _log_query(sqls[-1])
+ self.db_engine_spec.execute(cursor, sqls[-1])
Review Comment:
It appears `sqls[-1]` is needed in both branches of the if statement. This should simplify it somewhat
```suggestion
last_sql = sqls[-1]
if mutate_after_split:
last_sql = sql_query_mutator(
last_sql,
user_name=username,
security_manager=security_manager,
database=None
)
_log_query(last_sql)
self.db_engine_spec.execute(cursor, last_sql)
else:
_log_query(last_sql)
self.db_engine_spec.execute(cursor, last_sql)
```
##########
superset/models/core.py:
##########
@@ -415,7 +415,11 @@ def get_df( # pylint: disable=too-many-locals
mutator: Optional[Callable[[pd.DataFrame], None]] = None,
) -> pd.DataFrame:
sqls = self.db_engine_spec.parse_sql(sql)
+
engine = self.get_sqla_engine(schema)
+ username = utils.get_username() or username
Review Comment:
This variable doesn't appear to be defined in this scope?
```suggestion
username = utils.get_username()
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] baz-bakerhughes commented on pull request #19837: feat(charts): allow query mutator to update queries after splitting original sql
Posted by "baz-bakerhughes (via GitHub)" <gi...@apache.org>.
baz-bakerhughes commented on PR #19837:
URL: https://github.com/apache/superset/pull/19837#issuecomment-1420848259
Thank you sir. Yes apologies we should have closed it as this was deprecated and a new one was created because of branch permissions etc.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai commented on a diff in pull request #19837: feat(charts): allow query mutator to update queries after splitting original sql
Posted by GitBox <gi...@apache.org>.
nytai commented on code in PR #19837:
URL: https://github.com/apache/superset/pull/19837#discussion_r875231537
##########
superset/connectors/sqla/models.py:
##########
@@ -916,7 +916,8 @@ def mutate_query_from_config(self, sql: str) -> str:
Typically adds comments to the query with context"""
sql_query_mutator = config["SQL_QUERY_MUTATOR"]
- if sql_query_mutator:
+ mutate_after_split = config["MUTATE_AFTER_SPLIT"]
Review Comment:
New config vars should be added to config.py with a default value and a comment explaining what it does.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] baz-bakerhughes commented on pull request #19837: feat(charts): allow query mutator to update queries after splitting original sql
Posted by GitBox <gi...@apache.org>.
baz-bakerhughes commented on PR #19837:
URL: https://github.com/apache/superset/pull/19837#issuecomment-1195523214
@john-bodley @nytai How can we get this to the next level our colleague was an intern whose internship has ended and we would like to get this merged into main superset.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] baz-bakerhughes commented on a diff in pull request #19837: feat(charts): allow query mutator to update queries after splitting original sql
Posted by GitBox <gi...@apache.org>.
baz-bakerhughes commented on code in PR #19837:
URL: https://github.com/apache/superset/pull/19837#discussion_r932221031
##########
superset/models/core.py:
##########
@@ -415,7 +415,11 @@ def get_df( # pylint: disable=too-many-locals
mutator: Optional[Callable[[pd.DataFrame], None]] = None,
) -> pd.DataFrame:
sqls = self.db_engine_spec.parse_sql(sql)
- engine = self.get_sqla_engine(schema)
+
+ engine = self.get_sqla_engine(schema=schema, user_name=username)
Review Comment:
@john-bodley @nytai How can we get this to the next level our colleague was an intern whose internship has ended and we would like to get this merged into main superset. Apologies for my ignorance on what steps are pending on our part if any. But as none of us are approved reviewers we need a bit of guidance on how to get this into main.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AkashN7 commented on a diff in pull request #19837: feat(charts): allow query mutator to update queries after splitting original sql
Posted by GitBox <gi...@apache.org>.
AkashN7 commented on code in PR #19837:
URL: https://github.com/apache/superset/pull/19837#discussion_r877435243
##########
superset/connectors/sqla/models.py:
##########
@@ -916,7 +916,8 @@ def mutate_query_from_config(self, sql: str) -> str:
Typically adds comments to the query with context"""
sql_query_mutator = config["SQL_QUERY_MUTATOR"]
- if sql_query_mutator:
+ mutate_after_split = config["MUTATE_AFTER_SPLIT"]
Review Comment:
config.py has been updated with a default value and comment on usage
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AkashN7 commented on a diff in pull request #19837: feat(charts): allow query mutator to update queries after splitting original sql
Posted by GitBox <gi...@apache.org>.
AkashN7 commented on code in PR #19837:
URL: https://github.com/apache/superset/pull/19837#discussion_r881943324
##########
superset/models/core.py:
##########
@@ -415,7 +415,11 @@ def get_df( # pylint: disable=too-many-locals
mutator: Optional[Callable[[pd.DataFrame], None]] = None,
) -> pd.DataFrame:
sqls = self.db_engine_spec.parse_sql(sql)
- engine = self.get_sqla_engine(schema)
+
+ engine = self.get_sqla_engine(schema=schema, user_name=username)
Review Comment:
I have updated the call to `get_sqla_engine`
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] merkinrob commented on pull request #19837: feat(charts): allow query mutator to update queries after splitting original sql
Posted by GitBox <gi...@apache.org>.
merkinrob commented on PR #19837:
URL: https://github.com/apache/superset/pull/19837#issuecomment-1246794249
Thank you @villebro we will get back to you ASAP
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #19837: feat(charts): allow query mutator to update queries after splitting original sql
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #19837:
URL: https://github.com/apache/superset/pull/19837#issuecomment-1420845670
Closing as a rewrite of this PR has already been merged.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org