[bugfix] show results in query history #6478

youngyjd · 2018-12-03T19:13:53Z

This PR is to fix a bug caused by #5848

In my local environment, I have set up running query on main database as async. This is the demo for showing results.

codecov-io · 2018-12-03T19:25:42Z

Codecov Report

Merging #6478 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6478   +/-   ##
=======================================
  Coverage   73.32%   73.32%           
=======================================
  Files          67       67           
  Lines        9592     9592           
=======================================
  Hits         7033     7033           
  Misses       2559     2559

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 cc3a625...2b137ce. Read the comment docs.

codecov-io · 2018-12-03T19:25:42Z

Codecov Report

Merging #6478 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6478   +/-   ##
=======================================
  Coverage   73.32%   73.32%           
=======================================
  Files          67       67           
  Lines        9592     9592           
=======================================
  Hits         7033     7033           
  Misses       2559     2559

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 cc3a625...a562aa1. Read the comment docs.

youngyjd · 2018-12-03T19:26:19Z

@graceguo-supercat

graceguo-supercat · 2018-12-03T21:18:11Z

@youngyjd Thanks for the fix.

see this line:
https://github.com/apache/incubator-superset/blob/69e8df404d46e35bf686cc92992d6e0415172d90/superset/assets/src/SqlLab/components/QueryAutoRefresh.jsx#L29

This line was added in #5848. Auto-refresh should run periodically as along as we have queries not finished in backend. For query in rendering state, it should be finished in backend and also fetched into browser already. Not sure we should still check status of these queries in the backend?

see this line:
https://github.com/apache/incubator-superset/blob/fc3b68e23433522b3a6d7327cc95199bf01dcf96/superset/assets/src/SqlLab/constants.js#L12

This is all available status for the lifecycle of a query. If you add a state, can you also update this set? So that this new state will be taken care by other new or existed features.

This is the lifecycle of a query (in my mind):
pending -> running -> success (or failed, stopped) ->fetching -> rendering
some state may too short to display, but query should be able to flow in this sequence.
- success is for a state that query is successfully finished in backend.
- fetching is for a state that downloading data to browser. For failed or stopped query, they don't have fetching state.
- I assume rendering means rendering data table. Can you make sure failed/stopped state also connected to rendering state? otherwise in error/stopped case, user will not see any updates.

youngyjd · 2018-12-04T01:02:10Z

@graceguo-supercat Thanks for reviewing. Updates:

Good catch. I have removed rendering state from QueryAutoRefresh, so after query results arrive in browser, it will not query backend again. This is really a good point.
https://github.com/apache/incubator-superset/blob/fc3b68e23433522b3a6d7327cc95199bf01dcf96/superset/assets/src/SqlLab/constants.js#L12 is used in QuerySearch. Not only rendering state is not in STATUS_OPTION, but also stopped and fetching. We will only need to add new state to STATUS_OPTION if we want to search for it in QuerySearch. I don't think we need to search for a query in rendering state as it is super fast that no query will remain in that state for a long time. Not adding rendering to STATUS_OPTION should be the same reason as why fetching is not in it either.
I digged into the sqllab code and the lifecycle I believe was designed as below:

For sync queries: running -> success / failed / stopped
For async queries: pending -> running -> fetching -> success or pending -> running -> fetching -> failed

After I added the rendering state, it should look like:

For sync queries: running -> rendering -> success / failed / stopped
For async queries: pending -> running -> fetching -> rendering -> success or pending -> running -> fetching -> failed

The success here I think means that query result is successfully rendered on UI, but not the real-time state for query running in backend. Backend query state is in query.results which may already says success, while browser side is showing fetching or rendering.

I am open to redesign the lifecycle of a query, but this should be in a new PR as this PR is to fix the bug without aiming to redesign a new lifecycle. What do you think?

graceguo-supercat · 2018-12-04T07:56:52Z

superset/assets/src/SqlLab/components/ResultSet.jsx

@@ -163,7 +171,7 @@ export default class ResultSet extends React.PureComponent {
            </Button>
          </Alert>
        </div>);
-    } else if (query.state === 'success') {
+    } else if (['rendering', 'success'].indexOf(query.state) >= 0) {


If query state is changed from rendering to success, will this section get called twice?

And line 159, it should apply to success instead of rendering?

yes. this part is called twice because of componentDidUpdate() in ResultSet.jsx. It is because if the state is rendering. I am not sure how we can change the state to success without re-rendering. What's your suggestion? This sound like falling into the same discussion in your comment below.

graceguo-supercat · 2018-12-04T08:33:38Z

superset/assets/src/SqlLab/components/ResultSet.jsx

@@ -64,6 +64,14 @@ export default class ResultSet extends React.PureComponent {
      this.fetchResults(nextProps.query);
    }
  }
+  componentDidUpdate() {


Update query state here will cause whole component rerender. Is success state really necessary? which logic depends on success state?

I tried, true, success state is unnecessary for sqllab to show query results. However, if you navigate to Query History after that, all historic queries' state will show rendering instead of success, which is not ideal right? That's why I add this componentDidUpdate().

youngyjd · 2018-12-04T19:56:27Z

@graceguo-supercat how about this? Let me try only having rendering state, but in https://github.com/apache/incubator-superset/blob/fc3b68e23433522b3a6d7327cc95199bf01dcf96/superset/assets/src/SqlLab/constants.js#L12 I change success: 'success' to rendering: 'success', which means there won't be any state of success. All rendering state represents success. This may save us one round of component rerender. And I can remove componentDidUpdate() as well. Do you prefer this?

[bugfix] show resules in query history

2b137ce

youngyjd changed the title ~~[bugfix] show resules in query history~~ [bugfix] show results in query history Dec 3, 2018

youngyjd mentioned this pull request Dec 3, 2018

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

Merged

kristw added !deprecated-label:bug Deprecated label - Use #bug instead .ui labels Dec 3, 2018

address comments

897f0f7

fix test

a562aa1

graceguo-supercat reviewed Dec 4, 2018

View reviewed changes

youngyjd closed this Dec 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] show results in query history #6478

[bugfix] show results in query history #6478

youngyjd commented Dec 3, 2018 •

edited

Loading

codecov-io commented Dec 3, 2018

codecov-io commented Dec 3, 2018 •

edited

Loading

youngyjd commented Dec 3, 2018

graceguo-supercat commented Dec 3, 2018 •

edited

Loading

youngyjd commented Dec 4, 2018 •

edited

Loading

graceguo-supercat Dec 4, 2018

graceguo-supercat Dec 4, 2018

youngyjd Dec 4, 2018

graceguo-supercat Dec 4, 2018 •

edited

Loading

youngyjd Dec 4, 2018 •

edited

Loading

youngyjd commented Dec 4, 2018 •

edited

Loading

[bugfix] show results in query history #6478

[bugfix] show results in query history #6478

Conversation

youngyjd commented Dec 3, 2018 • edited Loading

codecov-io commented Dec 3, 2018

Codecov Report

codecov-io commented Dec 3, 2018 • edited Loading

Codecov Report

youngyjd commented Dec 3, 2018

graceguo-supercat commented Dec 3, 2018 • edited Loading

youngyjd commented Dec 4, 2018 • edited Loading

graceguo-supercat Dec 4, 2018

Choose a reason for hiding this comment

graceguo-supercat Dec 4, 2018

Choose a reason for hiding this comment

youngyjd Dec 4, 2018

Choose a reason for hiding this comment

graceguo-supercat Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

youngyjd Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

youngyjd commented Dec 4, 2018 • edited Loading

youngyjd commented Dec 3, 2018 •

edited

Loading

codecov-io commented Dec 3, 2018 •

edited

Loading

graceguo-supercat commented Dec 3, 2018 •

edited

Loading

youngyjd commented Dec 4, 2018 •

edited

Loading

graceguo-supercat Dec 4, 2018 •

edited

Loading

youngyjd Dec 4, 2018 •

edited

Loading

youngyjd commented Dec 4, 2018 •

edited

Loading