-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
feat: Support multiple queries per request #11880
Changes from 12 commits
498e9b1
a4b2a75
efd98fb
618fbaf
6267716
af13a23
4e4238c
b6f4fe1
df1f3cb
2c8fc5d
47a1293
85a3aa5
4f6bd66
dc23d64
f01b9af
8428c07
a61c985
80460ea
0d8b758
197ba55
d6ffbf2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,7 @@ export const chart = { | |
chartUpdateStartTime: 0, | ||
latestQueryFormData: {}, | ||
queryController: null, | ||
queryResponse: null, | ||
queriesResponse: null, | ||
triggerQuery: true, | ||
lastRendered: 0, | ||
}; | ||
|
@@ -47,8 +47,8 @@ export default function chartReducer(charts = {}, action) { | |
return { | ||
...state, | ||
chartStatus: 'success', | ||
queryResponse: action.queryResponse, | ||
chartAlert: null, | ||
queriesResponse: action.queriesResponse, | ||
chartUpdateEndTime: now(), | ||
}; | ||
}, | ||
|
@@ -89,13 +89,13 @@ export default function chartReducer(charts = {}, action) { | |
return { | ||
...state, | ||
chartStatus: 'failed', | ||
chartAlert: action.queryResponse | ||
? action.queryResponse.error | ||
chartAlert: action.queriesResponse | ||
? action.queriesResponse?.[0]?.error | ||
: t('Network error.'), | ||
chartUpdateEndTime: now(), | ||
queryResponse: action.queryResponse, | ||
chartStackTrace: action.queryResponse | ||
? action.queryResponse.stacktrace | ||
queriesResponse: action.queriesResponse, | ||
chartStackTrace: action.queriesResponse | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etr2460 and also this one please |
||
? action.queriesResponse?.[0]?.stacktrace | ||
: null, | ||
}; | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think we should probably be applying the same filters to all |
||
(filter: any) => filter.column, | ||
), | ||
); | ||
const rejectedColumns: Set<string> = new Set( | ||
(chart?.queryResponse?.rejected_filters || []).map( | ||
(chart?.queriesResponse?.[0]?.rejected_filters || []).map( | ||
(filter: any) => filter.column, | ||
), | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a screenshot of multiple errors rendered in a chart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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):