You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ju...@apache.org on 2023/08/28 19:27:24 UTC
[superset] branch master updated: fix(sqllab): rendering performance regression by resultset (#25091)
This is an automated email from the ASF dual-hosted git repository.
justinpark pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 72150ebadf fix(sqllab): rendering performance regression by resultset (#25091)
72150ebadf is described below
commit 72150ebadf1b76d2362969e9b4fad97f9f815ac9
Author: JUST.in DO IT <ju...@airbnb.com>
AuthorDate: Mon Aug 28 12:27:18 2023 -0700
fix(sqllab): rendering performance regression by resultset (#25091)
---
.../{SouthPane.test.jsx => SouthPane.test.tsx} | 9 ++--
.../src/SqlLab/components/SouthPane/index.tsx | 53 ++++++++++++----------
.../SqlLab/components/SqlEditor/SqlEditor.test.jsx | 24 +++++++---
3 files changed, 53 insertions(+), 33 deletions(-)
diff --git a/superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.jsx b/superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.tsx
similarity index 93%
rename from superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.jsx
rename to superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.tsx
index 276d8eea66..80a102ff21 100644
--- a/superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.jsx
+++ b/superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.tsx
@@ -20,11 +20,12 @@ import React from 'react';
import configureStore from 'redux-mock-store';
import thunk from 'redux-thunk';
import { render, screen, waitFor } from 'spec/helpers/testing-library';
-import SouthPane from 'src/SqlLab/components/SouthPane';
+import SouthPane, { SouthPaneProps } from 'src/SqlLab/components/SouthPane';
import '@testing-library/jest-dom/extend-expect';
import { STATUS_OPTIONS } from 'src/SqlLab/constants';
import { initialState, table, defaultQueryEditor } from 'src/SqlLab/fixtures';
import { denormalizeTimestamp } from '@superset-ui/core';
+import { Store } from 'redux';
const mockedProps = {
queryEditorId: defaultQueryEditor.id,
@@ -42,6 +43,8 @@ const mockedEmptyProps = {
defaultQueryLimit: 100,
};
+jest.mock('src/SqlLab/components/SqlEditorLeftBar', () => jest.fn());
+
const latestQueryProgressMsg = 'LATEST QUERY MESSAGE - LCly_kkIN';
const middlewares = [thunk];
@@ -100,14 +103,14 @@ const store = mockStore({
},
},
});
-const setup = (props, store) =>
+const setup = (props: SouthPaneProps, store: Store) =>
render(<SouthPane {...props} />, {
useRedux: true,
...(store && { store }),
});
describe('SouthPane', () => {
- const renderAndWait = (props, store) =>
+ const renderAndWait = (props: SouthPaneProps, store: Store) =>
waitFor(async () => setup(props, store));
it('Renders an empty state for results', async () => {
diff --git a/superset-frontend/src/SqlLab/components/SouthPane/index.tsx b/superset-frontend/src/SqlLab/components/SouthPane/index.tsx
index 42ee4830fd..d601018a6e 100644
--- a/superset-frontend/src/SqlLab/components/SouthPane/index.tsx
+++ b/superset-frontend/src/SqlLab/components/SouthPane/index.tsx
@@ -16,8 +16,8 @@
* specific language governing permissions and limitations
* under the License.
*/
-import React, { createRef } from 'react';
-import { useDispatch, useSelector } from 'react-redux';
+import React, { createRef, useMemo } from 'react';
+import { shallowEqual, useDispatch, useSelector } from 'react-redux';
import shortid from 'shortid';
import Alert from 'src/components/Alert';
import Tabs from 'src/components/Tabs';
@@ -105,11 +105,26 @@ const SouthPane = ({
defaultQueryLimit,
}: SouthPaneProps) => {
const dispatch = useDispatch();
-
- const { editorQueries, dataPreviewQueries, databases, offline, user } =
- useSelector(({ user, sqlLab }: SqlLabRootState) => {
- const { databases, offline, queries, tables } = sqlLab;
- const dataPreviewQueries = tables
+ const user = useSelector(({ user }: SqlLabRootState) => user, shallowEqual);
+ const { databases, offline, queries, tables } = useSelector(
+ ({ sqlLab: { databases, offline, queries, tables } }: SqlLabRootState) => ({
+ databases,
+ offline,
+ queries,
+ tables,
+ }),
+ shallowEqual,
+ );
+ const editorQueries = useMemo(
+ () =>
+ Object.values(queries).filter(
+ ({ sqlEditorId }) => sqlEditorId === queryEditorId,
+ ),
+ [queries, queryEditorId],
+ );
+ const dataPreviewQueries = useMemo(
+ () =>
+ tables
.filter(
({ dataPreviewQueryId, queryEditorId: qeId }) =>
dataPreviewQueryId &&
@@ -119,18 +134,13 @@ const SouthPane = ({
.map(({ name, dataPreviewQueryId }) => ({
...queries[dataPreviewQueryId || ''],
tableName: name,
- }));
- const editorQueries = Object.values(queries).filter(
- ({ sqlEditorId }) => sqlEditorId === queryEditorId,
- );
- return {
- editorQueries,
- dataPreviewQueries,
- databases,
- offline: offline ?? false,
- user,
- };
- });
+ })),
+ [queries, queryEditorId, tables],
+ );
+ const latestQuery = useMemo(
+ () => editorQueries.find(({ id }) => id === latestQueryId),
+ [editorQueries, latestQueryId],
+ );
const activeSouthPaneTab =
useSelector<SqlLabRootState, string>(
@@ -148,11 +158,6 @@ const SouthPane = ({
);
const renderResults = () => {
- let latestQuery;
- if (editorQueries.length > 0) {
- // get the latest query
- latestQuery = editorQueries.find(({ id }) => id === latestQueryId);
- }
let results;
if (latestQuery) {
if (latestQuery?.extra?.errors) {
diff --git a/superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.jsx b/superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.jsx
index ed3f3c9de2..23424ff264 100644
--- a/superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.jsx
+++ b/superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.jsx
@@ -30,6 +30,7 @@ import {
defaultQueryEditor,
} from 'src/SqlLab/fixtures';
import SqlEditorLeftBar from 'src/SqlLab/components/SqlEditorLeftBar';
+import ResultSet from 'src/SqlLab/components/ResultSet';
import { api } from 'src/hooks/apiResources/queryApi';
import { getExtensionsRegistry } from '@superset-ui/core';
import setupExtensions from 'src/setup/setupExtensions';
@@ -46,6 +47,7 @@ jest.mock('src/components/AsyncAceEditor', () => ({
),
}));
jest.mock('src/SqlLab/components/SqlEditorLeftBar', () => jest.fn());
+jest.mock('src/SqlLab/components/ResultSet', () => jest.fn());
fetchMock.get('glob:*/api/v1/database/*/function_names/', {
function_names: [],
@@ -56,10 +58,17 @@ fetchMock.post('glob:*/sqllab/execute/*', { result: [] });
let store;
let actions;
+const latestQuery = {
+ ...queries[0],
+ sqlEditorId: defaultQueryEditor.id,
+};
const mockInitialState = {
...initialState,
sqlLab: {
...initialState.sqlLab,
+ queries: {
+ [latestQuery.id]: { ...latestQuery, startDttm: new Date().getTime() },
+ },
databases: {
1991: {
allow_ctas: false,
@@ -77,6 +86,7 @@ const mockInitialState = {
unsavedQueryEditor: {
id: defaultQueryEditor.id,
dbId: 1991,
+ latestQueryId: latestQuery.id,
},
},
};
@@ -107,7 +117,6 @@ const createStore = initState =>
describe('SqlEditor', () => {
const mockedProps = {
queryEditor: initialState.sqlLab.queryEditors[0],
- latestQuery: queries[0],
tables: [table],
getHeight: () => '100px',
editorQueries: [],
@@ -125,6 +134,8 @@ describe('SqlEditor', () => {
SqlEditorLeftBar.mockImplementation(() => (
<div data-test="mock-sql-editor-left-bar" />
));
+ ResultSet.mockClear();
+ ResultSet.mockImplementation(() => <div data-test="mock-result-set" />);
});
afterEach(() => {
@@ -153,15 +164,18 @@ describe('SqlEditor', () => {
expect(await findByTestId('react-ace')).toBeInTheDocument();
});
- it('avoids rerendering EditorLeftBar while typing', async () => {
+ it('avoids rerendering EditorLeftBar and ResultSet while typing', async () => {
const { findByTestId } = setup(mockedProps, store);
const editor = await findByTestId('react-ace');
const sql = 'select *';
const renderCount = SqlEditorLeftBar.mock.calls.length;
+ const renderCountForSouthPane = ResultSet.mock.calls.length;
expect(SqlEditorLeftBar).toHaveBeenCalledTimes(renderCount);
+ expect(ResultSet).toHaveBeenCalledTimes(renderCountForSouthPane);
fireEvent.change(editor, { target: { value: sql } });
// Verify the rendering regression
expect(SqlEditorLeftBar).toHaveBeenCalledTimes(renderCount);
+ expect(ResultSet).toHaveBeenCalledTimes(renderCountForSouthPane);
});
it('renders sql from unsaved change', async () => {
@@ -198,10 +212,8 @@ describe('SqlEditor', () => {
});
it('render a SouthPane', async () => {
- const { findByText } = setup(mockedProps, store);
- expect(
- await findByText(/run a query to display results/i),
- ).toBeInTheDocument();
+ const { findByTestId } = setup(mockedProps, store);
+ expect(await findByTestId('mock-result-set')).toBeInTheDocument();
});
it('runs query action with ctas false', async () => {