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

fix: apply url_params in global async query #18921

Closed
wants to merge 1 commit into from

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Feb 24, 2022

SUMMARY

fix Jinja template is not working when the global async query is enabled.

Currently, Superset uses serialized query_obj and form_data to generate query_cache_key. But in the global async query, we didn't pass form_data. We have to get cached data from the data cache and put the cache data to g.form_data to fix it.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

url_params.before.mov

After

GAQ.after.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: fix: Jinja template is not working when async query is enabled #16650
  • 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
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Feb 24, 2022

Codecov Report

Merging #18921 (c0e10f6) into master (9d5c050) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #18921   +/-   ##
=======================================
  Coverage   66.36%   66.36%           
=======================================
  Files        1621     1621           
  Lines       63057    63058    +1     
  Branches     6382     6382           
=======================================
+ Hits        41850    41851    +1     
  Misses      19547    19547           
  Partials     1660     1660           
Flag Coverage Δ
hive 52.28% <100.00%> (+<0.01%) ⬆️
mysql 81.55% <100.00%> (+<0.01%) ⬆️
postgres 81.60% <100.00%> (+<0.01%) ⬆️
presto 52.12% <100.00%> (+<0.01%) ⬆️
python 82.04% <100.00%> (+<0.01%) ⬆️
sqlite 81.29% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
superset/charts/data/api.py 89.87% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d5c050...c0e10f6. Read the comment docs.

@kekwan
Copy link
Contributor

kekwan commented Feb 26, 2022

@zhaoyongjie I believe global async queries have 2 code paths due to legacy charts. Does superset/explore_json/data also need to be updated in this PR?

I applied this patch to our internal fork of v1.3 and verified this fix works when requests are sent to api/v1/chart. However I still see issues when url_params are used with GAQ and the request is sent to superset/explore_json/data.

@zhaoyongjie
Copy link
Member Author

zhaoyongjie commented Feb 26, 2022 via email

@zhaoyongjie
Copy link
Member Author

Hi @kekwan, This issue has been fixed by PR. So I closed this one.

@zhaoyongjie zhaoyongjie closed this Mar 3, 2022
@SanjaySharma
Copy link

Hi, I am facing the same kind of issue. I am using the Jinja template variable {{ current_aw_user_id() }} in my queries.
my queries is very simple:

SELECT * FROM attendance where id ='{{current_aw_user_id()}}';
SELECT * FROM user where reporting_to_id ='{{current_aw_user_id()}}';

I am facing the same "Unexpected error" issue.
Screenshot 2022-04-21 at 1 34 19 PM

Screenshot 2022-04-19 at 3 25 52 PM

Screenshot 2022-04-19 at 3 26 06 PM

what could be the issue, I went through multiple posts but didn't find any solution.

@kekwan
Copy link
Contributor

kekwan commented Apr 10, 2023

hey @zhaoyongjie. i don't believe this issue is fixed by #18960. I am on 1.5.1 which includes that PR but I'm still encountering this bug.

I put up #23641 which has thus fix and also patches it for legacy charts that use /explore_json endpoint as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants