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(sqllab): Async queries are now fetched properly #21698

Merged
merged 4 commits into from
Oct 11, 2022

Conversation

lyndsiWilliams
Copy link
Member

SUMMARY

This PR fixes a bug introduced by (#21186) that caused async queries to always render a refetch button on the first query fetch. This is because the first time an async query is run it doesn't have a prevQuery (previous query). Once the query is run (refetched through the button), there's an existing prevQuery so it would always work on the second click.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

unsuccessfulAsyncFetch

AFTER:

successfulAsyncFetch

TESTING INSTRUCTIONS

  • Ensure that you have a database with Asynchronous query execution enabled
  • Go to SQL Lab and select a database, schema, and table schema
    • I used a Google Sheets db for testing locally
  • Observe that the query runs without having to click "Fetch data preview" or "Refetch results" first.

Note: This also happens in the result set when you click "run", but that route is currently blocked. There is a fix available for this route here: #21667 - I pulled down this branch and tested, I can confirm that this also works for running queries and getting a full result (not just a table preview).

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
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Oct 4, 2022

Codecov Report

Merging #21698 (1e886b7) into master (200bed6) will decrease coverage by 0.15%.
The diff coverage is 100.00%.

❗ Current head 1e886b7 differs from pull request most recent head 2ee28bd. Consider uploading reports for the commit 2ee28bd to get more accurate results

@@            Coverage Diff             @@
##           master   #21698      +/-   ##
==========================================
- Coverage   66.84%   66.68%   -0.16%     
==========================================
  Files        1799     1799              
  Lines       68902    68869      -33     
  Branches     7324     7313      -11     
==========================================
- Hits        46057    45927     -130     
- Misses      20955    21062     +107     
+ Partials     1890     1880      -10     
Flag Coverage Δ
hive ?
javascript 53.16% <100.00%> (+0.01%) ⬆️
presto ?

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

Impacted Files Coverage Δ
...frontend/src/SqlLab/components/ResultSet/index.tsx 61.29% <100.00%> (+4.88%) ⬆️
superset/db_engines/hive.py 0.00% <0.00%> (-85.19%) ⬇️
superset/db_engine_specs/hive.py 71.14% <0.00%> (-16.21%) ⬇️
superset/db_engine_specs/presto.py 81.05% <0.00%> (-6.73%) ⬇️
...c/SqlLab/components/RunQueryActionButton/index.tsx 72.72% <0.00%> (-6.07%) ⬇️
superset/key_value/commands/delete_expired.py 80.76% <0.00%> (-3.24%) ⬇️
superset/key_value/commands/get.py 87.87% <0.00%> (-2.75%) ⬇️
superset/key_value/commands/delete.py 85.29% <0.00%> (-2.59%) ⬇️
superset/key_value/commands/update.py 88.88% <0.00%> (-2.03%) ⬇️
superset/key_value/commands/upsert.py 89.13% <0.00%> (-1.99%) ⬇️
... and 34 more

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

@lyndsiWilliams
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2022

@lyndsiWilliams Container image not yet published for this PR. Please try again when build is complete.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2022

@lyndsiWilliams Ephemeral environment creation failed. Please check the Actions logs for details.

@hughhhh hughhhh self-requested a review October 4, 2022 22:57
prevQuery?.resultsKey &&
query.resultsKey !== prevQuery.resultsKey
) {
if (query.resultsKey && query.resultsKey !== prevQuery?.resultsKey) {
Copy link
Member

Choose a reason for hiding this comment

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

is there any case where the refetch button will appear since we are removing this case?

Copy link
Member Author

@lyndsiWilliams lyndsiWilliams Oct 4, 2022

Choose a reason for hiding this comment

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

It should work normally, as it did before the refactor. Before the refactor the if statement looked like this:

if (
      nextProps.query.resultsKey &&
      nextProps.query.resultsKey !== this.props.query.resultsKey
    ) 

So I don't think prevQuery?.resultsKey && was supposed to be there in the first place.

@lyndsiWilliams
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2022

@lyndsiWilliams Ephemeral environment spinning up at http://34.216.3.212:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@EugeneTorap
Copy link
Contributor

Thank you for covering the async queries logic with unit tests ☺️

@hughhhh hughhhh self-requested a review October 5, 2022 23:09
@hughhhh hughhhh added the need:qa-review Requires QA review label Oct 6, 2022
@EugeneTorap
Copy link
Contributor

Can we merge it?

@lyndsiWilliams
Copy link
Member Author

Can we merge it?

This needs QA check before it can be merged. I've notified QA, once it's reviewed I'll merge it.

@lyndsiWilliams lyndsiWilliams force-pushed the lyndsi-fix-refetch-async-queries branch from 22177da to 2ee28bd Compare October 7, 2022 17:10
@lyndsiWilliams lyndsiWilliams merged commit d21e1d7 into master Oct 11, 2022
@lyndsiWilliams lyndsiWilliams deleted the lyndsi-fix-refetch-async-queries branch October 11, 2022 17:21
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@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
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 need:qa-review Requires QA review size/M 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants