From c590e90c875156fc4298423c91ca5571dbcebeca Mon Sep 17 00:00:00 2001 From: "JUST.in DO IT" Date: Fri, 31 Jan 2025 10:19:37 -0800 Subject: [PATCH] feat(sqllab): improve table metadata UI (#32051) --- .../src/ui-overrides/types.ts | 11 + .../src/SqlLab/actions/sqlLab.js | 40 +- .../src/SqlLab/actions/sqlLab.test.js | 70 ++- .../AceEditorWrapper.test.tsx | 19 +- .../AceEditorWrapper/useKeywords.test.ts | 2 + .../src/SqlLab/components/App/index.tsx | 2 +- .../src/SqlLab/components/ShowSQL/index.tsx | 12 +- .../components/SouthPane/SouthPane.test.tsx | 6 +- .../src/SqlLab/components/SouthPane/index.tsx | 95 ++-- .../SqlEditorLeftBar.test.tsx | 63 ++- .../components/SqlEditorLeftBar/index.tsx | 11 +- .../TableElement/TableElement.test.tsx | 10 +- .../TablePreview/TablePreview.test.tsx | 173 +++++++ .../SqlLab/components/TablePreview/index.tsx | 430 ++++++++++++++++++ superset-frontend/src/SqlLab/fixtures.ts | 5 +- .../SqlLab/reducers/getInitialState.test.ts | 10 +- .../src/SqlLab/reducers/getInitialState.ts | 7 +- .../src/SqlLab/reducers/sqlLab.js | 50 +- superset-frontend/src/SqlLab/types.ts | 2 +- .../src/components/Icons/AntdEnhanced.tsx | 2 + .../src/hooks/apiResources/sqlLab.ts | 4 +- .../src/hooks/apiResources/tables.ts | 10 +- 22 files changed, 889 insertions(+), 145 deletions(-) create mode 100644 superset-frontend/src/SqlLab/components/TablePreview/TablePreview.test.tsx create mode 100644 superset-frontend/src/SqlLab/components/TablePreview/index.tsx diff --git a/superset-frontend/packages/superset-ui-core/src/ui-overrides/types.ts b/superset-frontend/packages/superset-ui-core/src/ui-overrides/types.ts index 3f0ff82b2fcb5..6583bee732848 100644 --- a/superset-frontend/packages/superset-ui-core/src/ui-overrides/types.ts +++ b/superset-frontend/packages/superset-ui-core/src/ui-overrides/types.ts @@ -144,6 +144,13 @@ export interface SQLResultTableExtensionProps { allowHTML?: boolean; } +export interface SQLTablePreviewExtensionProps { + dbId: number; + catalog?: string; + schema: string; + tableName: string; +} + /** * Interface for extensions to Slice Header */ @@ -229,4 +236,8 @@ export type Extensions = Partial<{ 'sqleditor.extension.customAutocomplete': ( args: CustomAutoCompleteArgs, ) => CustomAutocomplete[] | undefined; + 'sqleditor.extension.tablePreview': [ + string, + ComponentType, + ][]; }>; diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.js b/superset-frontend/src/SqlLab/actions/sqlLab.js index d8ec7a19a564e..9f62d781ed3b7 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.js @@ -237,7 +237,7 @@ export function clearInactiveQueries(interval) { return { type: CLEAR_INACTIVE_QUERIES, interval }; } -export function startQuery(query) { +export function startQuery(query, runPreviewOnly) { Object.assign(query, { id: query.id ? query.id : nanoid(11), progress: 0, @@ -245,7 +245,7 @@ export function startQuery(query) { state: query.runAsync ? 'pending' : 'running', cached: false, }); - return { type: START_QUERY, query }; + return { type: START_QUERY, query, runPreviewOnly }; } export function querySuccess(query, results) { @@ -327,9 +327,9 @@ export function fetchQueryResults(query, displayLimit, timeoutInMs) { }; } -export function runQuery(query) { +export function runQuery(query, runPreviewOnly) { return function (dispatch) { - dispatch(startQuery(query)); + dispatch(startQuery(query, runPreviewOnly)); const postPayload = { client_id: query.id, database_id: query.dbId, @@ -947,29 +947,25 @@ export function mergeTable(table, query, prepend) { export function addTable(queryEditor, tableName, catalogName, schemaName) { return function (dispatch, getState) { - const query = getUpToDateQuery(getState(), queryEditor, queryEditor.id); + const { dbId } = getUpToDateQuery(getState(), queryEditor, queryEditor.id); const table = { - dbId: query.dbId, - queryEditorId: query.id, + dbId, + queryEditorId: queryEditor.id, catalog: catalogName, schema: schemaName, name: tableName, }; dispatch( - mergeTable( - { - ...table, - id: nanoid(11), - expanded: true, - }, - null, - true, - ), + mergeTable({ + ...table, + id: nanoid(11), + expanded: true, + }), ); }; } -export function runTablePreviewQuery(newTable) { +export function runTablePreviewQuery(newTable, runPreviewOnly) { return function (dispatch, getState) { const { sqlLab: { databases }, @@ -979,7 +975,7 @@ export function runTablePreviewQuery(newTable) { if (database && !database.disable_data_preview) { const dataPreviewQuery = { - id: nanoid(11), + id: newTable.previewQueryId ?? nanoid(11), dbId, catalog, schema, @@ -991,6 +987,9 @@ export function runTablePreviewQuery(newTable) { ctas: false, isDataPreview: true, }; + if (runPreviewOnly) { + return dispatch(runQuery(dataPreviewQuery, runPreviewOnly)); + } return Promise.all([ dispatch( mergeTable( @@ -1024,7 +1023,7 @@ export function syncTable(table, tableMetadata) { return sync .then(({ json: resultJson }) => { - const newTable = { ...table, id: resultJson.id }; + const newTable = { ...table, id: `${resultJson.id}` }; dispatch( mergeTable({ ...newTable, @@ -1032,9 +1031,6 @@ export function syncTable(table, tableMetadata) { initialized: true, }), ); - if (!table.dataPreviewQueryId) { - dispatch(runTablePreviewQuery({ ...tableMetadata, ...newTable })); - } }) .catch(() => dispatch( diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.test.js b/superset-frontend/src/SqlLab/actions/sqlLab.test.js index 58ba6b764d722..abbdb0c99ef65 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.test.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.test.js @@ -1031,30 +1031,38 @@ describe('async actions', () => { }); describe('runTablePreviewQuery', () => { - it('updates and runs data preview query when configured', () => { - expect.assertions(3); + const results = { + data: mockBigNumber, + query: { sqlEditorId: 'null', dbId: 1 }, + query_id: 'efgh', + }; + const tableName = 'table'; + const catalogName = null; + const schemaName = 'schema'; + const store = mockStore({ + ...initialState, + sqlLab: { + ...initialState.sqlLab, + databases: { + 1: { disable_data_preview: false }, + }, + }, + }); - const results = { - data: mockBigNumber, - query: { sqlEditorId: 'null', dbId: 1 }, - query_id: 'efgh', - }; + beforeEach(() => { fetchMock.post(runQueryEndpoint, JSON.stringify(results), { overwriteRoutes: true, }); + }); + + afterEach(() => { + store.clearActions(); + fetchMock.resetHistory(); + }); + + it('updates and runs data preview query when configured', () => { + expect.assertions(3); - const tableName = 'table'; - const catalogName = null; - const schemaName = 'schema'; - const store = mockStore({ - ...initialState, - sqlLab: { - ...initialState.sqlLab, - databases: { - 1: { disable_data_preview: false }, - }, - }, - }); const expectedActionTypes = [ actions.MERGE_TABLE, // addTable (data preview) actions.START_QUERY, // runQuery (data preview) @@ -1075,6 +1083,30 @@ describe('async actions', () => { expect(fetchMock.calls(updateTabStateEndpoint)).toHaveLength(0); }); }); + + it('runs data preview query only', () => { + const expectedActionTypes = [ + actions.START_QUERY, // runQuery (data preview) + actions.QUERY_SUCCESS, // querySuccess + ]; + const request = actions.runTablePreviewQuery( + { + dbId: 1, + name: tableName, + catalog: catalogName, + schema: schemaName, + }, + true, + ); + return request(store.dispatch, store.getState).then(() => { + expect(store.getActions().map(a => a.type)).toEqual( + expectedActionTypes, + ); + expect(fetchMock.calls(runQueryEndpoint)).toHaveLength(1); + // tab state is not updated, since the query is a data preview + expect(fetchMock.calls(updateTabStateEndpoint)).toHaveLength(0); + }); + }); }); describe('expandTable', () => { diff --git a/superset-frontend/src/SqlLab/components/AceEditorWrapper/AceEditorWrapper.test.tsx b/superset-frontend/src/SqlLab/components/AceEditorWrapper/AceEditorWrapper.test.tsx index e2abec9f8c767..82a79103fe274 100644 --- a/superset-frontend/src/SqlLab/components/AceEditorWrapper/AceEditorWrapper.test.tsx +++ b/superset-frontend/src/SqlLab/components/AceEditorWrapper/AceEditorWrapper.test.tsx @@ -16,8 +16,6 @@ * specific language governing permissions and limitations * under the License. */ -import configureStore from 'redux-mock-store'; -import thunk from 'redux-thunk'; import reducerIndex from 'spec/helpers/reducerIndex'; import { render, waitFor, createStore } from 'spec/helpers/testing-library'; import { QueryEditor } from 'src/SqlLab/types'; @@ -34,9 +32,6 @@ import { } from 'src/SqlLab/actions/sqlLab'; import fetchMock from 'fetch-mock'; -const middlewares = [thunk]; -const mockStore = configureStore(middlewares); - fetchMock.get('glob:*/api/v1/database/*/function_names/', { function_names: [], }); @@ -79,7 +74,8 @@ describe('AceEditorWrapper', () => { }); it('renders ace editor including sql value', async () => { - const { getByTestId } = setup(defaultQueryEditor, mockStore(initialState)); + const store = createStore(initialState, reducerIndex); + const { getByTestId } = setup(defaultQueryEditor, store); await waitFor(() => expect(getByTestId('react-ace')).toBeInTheDocument()); expect(getByTestId('react-ace')).toHaveTextContent( @@ -89,9 +85,8 @@ describe('AceEditorWrapper', () => { it('renders current sql for unrelated unsaved changes', () => { const expectedSql = 'SELECT updated_column\nFROM updated_table\nWHERE'; - const { getByTestId } = setup( - defaultQueryEditor, - mockStore({ + const store = createStore( + { ...initialState, sqlLab: { ...initialState.sqlLab, @@ -100,8 +95,10 @@ describe('AceEditorWrapper', () => { sql: expectedSql, }, }, - }), + }, + reducerIndex, ); + const { getByTestId } = setup(defaultQueryEditor, store); expect(getByTestId('react-ace')).not.toHaveTextContent( JSON.stringify({ value: expectedSql }).slice(1, -1), @@ -122,7 +119,7 @@ describe('AceEditorWrapper', () => { queryEditorSetCursorPosition(defaultQueryEditor, updatedCursorPosition), ); expect(FullSQLEditor).toHaveBeenCalledTimes(renderCount); - store.dispatch(queryEditorSetDb(defaultQueryEditor, 1)); + store.dispatch(queryEditorSetDb(defaultQueryEditor, 2)); expect(FullSQLEditor).toHaveBeenCalledTimes(renderCount + 1); }); }); diff --git a/superset-frontend/src/SqlLab/components/AceEditorWrapper/useKeywords.test.ts b/superset-frontend/src/SqlLab/components/AceEditorWrapper/useKeywords.test.ts index 350e7ec12109c..e0f49b70e316f 100644 --- a/superset-frontend/src/SqlLab/components/AceEditorWrapper/useKeywords.test.ts +++ b/superset-frontend/src/SqlLab/components/AceEditorWrapper/useKeywords.test.ts @@ -202,6 +202,7 @@ test('returns column keywords among selected tables', async () => { { name: expectColumn, type: 'VARCHAR', + longType: 'VARCHAR', }, ], }, @@ -223,6 +224,7 @@ test('returns column keywords among selected tables', async () => { { name: unexpectedColumn, type: 'VARCHAR', + longType: 'VARCHAR', }, ], }, diff --git a/superset-frontend/src/SqlLab/components/App/index.tsx b/superset-frontend/src/SqlLab/components/App/index.tsx index 7da80d8e9c965..8b9c2790834cf 100644 --- a/superset-frontend/src/SqlLab/components/App/index.tsx +++ b/superset-frontend/src/SqlLab/components/App/index.tsx @@ -47,7 +47,7 @@ const SqlLabStyles = styled.div` left: 0; padding: 0 ${theme.gridUnit * 2}px; - pre { + pre:not(.code) { padding: 0 !important; margin: 0; border: none; diff --git a/superset-frontend/src/SqlLab/components/ShowSQL/index.tsx b/superset-frontend/src/SqlLab/components/ShowSQL/index.tsx index 3e0192d051410..4ae6f3b0edac9 100644 --- a/superset-frontend/src/SqlLab/components/ShowSQL/index.tsx +++ b/superset-frontend/src/SqlLab/components/ShowSQL/index.tsx @@ -28,21 +28,25 @@ interface ShowSQLProps { sql: string; title: string; tooltipText: string; + triggerNode?: React.ReactNode; } export default function ShowSQL({ tooltipText, title, sql: sqlString, + triggerNode, }: ShowSQLProps) { return ( + triggerNode || ( + + ) } modalBody={
diff --git a/superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.tsx b/superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.tsx index 3fc6df96fba4c..0d30c46f1e91d 100644 --- a/superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.tsx +++ b/superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.tsx @@ -135,7 +135,7 @@ test('should render empty result state when latestQuery is empty', () => { expect(resultPanel).toHaveTextContent('Run a query to display results'); }); -test('should render tabs for table preview queries', () => { +test('should render tabs for table metadata view', () => { const { getAllByRole } = render(, { useRedux: true, initialState: mockState, @@ -145,7 +145,7 @@ test('should render tabs for table preview queries', () => { expect(tabs).toHaveLength(mockState.sqlLab.tables.length + 2); expect(tabs[0]).toHaveTextContent('Results'); expect(tabs[1]).toHaveTextContent('Query history'); - mockState.sqlLab.tables.forEach(({ name }, index) => { - expect(tabs[index + 2]).toHaveTextContent(`Preview: \`${name}\``); + mockState.sqlLab.tables.forEach(({ name, schema }, index) => { + expect(tabs[index + 2]).toHaveTextContent(`${schema}.${name}`); }); }); diff --git a/superset-frontend/src/SqlLab/components/SouthPane/index.tsx b/superset-frontend/src/SqlLab/components/SouthPane/index.tsx index 7af63b39580b5..dbde83ae9d693 100644 --- a/superset-frontend/src/SqlLab/components/SouthPane/index.tsx +++ b/superset-frontend/src/SqlLab/components/SouthPane/index.tsx @@ -16,24 +16,25 @@ * specific language governing permissions and limitations * under the License. */ -import { createRef, useMemo } from 'react'; +import { createRef, useCallback, useMemo } from 'react'; import { shallowEqual, useDispatch, useSelector } from 'react-redux'; import { nanoid } from 'nanoid'; import Tabs from 'src/components/Tabs'; -import { styled, t } from '@superset-ui/core'; +import { css, styled, t } from '@superset-ui/core'; -import { setActiveSouthPaneTab } from 'src/SqlLab/actions/sqlLab'; +import { removeTables, setActiveSouthPaneTab } from 'src/SqlLab/actions/sqlLab'; import Label from 'src/components/Label'; +import Icons from 'src/components/Icons'; import { SqlLabRootState } from 'src/SqlLab/types'; import QueryHistory from '../QueryHistory'; -import ResultSet from '../ResultSet'; import { STATUS_OPTIONS, STATE_TYPE_MAP, STATUS_OPTIONS_LOCALIZED, } from '../../constants'; import Results from './Results'; +import TablePreview from '../TablePreview'; const TAB_HEIGHT = 130; @@ -98,31 +99,45 @@ const SouthPane = ({ }), shallowEqual, ); - const queries = useSelector( - ({ sqlLab: { queries } }: SqlLabRootState) => Object.keys(queries), - shallowEqual, - ); const activeSouthPaneTab = useSelector( state => state.sqlLab.activeSouthPaneTab as string, ) ?? 'Results'; - const querySet = useMemo(() => new Set(queries), [queries]); - const dataPreviewQueries = useMemo( + const pinnedTables = useMemo( () => tables.filter( - ({ dataPreviewQueryId, queryEditorId: qeId }) => - dataPreviewQueryId && - queryEditorId === qeId && - querySet.has(dataPreviewQueryId), + ({ queryEditorId: qeId }) => String(queryEditorId) === qeId, + ), + [queryEditorId, tables], + ); + const pinnedTableKeys = useMemo( + () => + Object.fromEntries( + pinnedTables.map(({ id, dbId, catalog, schema, name }) => [ + id, + [dbId, catalog, schema, name].join(':'), + ]), ), - [queryEditorId, tables, querySet], + [pinnedTables], ); const innerTabContentHeight = height - TAB_HEIGHT; const southPaneRef = createRef(); const switchTab = (id: string) => { dispatch(setActiveSouthPaneTab(id)); }; + const removeTable = useCallback( + (key, action) => { + if (action === 'remove') { + const table = pinnedTables.find( + ({ dbId, catalog, schema, name }) => + [dbId, catalog, schema, name].join(':') === key, + ); + dispatch(removeTables([table])); + } + }, + [dispatch, queryEditorId], + ); return offline ? (