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

feat(sqllab): improve table metadata UI #32051

Merged
merged 4 commits into from
Jan 31, 2025
Merged

feat(sqllab): improve table metadata UI #32051

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
f693cac
feat(sqllab): improve table metadata UI
justinpark Jan 30, 2025
f20e9a0
design the tab key by immutable items to optimize the rerendering
justinpark Jan 30, 2025
a739904
disableDataPreview
justinpark Jan 30, 2025
f93dba5
fix type
justinpark Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions superset-frontend/packages/superset-ui-core/src/ui-overrides/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -229,4 +236,8 @@ export type Extensions = Partial<{
'sqleditor.extension.customAutocomplete': (
args: CustomAutoCompleteArgs,
) => CustomAutocomplete[] | undefined;
'sqleditor.extension.tablePreview': [
string,
ComponentType<SQLTablePreviewExtensionProps>,
][];
}>;
40 changes: 18 additions & 22 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 @@ -237,15 +237,15 @@ 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,
startDttm: now(),
state: query.runAsync ? 'pending' : 'running',
cached: false,
});
return { type: START_QUERY, query };
return { type: START_QUERY, query, runPreviewOnly };
}

export function querySuccess(query, results) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 },
Expand All @@ -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,
Expand All @@ -991,6 +987,9 @@ export function runTablePreviewQuery(newTable) {
ctas: false,
isDataPreview: true,
};
if (runPreviewOnly) {
return dispatch(runQuery(dataPreviewQuery, runPreviewOnly));
}
return Promise.all([
dispatch(
mergeTable(
Expand Down Expand Up @@ -1024,17 +1023,14 @@ 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,
expanded: true,
initialized: true,
}),
);
if (!table.dataPreviewQueryId) {
dispatch(runTablePreviewQuery({ ...tableMetadata, ...newTable }));
}
})
.catch(() =>
dispatch(
Expand Down
70 changes: 51 additions & 19 deletions superset-frontend/src/SqlLab/actions/sqlLab.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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', () => {
Expand Down
19 changes: 8 additions & 11 deletions superset-frontend/src/SqlLab/components/AceEditorWrapper/AceEditorWrapper.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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: [],
});
Expand Down Expand Up @@ -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(
Expand All @@ -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,
Expand All @@ -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),
Expand All @@ -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);
});
});
2 changes: 2 additions & 0 deletions superset-frontend/src/SqlLab/components/AceEditorWrapper/useKeywords.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ test('returns column keywords among selected tables', async () => {
{
name: expectColumn,
type: 'VARCHAR',
longType: 'VARCHAR',
},
],
},
Expand All @@ -223,6 +224,7 @@ test('returns column keywords among selected tables', async () => {
{
name: unexpectedColumn,
type: 'VARCHAR',
longType: 'VARCHAR',
},
],
},
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/SqlLab/components/App/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ const SqlLabStyles = styled.div`
left: 0;
padding: 0 ${theme.gridUnit * 2}px;

pre {
pre:not(.code) {
Copy link

Choose a reason for hiding this comment

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

Unclear CSS Selector Exclusion category Readability and Maintainability

Tell me more
What is the issue?

The CSS selector .code is used in a negation without context or explanation for why certain pre elements should be excluded.

Why this matters

Without understanding which pre elements have the .code class and why they need different styling, future maintainers might accidentally break the styling when modifying the codebase.

Suggested change ∙ Feature Preview

Add a comment explaining which pre elements have the .code class and why they need different styling:

/* Exclude pre elements with .code class which are used for syntax highlighting */
pre:not(.code) {

Report a problem with this comment

💬 Chat with Korbit by mentioning @korbit-ai.

padding: 0 !important;
margin: 0;
border: none;
Expand Down
12 changes: 8 additions & 4 deletions superset-frontend/src/SqlLab/components/ShowSQL/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<ModalTrigger
modalTitle={title}
triggerNode={
<IconTooltip
className="fa fa-eye pull-left m-l-2"
tooltip={tooltipText}
/>
triggerNode || (
<IconTooltip
className="fa fa-eye pull-left m-l-2"
tooltip={tooltipText}
/>
)
}
modalBody={
<div>
Expand Down
6 changes: 3 additions & 3 deletions superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(<SouthPane {...mockedProps} />, {
useRedux: true,
initialState: mockState,
Expand All @@ -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}`);
});
});
Loading
Loading