feat: Support multiple queries per request #11880

simcha90 · 2020-12-01T18:30:06Z

SUMMARY

This feat give opportunity to get from BE response multiple query results and pass them to Chart plugins

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Network Response:

Results in chart plugin before:

Results in chart plugin after:

TEST PLAN

Build request with multiple queries
Related to: apache-superset/superset-ui#846

ADDITIONAL INFORMATION

amitmiran137

look stright forward
extending the object and keeping backward comparability to a single query response

codecov-io · 2020-12-01T22:51:42Z

Codecov Report

Merging #11880 (197ba55) into master (b592cc7) will increase coverage by 0.23%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #11880      +/-   ##
==========================================
+ Coverage   63.37%   63.60%   +0.23%     
==========================================
  Files         970      481     -489     
  Lines       47937    29682   -18255     
  Branches     4736        0    -4736     
==========================================
- Hits        30380    18880   -11500     
+ Misses      17370    10802    -6568     
+ Partials      187        0     -187

Flag	Coverage Δ
javascript	`?`
python	`63.60% <ø> (-0.55%)`	⬇️

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

Impacted Files	Coverage Δ
superset/db_engines/hive.py	`0.00% <0.00%> (-85.72%)`	⬇️
superset/db_engine_specs/hive.py	`54.61% <0.00%> (-29.62%)`	⬇️
superset/db_engine_specs/presto.py	`69.95% <0.00%> (-12.45%)`	⬇️
superset/examples/world_bank.py	`97.10% <0.00%> (-2.90%)`	⬇️
superset/examples/birth_names.py	`96.51% <0.00%> (-2.33%)`	⬇️
superset/views/database/mixins.py	`80.70% <0.00%> (-1.76%)`	⬇️
superset/models/core.py	`88.07% <0.00%> (-0.82%)`	⬇️
superset/connectors/sqla/models.py	`91.37% <0.00%> (-0.14%)`	⬇️
superset/views/core.py	`75.37% <0.00%> (-0.08%)`	⬇️
...rontend/src/components/ListView/CardSortSelect.tsx
... and 475 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 b592cc7...d6ffbf2. Read the comment docs.

robdiciuccio · 2020-12-02T00:27:02Z

@simchanielsen there's currently a PR open implementing async queries for charts (SIP-39) that I'm hoping to get merged in the next week or so. I'm curious how you see multiple queries working with this async model, and what changes you're proposing on the backend to support this.

Also curious, in general, what the use cases are for multiple queries in a visualization.

amitmiran137 · 2020-12-02T06:26:31Z

@robdiciuccio A classic use-case for multiple queries is uplifting table pagination in which you query the page data and second the total item count for determining how many pages to display

amitmiran137 · 2020-12-02T06:27:45Z

And about async queries, once feature is mature we can consider supporting multiple queries there as well
But for now we not familiar with that feature

villebro · 2020-12-02T08:07:00Z

@robdiciuccio based on my latest review of the async PR, I don't anticipate this feature will cause any complications with said feature.

villebro

LGTM on first pass, but I'd be interested in seeing what the cached status looks like. I assume it should be like this:

If all queries missed the cache, no cache pill is visible.
If at least one result came from the cache, the cache pill is visible.
On hover the cache status of all queries should be visible?
Curious to hear your thoughts.

villebro · 2020-12-02T11:57:50Z

superset-frontend/src/dashboard/components/SliceHeaderControls.jsx

-    const cachedWhen = moment.utc(cachedDttm).fromNow();
+    const cachedWhen = cachedDttm.map(itemCachedDttm =>
+      moment.utc(itemCachedDttm).fromNow(),
+    );
    const updatedWhen = updatedDttm ? moment.utc(updatedDttm).fromNow() : '';
-    const refreshTooltip = isCached
-      ? t('Cached %s', cachedWhen)
-      : (updatedWhen && t('Fetched %s', updatedWhen)) || '';
+    const getCachedTitle = itemCached => {
+      return itemCached
+        ? t('Cached %s', cachedWhen)
+        : updatedWhen && t('Fetched %s', updatedWhen);
+    };
+    let refreshTooltip = isCached.map(getCachedTitle) || '';
+    // If all queries have same cache time we can unit them to one
+    refreshTooltip = [...new Set(refreshTooltip)].join(', ');


Could you share a screenshot of what this looks like?

@villebro So basically if all requests will have same time it will look like"

And if different then like this (just emulate by code changes for screenshot):

I wonder if we should instead have something like this for the case when there are more than one result:

Force refresh Query 1: Fetched a minute ago Query 2: Fetched 3 minutes ago Query 3: Not cached

Also, we should think about how to display multiple results in the "View Results" tab. But that can wait; let's just make sure the user experience is unchanged for single queries.

@simchanielsen
could you share the new screenshot?

It's only emulation of 2 queries, by code changes, in real case:

if all queries have same cache time they will be squashed to one notification like it was before my change

if queries will have different time it will be shown like here

villebro

LGTM. As this feature requires superset-ui/core: "^0.15.15", it won't yet function without the bump. So you may potentially want to wait for the dashboard native filter to merge which should happen shortly (it bumps all relevant deps). If this is urgent, feel free to add the bump to this PR.

robdiciuccio · 2020-12-02T19:08:44Z

What's still unclear to me are the proposed backend changes to support multiple queries, and how that will affect the async query feature, which will be coming out of draft status this week.

Which endpoints will support multiple queries? /api/v1/chart/data? /superset/explore_json?
What changes will be made in QueryContext and viz.py to support this?

robdiciuccio · 2020-12-02T19:11:17Z

Is there a SIP that references support for multiple queries in charts?

amitmiran137 · 2020-12-02T19:54:08Z

@robdiciuccio BE already supports running multiple queries for a long time now.
The what handling the multiple responses properly in the FE

villebro · 2020-12-02T19:57:13Z

@robdiciuccio here's my take on the topic:

The multiple queries feature was added prior to the SIP process, but was only implemented on the backend - the necessary frontend changes weren't done.
The api/v1/chart/data already supports multiple queries, so no backend changes are necessary.
superset/explore_json and viz.py don't need to be changed, as they won't be supporting multiple queries.
Based on my understanding of the async PR, QueryContext gets a cache_key of it's own in QueryContext.cache_key(), and that can handle multiple queries. I'm uncertain if we want to cache only the QueryObject or both the QueryObject and QueryContext going forward, but both can be done.
There are a few things that still need to be addressed. For instance, the "View Results" and "View Query" features currently only support showing the query/results for a single query object. But AFAIK this isn't a major problem and can be addressed later. It would be great to get design input on these at some point.

robdiciuccio · 2020-12-02T22:23:11Z

Thanks for the context @villebro. Good to know the plan is not to modify the backend here.

robdiciuccio · 2020-12-02T22:26:12Z

superset-frontend/src/chart/Chart.jsx

    const isFaded = refreshOverlayVisible && !errorMessage;
    this.renderContainerStartTime = Logger.getTimestamp();
    if (chartStatus === 'failed') {
-      return this.renderErrorMessage();
+      return queriesResponse.map(item => this.renderErrorMessage(item));


Do you have a screenshot of multiple errors rendered in a chart?

@robdiciuccio Hi good catch, I didn't check it properly for all kinds of charts (new/old). I pushed now some small fix for it. Basically for now for new api there is no case that it returns more than 1 error per request even for multi queries, but if it will return we can support it, but for now it will be only one error.

P.S. If you have some more places where I can check this functionality, update me please, because I'm new in Superset so I don't know all places and cases of its usages :)

Attaching screenshots after my fix for charts:

Old charts:

New charts:

New charts with multiqueries (looks the same):

robdiciuccio · 2020-12-02T22:28:42Z

superset-frontend/src/chart/chartAction.js

+          ),
+        );
+        return dispatch(
+          chartUpdateSucceeded(queryResponse, queriesResponse, key),


What's the thinking behind adding another value here rather than replacing the current queryResponse? This is essentially doubling the data that's being passed around to components.

@robdiciuccio hmm... good point, actually I wanted avoid refactoring of a lot places in code, but now I see I think I can handle it, I also will add someone more to review, where I see that my refactor will affect his parts of code to be sure.

I pushed now refactored code as you proposed and asked @etr2460 to review also... I tested these changes in explore/dashboard in areas that I know, but if there are some other places, that I need to test please update me.

I also see that affected some code that @suddjian implemented, may be it can be good to add him as reviewer?

Thanks

simcha90 · 2020-12-03T06:04:18Z

LGTM. As this feature requires superset-ui/core: "^0.15.15", it won't yet function without the bump. So you may potentially want to wait for the dashboard native filter to merge which should happen shortly (it bumps all relevant deps). If this is urgent, feel free to add the bump to this PR.

It's backward compatible feature so can be merged unrelated superset-ui

simcha90 · 2020-12-03T08:03:18Z

superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts

@@ -132,12 +132,12 @@ export const selectIndicatorsForChart = (
  // for now we only need to know which columns are compatible/incompatible,
  // so grab the columns from the applied/rejected filters
  const appliedColumns: Set<string> = new Set(
-    (chart?.queryResponse?.applied_filters || []).map(
+    (chart?.queriesResponse?.[0]?.applied_filters || []).map(


@etr2460 Hi I see you implemented this part of code, I refactored it to use multi queries, can you please look that my changes not break some functionality. Thanks

I implemented this logic. The goal of these two lines is to get a set of all the filters that were applied on a chart, and the filters that were rejected (due to the column not existing in the dataset), in order to display them in the filter badge.

If a chart makes multiple queries, it would probably be more correct to add the applied_filters and rejected_filters from each of the queries, instead of just the first one. But that really depends on how dashboard filters works with multiple queries.

If a chart makes multiple queries, it would probably be more correct to add the applied_filters and rejected_filters from each of the queries, instead of just the first one. But that really depends on how dashboard filters works with multiple queries.

I think we should probably be applying the same filters to all QueryObjects. Since a QueryContext currently only supports a single datasource, the applied/rejected columns should be identical. However, temporal filters will only be applicable for QueryObjects with is_temporal: true.

simcha90 · 2020-12-03T08:05:50Z

superset-frontend/src/chart/chartReducer.js

-        chartStackTrace: action.queryResponse
-          ? action.queryResponse.stacktrace
+        queriesResponse: action.queriesResponse,
+        chartStackTrace: action.queriesResponse


@etr2460 and also this one please

etr2460

I think the error message changes are fine

amitmiran137 · 2020-12-10T17:25:41Z

Can this be merged?
or those wait for additional review?

robdiciuccio · 2020-12-11T22:23:11Z

Hi @simchanielsen, heads-up that the async queries PR has been merged, which may affect how the chart actions are called here. Happy to answer any questions about the changes.

amitmiran137 · 2020-12-12T14:18:36Z

superset-frontend/src/chart/chartAction.js

@@ -361,38 +361,36 @@ export function exploreJSON(

    const chartDataRequestCaught = chartDataRequest
      .then(response => {
+        const queriesResponse = response.result;
+
        if (isFeatureEnabled(FeatureFlag.GLOBAL_ASYNC_QUERIES)) {
          // deal with getChartDataRequest transforming the response data
          const result = 'result' in response ? response.result[0] : response;
          return dispatch(chartUpdateQueued(result, key));


@robdiciuccio should we continue sending a single result to chartUpdateQuered action?

Yes, the return value here is a async job metadata payload.

amitmiran137 · 2020-12-17T23:04:38Z

Can we review and merge ?

robdiciuccio · 2020-12-18T01:16:50Z

Running this locally with the GLOBAL_ASYNC_QUERIES feature flag enabled results in the following error when rendering a dashboard:

TypeError: Cannot read property 'call' of undefined
    at Chart.render (Chart.jsx?484f:272)

Also:

Warning: Failed prop type: Invalid prop `charts.597.queriesResponse` of type `object` supplied to `Dashboard`, expected an array.
Warning: Failed prop type: Invalid prop `chart.queriesResponse` of type `object` supplied to `Chart`, expected an array.

Looking into the cause.

robdiciuccio · 2020-12-18T01:28:13Z

Here are the changes necessary. Let me know if you'd like me to commit them to the PR.

* upstream/master: (55 commits) feat(explore): time picker enhancement (apache#11418) feat: update alert/report icons and column order (apache#12081) feat(explore): metrics and filters controls redesign (apache#12095) feat(alerts/reports): add refresh action (apache#12071) chore: add latest tag action (apache#11148) fix(reports): increase crontab size and alert fixes (apache#12056) Small typo fix in Athena connection docs (apache#12099) feat(queries): security perm simplification (apache#12072) feat(databases): security perm simplification (apache#12036) feat(dashboards): security permissions simplification (apache#12012) feat(logs): security permissions simplification (apache#12061) chore: Remove unused CodeModal (apache#11972) Fix typescript error (apache#12074) fix: handle context-dependent feature flags in CLI (apache#12088) fix: Fix "View in SQLLab" bug (apache#12086) feat(alert/report): add 'not null' condition option to modal (apache#12077) bumping superset ui to 15.18 and deckgl to 0.3.2 (apache#12078) fix: Python dependencies in apache#11499 (apache#12079) reset active tab on open (apache#12048) fix: improve import flow UI/UX (apache#12070) ...

amitmiran137 · 2020-12-18T12:06:19Z

@robdiciuccio I've added necessary changes.
could anyone rerun the E2E?

villebro · 2020-12-18T13:39:40Z

@amitmiran137 CI has been slightly flaky lately. I restarted, let's see if this fixes it.

* refactor: add queriesData fields for multiple queries * feat: support multi queries request * lint: fix lint * lint: fix lint * lint: fix lint * fix: fix CR notes * fix: fix CR notes * fix: fix CR notes * fix: fix error case for multi queries * feat: change queryResponse to queriesResponse * fix: revert webpack * test: fix tests * chore: lint * chore: adjust asyncEvent to multiple results * fix: lint * fix: eslint * fix: another eslint rule Co-authored-by: Amit Miran <[email protected]> Co-authored-by: amitmiran137 <[email protected]>

simcha90 added 2 commits December 1, 2020 12:57

refactor: add queriesData fields for multiple queries

498e9b1

feat: support multi queries request

a4b2a75

pull-request-size bot added the size/L label Dec 1, 2020

villebro requested a review from robdiciuccio December 1, 2020 18:43

amitmiran137 approved these changes Dec 1, 2020

View reviewed changes

lint: fix lint

efd98fb

simcha90 added 2 commits December 2, 2020 09:03

lint: fix lint

618fbaf

lint: fix lint

6267716

villebro reviewed Dec 2, 2020

View reviewed changes

simcha90 added 3 commits December 2, 2020 15:53

fix: fix CR notes

af13a23

fix: fix CR notes

4e4238c

fix: fix CR notes

b6f4fe1

villebro approved these changes Dec 2, 2020

View reviewed changes

robdiciuccio reviewed Dec 2, 2020

View reviewed changes

simcha90 changed the title ~~feat: Support multiple queries per request~~ WIP: feat: Support multiple queries per request Dec 3, 2020

simcha90 added 3 commits December 3, 2020 09:06

fix: fix error case for multi queries

df1f3cb

feat: change queryResponse to queriesResponse

2c8fc5d

fix: revert webpack

47a1293

simcha90 commented Dec 3, 2020

View reviewed changes

simcha90 changed the title ~~WIP: feat: Support multiple queries per request~~ feat: Support multiple queries per request Dec 3, 2020

test: fix tests

85a3aa5

etr2460 reviewed Dec 7, 2020

View reviewed changes

Merge branch 'master' into multi-queries

4f6bd66

Merge branch 'master' into multi-queries

dc23d64

amitmiran137 reviewed Dec 12, 2020

View reviewed changes

amitmiran137 added 2 commits December 13, 2020 19:58

chore: lint

f01b9af

Merge branch 'master' into multi-queries

8428c07

amitmiran137 added 5 commits December 18, 2020 07:31

chore: adjust asyncEvent to multiple results

80460ea

fix: lint

0d8b758

fix: eslint

197ba55

fix: another eslint rule

d6ffbf2

robdiciuccio approved these changes Dec 18, 2020

View reviewed changes

robdiciuccio merged commit 49ec13c into apache:master Dec 18, 2020

junlincc mentioned this pull request Jan 7, 2021

[chart]MultiLine is BROKEN(again) and is incompatible with async queries #12329

Closed

m-ajay mentioned this pull request Jul 4, 2023

[Snyk] Security upgrade @storybook/react from 5.3.21 to 6.0.0 m-ajay/superset#150

Open

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024

feat: Support multiple queries per request #11880

feat: Support multiple queries per request #11880

Conversation

simcha90 commented Dec 1, 2020 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

amitmiran137 left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 1, 2020 • edited Loading

Codecov Report

robdiciuccio commented Dec 2, 2020

amitmiran137 commented Dec 2, 2020

amitmiran137 commented Dec 2, 2020

villebro commented Dec 2, 2020

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simcha90 Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

villebro Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simcha90 Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

robdiciuccio commented Dec 2, 2020

robdiciuccio commented Dec 2, 2020

amitmiran137 commented Dec 2, 2020

villebro commented Dec 2, 2020 • edited Loading

robdiciuccio commented Dec 2, 2020

Choose a reason for hiding this comment

simcha90 Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simcha90 Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

simcha90 commented Dec 3, 2020

Choose a reason for hiding this comment

suddjian Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

amitmiran137 commented Dec 10, 2020

robdiciuccio commented Dec 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitmiran137 commented Dec 17, 2020

robdiciuccio commented Dec 18, 2020

robdiciuccio commented Dec 18, 2020

amitmiran137 commented Dec 18, 2020

villebro commented Dec 18, 2020

simcha90 commented Dec 1, 2020 •

edited

Loading

codecov-io commented Dec 1, 2020 •

edited

Loading

simcha90 Dec 2, 2020 •

edited

Loading

villebro Dec 2, 2020 •

edited

Loading

simcha90 Dec 3, 2020 •

edited

Loading

villebro commented Dec 2, 2020 •

edited

Loading

simcha90 Dec 3, 2020 •

edited

Loading

simcha90 Dec 3, 2020 •

edited

Loading

suddjian Dec 7, 2020 •

edited

Loading