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.

Sign up for GitHub

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

Jump to bottom

perf(sqllab): Rendering perf improvement using immutable state #20877

Merged
merged 4 commits into from
Aug 23, 2022
Merged

perf(sqllab): Rendering perf improvement using immutable state #20877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b01e756
perf(sqllab): Rendering perf improvement using immutable state
justinpark Jul 26, 2022
b799385
revert ISaveableDatasource type cast
justinpark Aug 5, 2022
5920999
missing trigger prop
justinpark Aug 17, 2022
dfb30fc
a default of an empty object and optional operator
justinpark Aug 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions superset-frontend/cypress-base/cypress/integration/sqllab/tabs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ describe('SqlLab query tabs', () => {
const initialUntitledCount = Math.max(
0,
...tabs
.map((i, tabItem) =>
Number(tabItem.textContent?.match(/Untitled Query (\d+)/)?.[1]),
.map(
(i, tabItem) =>
Number(tabItem.textContent?.match(/Untitled Query (\d+)/)?.[1]) ||
0,
)
.toArray(),
);
Expand Down
9 changes: 9 additions & 0 deletions superset-frontend/src/SqlLab/App.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ const sqlLabPersistStateConfig = {
...state[path],
queries: emptyQueryResults(state[path].queries),
queryEditors: clearQueryEditors(state[path].queryEditors),
unsavedQueryEditor: clearQueryEditors([
state[path].unsavedQueryEditor,
])[0],
};
}
});
Expand All @@ -91,6 +94,12 @@ const sqlLabPersistStateConfig = {
const result = {
...initialState,
...persistedState,
sqlLab: {
...(persistedState?.sqlLab || {}),
// Overwrite initialState over persistedState for sqlLab
// since a logic in getInitialState overrides the value from persistedState
...initialState.sqlLab,
},
};
// Filter out any user data that may have been persisted in an older version.
// Get user from bootstrap data instead, every time
Expand Down
102 changes: 86 additions & 16 deletions superset-frontend/src/SqlLab/actions/sqlLab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,17 @@ const fieldConverter = mapping => obj =>
const convertQueryToServer = fieldConverter(queryServerMapping);
const convertQueryToClient = fieldConverter(queryClientMapping);

export function getUpToDateQuery(rootState, queryEditor, key) {
const {
sqlLab: { unsavedQueryEditor },
} = rootState;
const id = key ?? queryEditor.id;
return {
...queryEditor,
...(id === unsavedQueryEditor.id && unsavedQueryEditor),
};
}

export function resetState() {
return { type: RESET_STATE };
}
Expand Down Expand Up @@ -167,24 +178,26 @@ export function scheduleQuery(query) {
);
}

export function estimateQueryCost(query) {
const { dbId, schema, sql, templateParams } = query;
const endpoint =
schema === null
? `/superset/estimate_query_cost/${dbId}/`
: `/superset/estimate_query_cost/${dbId}/${schema}/`;
return dispatch =>
Promise.all([
dispatch({ type: COST_ESTIMATE_STARTED, query }),
export function estimateQueryCost(queryEditor) {
return (dispatch, getState) => {
const { dbId, schema, sql, selectedText, templateParams } =
getUpToDateQuery(getState(), queryEditor);
const requestSql = selectedText || sql;
const endpoint =
schema === null
? `/superset/estimate_query_cost/${dbId}/`
: `/superset/estimate_query_cost/${dbId}/${schema}/`;
return Promise.all([
dispatch({ type: COST_ESTIMATE_STARTED, query: queryEditor }),
SupersetClient.post({
endpoint,
postPayload: {
sql,
sql: requestSql,
templateParams: JSON.parse(templateParams || '{}'),
},
})
.then(({ json }) =>
dispatch({ type: COST_ESTIMATE_RETURNED, query, json }),
dispatch({ type: COST_ESTIMATE_RETURNED, query: queryEditor, json }),
)
.catch(response =>
getClientErrorObject(response).then(error => {
Expand All @@ -194,12 +207,13 @@ export function estimateQueryCost(query) {
t('Failed at retrieving results');
return dispatch({
type: COST_ESTIMATE_FAILED,
query,
query: queryEditor,
error: message,
});
}),
),
]);
};
}

export function startQuery(query) {
Expand Down Expand Up @@ -357,15 +371,58 @@ export function runQuery(query) {
};
}

export function runQueryFromSqlEditor(
database,
queryEditor,
defaultQueryLimit,
tempTable,
ctas,
ctasMethod,
) {
return function (dispatch, getState) {
const qe = getUpToDateQuery(getState(), queryEditor, queryEditor.id);
const query = {
dbId: qe.dbId,
sql: qe.selectedText || qe.sql,
sqlEditorId: qe.id,
tab: qe.name,
schema: qe.schema,
tempTable,
templateParams: qe.templateParams,
queryLimit: qe.queryLimit || defaultQueryLimit,
runAsync: database ? database.allow_run_async : false,
ctas,
ctas_method: ctasMethod,
updateTabState: !qe.selectedText,
};
dispatch(runQuery(query));
};
}

export function reRunQuery(query) {
// run Query with a new id
return function (dispatch) {
dispatch(runQuery({ ...query, id: shortid.generate() }));
};
}

export function validateQuery(query) {
return function (dispatch) {
export function validateQuery(queryEditor, sql) {
return function (dispatch, getState) {
const {
sqlLab: { unsavedQueryEditor },
} = getState();
const qe = {
...queryEditor,
...(queryEditor.id === unsavedQueryEditor.id && unsavedQueryEditor),
};

const query = {
dbId: qe.dbId,
sql,
sqlEditorId: qe.id,
schema: qe.schema,
templateParams: qe.templateParams,
};
dispatch(startQueryValidation(query));

const postPayload = {
Expand Down Expand Up @@ -620,6 +677,7 @@ export function switchQueryEditor(queryEditor, displayLimit) {
return function (dispatch) {
if (
isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE) &&
queryEditor &&
Copy link
Member

Choose a reason for hiding this comment

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

maybe use optional chaining here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good

!queryEditor.loaded
) {
SupersetClient.get({
Expand Down Expand Up @@ -723,6 +781,17 @@ export function removeQueryEditor(queryEditor) {
};
}

export function removeAllOtherQueryEditors(queryEditor) {
return function (dispatch, getState) {
const { sqlLab } = getState();
sqlLab.queryEditors?.forEach(otherQueryEditor => {
if (otherQueryEditor.id !== queryEditor.id) {
dispatch(removeQueryEditor(otherQueryEditor));
}
});
};
}

export function removeQuery(query) {
return function (dispatch) {
const sync = isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE)
Expand Down Expand Up @@ -921,8 +990,9 @@ export function queryEditorSetSql(queryEditor, sql) {
return { type: QUERY_EDITOR_SET_SQL, queryEditor, sql };
}

export function queryEditorSetAndSaveSql(queryEditor, sql) {
return function (dispatch) {
export function queryEditorSetAndSaveSql(targetQueryEditor, sql) {
return function (dispatch, getState) {
const queryEditor = getUpToDateQuery(getState(), targetQueryEditor);
// saved query and set tab state use this action
dispatch(queryEditorSetSql(queryEditor, sql));
if (isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE)) {
Expand Down