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

[bugfix] show results in query history & revert #5848 #6436

Merged
merged 2 commits into from
Jan 14, 2019

Conversation

youngyjd
Copy link
Contributor

@youngyjd youngyjd commented Nov 26, 2018

Update:

Rendering is not necessary for query state. @graceguo-supercat made a really good point that it does not solve the inconsistency in 100% progress bar and running query state.

The query lifecycle like pending -> running -> success (or failed, stopped) ->fetching would be a simpler and better solution to solve it.

I have tested show query results in query history and verified it is not broken.

@mistercrunch
Copy link
Member

Are you sure you want to revert, or should we push forward? Did any following PRs assumed a rendering state?

@codecov-io
Copy link

codecov-io commented Nov 26, 2018

Codecov Report

Merging #6436 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6436   +/-   ##
=======================================
  Coverage   56.25%   56.25%           
=======================================
  Files         519      519           
  Lines       23077    23077           
  Branches     2755     2755           
=======================================
  Hits        12982    12982           
  Misses       9686     9686           
  Partials      409      409
Impacted Files Coverage Δ
superset/assets/src/SqlLab/reducers/sqlLab.js 56.8% <ø> (ø) ⬆️
superset/assets/src/SqlLab/constants.js 100% <ø> (ø) ⬆️
.../assets/src/SqlLab/components/QueryAutoRefresh.jsx 13.15% <0%> (ø) ⬆️

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 0053a86...e45c59a. Read the comment docs.

@graceguo-supercat
Copy link

@mistercrunch @youngyjd
I found view results didn't work in Superset Master branch. Looking at
https://github.com/apache/incubator-superset/blob/e469086d613e7f4294b45761373e5f868805738b/superset/assets/src/SqlLab/components/ResultSet.jsx#L166
it seems ResultSet didn't handle query rendering state. not sure this is the root problem.

@youngyjd youngyjd closed this Nov 28, 2018
@youngyjd youngyjd reopened this Dec 17, 2018
@youngyjd youngyjd changed the title revert #5848 [bugfix] show results in query history & revert #5848 Dec 17, 2018
@youngyjd
Copy link
Contributor Author

@graceguo-supercat @mistercrunch Commit message updated. @graceguo-supercat is a really good code reviewer. 👍

@mistercrunch
Copy link
Member

I'll let @graceguo-supercat chime in since I think she has more background on this.

@kristw kristw added the !deprecated-label:bug Deprecated label - Use #bug instead label Dec 18, 2018
@youngyjd youngyjd force-pushed the revert-5848 branch 2 times, most recently from b7ad78e to 899b034 Compare January 12, 2019 00:04
Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

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

LGTM

@graceguo-supercat graceguo-supercat merged commit b1dbd1c into apache:master Jan 14, 2019
youngyjd added a commit to lyft/incubator-superset that referenced this pull request Jan 14, 2019
)

* revert 848

* nit

(cherry picked from commit b1dbd1c)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 14, 2019
michellethomas pushed a commit that referenced this pull request Jan 14, 2019
* revert 848

* nit

(cherry picked from commit b1dbd1c)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 17, 2019
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 24, 2019
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 30, 2019
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 30, 2019
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 30, 2019
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 30, 2019
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 30, 2019
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 30, 2019
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 27, 2024
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 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 !deprecated-label:bug Deprecated label - Use #bug instead 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants