Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(charts): allow query mutator to update queries after splitting original sql #21645

Merged
merged 24 commits into from
Jan 12, 2023

Conversation

solanksh
Copy link
Contributor

feat(charts): allow query mutator to update queries after splitting original sql

SUMMARY

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

TESTING INSTRUCTIONS

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

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

engine = self.get_sqla_engine(schema)
username = utils.get_username() or username

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@villebro Comment from original Pull Request
This variable doesn't appear to be defined in this scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baz-bakerhughes @villebro If caching is enabled in the Superset configuration, then by default the username value will be used and so username might not need be defined in the config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@villebro @baz-bakerhughes I have pushed the changes suggested.

@codecov
Copy link

codecov bot commented Sep 30, 2022

Codecov Report

Merging #21645 (5c72880) into master (2679ee2) will decrease coverage by 0.06%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master   #21645      +/-   ##
==========================================
- Coverage   66.90%   66.84%   -0.07%     
==========================================
  Files        1850     1850              
  Lines       70701    70713      +12     
  Branches     7750     7750              
==========================================
- Hits        47306    47271      -35     
- Misses      21379    21426      +47     
  Partials     2016     2016              
Flag Coverage Δ
hive 52.46% <56.25%> (-0.01%) ⬇️
mysql 77.96% <62.50%> (-0.01%) ⬇️
postgres 78.02% <56.25%> (-0.01%) ⬇️
presto ?
python 81.11% <62.50%> (-0.14%) ⬇️
sqlite 76.49% <56.25%> (-0.01%) ⬇️
unit 50.93% <18.75%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/db_engine_specs/base.py 89.07% <0.00%> (-0.14%) ⬇️
superset/models/core.py 89.56% <63.63%> (-0.95%) ⬇️
superset/config.py 91.48% <100.00%> (+0.02%) ⬆️
superset/connectors/sqla/models.py 88.77% <100.00%> (-0.59%) ⬇️
superset/db_engine_specs/presto.py 82.02% <0.00%> (-6.20%) ⬇️
superset/models/sql_lab.py 72.83% <0.00%> (-1.97%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tooptoop4
Copy link
Contributor

boomp

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@villebro villebro merged commit cf00970 into apache:master Jan 12, 2023
@tooptoop4
Copy link
Contributor

tooptoop4 commented Apr 3, 2023

@solanksh this worked great as a patch on 2.0.1 but in 2.1.0 this no longer seems to work. after clicking 'create chart' on results from sqllab it then shows "Unexpected error
Error: no results to fetch"...when i 'view query' in the explore page it shows my additional set role query in front of the main query

however if i click save dataset then it works...in 2.0.1 clicking explore forced u to save the dataset (as i notice it would send datasource type: table to the data/ endpoint) whereas 2.1.0 sends datasource type:query and doesn't force u to save the dataset

image

@lyndsiWilliams @AAfghahi i see several commits about changing sqllab to explore flow, is there way to bring back the mandatory save dataset popup when clicking 'create chart' ?

breaking change mentioned in #22802 (comment) seems like the mutate feature doesn't handle going straight into explore without creating dataset 😢 @rusackas @eschutho

@eschutho
Copy link
Member

eschutho commented Apr 4, 2023

@tooptoop4 is the functionality only broken when the new config is set to True? I tested this with the flag off and couldn't reproduce.

@tooptoop4
Copy link
Contributor

tooptoop4 commented Apr 5, 2023

@eschutho I always use with True because I need SET SESSION AUTHORIZATION '<user>'; prepended before every sql

@okayhooni
Copy link
Contributor

okayhooni commented Dec 19, 2023

@tooptoop4

I got the same issue as yours..

Could you give me some alternative method to bypass this issue..?

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
@tooptoop4
Copy link
Contributor

did u solve @okayhooni ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants