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: sql lab refetch button #16469

Merged

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented Aug 26, 2021

SUMMARY

See #15109, SQL Lab show "Refetch Results" button while fetching large query results. The fix in #15109 works, but it introduce another issue so we had to revert it. This PR adds extra fix on top of it.

See the 2nd commit: in airbnb most users still using client side localStorage to store query history. it seems sometimes the queries data was broken or messed up (for example, after user clear localStorage or delete some queries from History Tab). When user tries to re-run a query and sql lab start polling, sql lab can not find query id in the Redux state, which will cause exception, and show offline status.

ADDITIONAL INFORMATION

Grace Guo added 2 commits August 25, 2021 23:20
…esults (apache#15109)

* fix: SQL Lab show "Refetch Results" button while fetching new query results

* fix comments

(cherry picked from commit 408d58f)
@codecov
Copy link

codecov bot commented Aug 26, 2021

Codecov Report

Merging #16469 (147d103) into master (18be181) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16469      +/-   ##
==========================================
- Coverage   76.63%   76.62%   -0.01%     
==========================================
  Files        1002     1002              
  Lines       53637    53653      +16     
  Branches     6853     6861       +8     
==========================================
+ Hits        41104    41114      +10     
- Misses      12294    12300       +6     
  Partials      239      239              
Flag Coverage Δ
javascript 70.81% <0.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
superset-frontend/src/SqlLab/reducers/sqlLab.js 33.99% <0.00%> (-0.83%) ⬇️
superset-frontend/src/filters/utils.ts 85.18% <0.00%> (-8.15%) ⬇️
...nd/src/explore/components/DataTablesPane/index.tsx 75.24% <0.00%> (-1.99%) ⬇️
...ontrols/DndColumnSelectControl/DndMetricSelect.tsx 40.88% <0.00%> (-0.53%) ⬇️
...c/filters/components/Select/SelectFilterPlugin.tsx 80.89% <0.00%> (-0.22%) ⬇️
superset-frontend/src/views/CRUD/types.ts 100.00% <0.00%> (ø)
superset-frontend/src/components/Select/Select.tsx 76.57% <0.00%> (ø)
...perset-frontend/src/explore/components/Control.tsx 76.47% <0.00%> (ø)
...d/src/explore/components/DatasourcePanel/index.tsx 73.95% <0.00%> (ø)
...controls/DndColumnSelectControl/DndSelectLabel.tsx 77.27% <0.00%> (ø)
... and 2 more

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 18be181...147d103. Read the comment docs.

Copy link
Member

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

LGTM given #15109 was already approved

@ktmud
Copy link
Member

ktmud commented Aug 30, 2021

Reading the original description of #15109, I'm also wondering whether we can just replace the first "success" state to "fetching"?

@graceguo-supercat
Copy link
Author

graceguo-supercat commented Aug 30, 2021

Reading the original description of #15109, I'm also wondering whether we can just replace the first "success" state to "fetching"?

I think what is this PR doing.
client-side can set fetching state, but engine-side, the query state in query table, is success. In the next polling, API will return status: success and try to overwrite redux state.

@ktmud
Copy link
Member

ktmud commented Aug 30, 2021

I mean where is the original first success state set? Does it have to be success for it to not break something else?

@graceguo-supercat
Copy link
Author

I mean where is the original first success state set? Does it have to be success for it to not break something else?

the first success is read from polling API, which is stored in queries table.

@ktmud
Copy link
Member

ktmud commented Aug 30, 2021

Got it. Thanks for the clarification!

@graceguo-supercat graceguo-supercat merged commit 2199f65 into apache:master Aug 30, 2021
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
* fix: SQL Lab show "Refetch Results" button while fetching new query results (apache#15109)

* fix: SQL Lab show "Refetch Results" button while fetching new query results

* fix comments

(cherry picked from commit 408d58f)

* handle exception caused by invalid query id
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
* fix: SQL Lab show "Refetch Results" button while fetching new query results (apache#15109)

* fix: SQL Lab show "Refetch Results" button while fetching new query results

* fix comments

(cherry picked from commit 408d58f)

* handle exception caused by invalid query id
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 2024
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/S 🚢 1.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants