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

chore(sqllab): Remove schemaOptions from redux store #23257

Merged
merged 3 commits into from
Mar 22, 2023
Merged

chore(sqllab): Remove schemaOptions from redux store #23257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd100e7
chore(sqllab): Remove schemaOptions from redux store
justinpark Feb 3, 2023
3adc844
remove unused schemaOptions
justinpark Mar 1, 2023
4d0f3d7
remove unused ref
justinpark Mar 1, 2023
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
6 changes: 0 additions & 6 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 @@ -48,8 +48,6 @@ export const EXPAND_TABLE = 'EXPAND_TABLE';
export const COLLAPSE_TABLE = 'COLLAPSE_TABLE';
export const QUERY_EDITOR_SETDB = 'QUERY_EDITOR_SETDB';
export const QUERY_EDITOR_SET_SCHEMA = 'QUERY_EDITOR_SET_SCHEMA';
export const QUERY_EDITOR_SET_SCHEMA_OPTIONS =
'QUERY_EDITOR_SET_SCHEMA_OPTIONS';
export const QUERY_EDITOR_SET_TABLE_OPTIONS = 'QUERY_EDITOR_SET_TABLE_OPTIONS';
export const QUERY_EDITOR_SET_TITLE = 'QUERY_EDITOR_SET_TITLE';
export const QUERY_EDITOR_SET_AUTORUN = 'QUERY_EDITOR_SET_AUTORUN';
Expand Down Expand Up @@ -922,10 +920,6 @@ export function queryEditorSetSchema(queryEditor, schema) {
};
}

export function queryEditorSetSchemaOptions(queryEditor, options) {
return { type: QUERY_EDITOR_SET_SCHEMA_OPTIONS, queryEditor, options };
}

export function queryEditorSetTableOptions(queryEditor, options) {
return { type: QUERY_EDITOR_SET_TABLE_OPTIONS, queryEditor, options };
}
Expand Down
1 change: 0 additions & 1 deletion 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 @@ -44,7 +44,6 @@ describe('async actions', () => {
latestQueryId: null,
sql: 'SELECT *\nFROM\nWHERE',
name: 'Untitled Query 1',
schemaOptions: [{ value: 'main', label: 'main', title: 'main' }],
};

let dispatch;
Expand Down
32 changes: 20 additions & 12 deletions superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { useState, useEffect, useRef } from 'react';
import React, { useState, useEffect, useRef, useMemo } from 'react';
import { useDispatch } from 'react-redux';
import { css, styled } from '@superset-ui/core';

Expand All @@ -40,6 +40,7 @@ import {
FullSQLEditor as AceEditor,
} from 'src/components/AsyncAceEditor';
import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor';
import { useSchemas } from 'src/hooks/apiResources';

type HotKey = {
key: string;
Expand Down Expand Up @@ -81,6 +82,7 @@ const StyledAceEditor = styled(AceEditor)`
}
`}
`;

const AceEditorWrapper = ({
autocomplete,
onBlur = () => {},
Expand All @@ -98,14 +100,27 @@ const AceEditorWrapper = ({
'dbId',
'sql',
'functionNames',
'schemaOptions',
'tableOptions',
'validationResult',
'schema',
]);
const { data: schemaOptions } = useSchemas({ dbId: queryEditor.dbId });
const currentSql = queryEditor.sql ?? '';
const functionNames = queryEditor.functionNames ?? [];
const schemas = queryEditor.schemaOptions ?? [];

// Loading schema, table and column names as auto-completable words
const { schemas, schemaWords } = useMemo(
() => ({
schemas: schemaOptions ?? [],
schemaWords: (schemaOptions ?? []).map(s => ({
name: s.label,
value: s.value,
score: SCHEMA_AUTOCOMPLETE_SCORE,
meta: 'schema',
})),
}),
[schemaOptions],
);
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need a useMemo here? It may look cleaner to keep the words logic with other autocomplete words.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. useMemo will reduce the cost of schemaWords mapping from schemaOptions.
(For example, airbnb has > 10,000 schema options so useMemo should be needed)

const tables = queryEditor.tableOptions ?? [];

const [sql, setSql] = useState(currentSql);
Expand Down Expand Up @@ -193,14 +208,7 @@ const AceEditorWrapper = ({
onChange(text);
};

const setAutoCompleter = () => {
// Loading schema, table and column names as auto-completable words
const schemaWords = schemas.map(s => ({
name: s.label,
value: s.value,
score: SCHEMA_AUTOCOMPLETE_SCORE,
meta: 'schema',
}));
function setAutoCompleter() {
const columns = {};

const tableWords = tables.map(t => {
Expand Down Expand Up @@ -264,7 +272,7 @@ const AceEditorWrapper = ({
}));

setWords(words);
};
}

const getAceAnnotations = () => {
const { validationResult } = queryEditor;
Expand Down
1 change: 0 additions & 1 deletion superset-frontend/src/SqlLab/components/SaveQuery/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ const SaveQuery = ({
'latestQueryId',
'queryLimit',
'schema',
'schemaOptions',
'selectedText',
'sql',
'tableOptions',
Expand Down
11 changes: 0 additions & 11 deletions superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import {
expandTable,
queryEditorSetSchema,
queryEditorSetTableOptions,
queryEditorSetSchemaOptions,
setDatabases,
addDangerToast,
resetState,
Expand Down Expand Up @@ -228,15 +227,6 @@ const SqlEditorLeftBar = ({
[dispatch, queryEditor],
);

const handleSchemasLoad = useCallback(
(options: Array<any>) => {
if (queryEditor) {
dispatch(queryEditorSetSchemaOptions(queryEditor, options));
}
},
[dispatch, queryEditor],
);

const handleDbList = useCallback(
(result: DatabaseObject) => {
dispatch(setDatabases(result));
Expand Down Expand Up @@ -265,7 +255,6 @@ const SqlEditorLeftBar = ({
handleError={handleError}
onDbChange={onDbChange}
onSchemaChange={handleSchemaChange}
onSchemasLoad={handleSchemasLoad}
onTableSelectChange={onTablesChange}
onTablesLoad={handleTablesLoad}
schema={schema}
Expand Down
7 changes: 0 additions & 7 deletions superset-frontend/src/SqlLab/fixtures.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,13 +188,6 @@ export const defaultQueryEditor = {
tableOptions: [],
functionNames: [],
hideLeftBar: false,
schemaOptions: [
{
value: 'main',
label: 'main',
title: 'main',
},
],
templateParams: '{}',
};

Expand Down
9 changes: 2 additions & 7 deletions superset-frontend/src/SqlLab/hooks/useQueryEditor/useQueryEditor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,7 @@ const mockStore = configureStore(middlewares);
test('returns selected queryEditor values', () => {
const { result } = renderHook(
() =>
useQueryEditor(defaultQueryEditor.id, [
'id',
'name',
'dbId',
'schemaOptions',
]),
useQueryEditor(defaultQueryEditor.id, ['id', 'name', 'dbId', 'schema']),
{
wrapper: createWrapper({
useRedux: true,
Expand All @@ -47,7 +42,7 @@ test('returns selected queryEditor values', () => {
id: defaultQueryEditor.id,
name: defaultQueryEditor.name,
dbId: defaultQueryEditor.dbId,
schemaOptions: defaultQueryEditor.schemaOptions,
schema: defaultQueryEditor.schema,
});
});

Expand Down
12 changes: 0 additions & 12 deletions superset-frontend/src/SqlLab/reducers/sqlLab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,18 +587,6 @@ export default function sqlLabReducer(state = {}, action) {
),
};
},
[actions.QUERY_EDITOR_SET_SCHEMA_OPTIONS]() {
return {
...state,
...alterUnsavedQueryEditorState(
state,
{
schemaOptions: action.options,
},
action.queryEditor.id,
),
};
},
[actions.QUERY_EDITOR_SET_TABLE_OPTIONS]() {
return {
...state,
Expand Down
1 change: 0 additions & 1 deletion superset-frontend/src/SqlLab/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ export interface QueryEditor {
sql: string;
remoteId: number | null;
tableOptions: any[];
schemaOptions?: SchemaOption[];
functionNames: string[];
validationResult?: {
completed: boolean;
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/src/SqlLab/utils/emptyQueryResults.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ describe('reduxStateToLocalStorageHelper', () => {

it('should only return selected keys for query editor', () => {
const queryEditors = [defaultQueryEditor];
expect(Object.keys(queryEditors[0])).toContain('schemaOptions');
expect(Object.keys(queryEditors[0])).toContain('schema');

const clearedQueryEditors = clearQueryEditors(queryEditors);
expect(Object.keys(clearedQueryEditors)[0]).not.toContain('schemaOptions');
expect(Object.keys(clearedQueryEditors)[0]).not.toContain('schema');
});
});
1 change: 0 additions & 1 deletion superset-frontend/src/components/DatabaseSelector/DatabaseSelector.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ const createProps = (): DatabaseSelectorProps => ({
handleError: jest.fn(),
onDbChange: jest.fn(),
onSchemaChange: jest.fn(),
onSchemasLoad: jest.fn(),
});

const fakeDatabaseApiResult = {
Expand Down
4 changes: 0 additions & 4 deletions superset-frontend/src/components/DatabaseSelector/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ export interface DatabaseSelectorProps {
onDbChange?: (db: DatabaseObject) => void;
onEmptyResults?: (searchText?: string) => void;
onSchemaChange?: (schema?: string) => void;
onSchemasLoad?: (schemas: Array<object>) => void;
readOnly?: boolean;
schema?: string;
sqlLabMode?: boolean;
Expand Down Expand Up @@ -126,7 +125,6 @@ export default function DatabaseSelector({
onDbChange,
onEmptyResults,
onSchemaChange,
onSchemasLoad,
readOnly = false,
schema,
sqlLabMode = false,
Expand Down Expand Up @@ -230,8 +228,6 @@ export default function DatabaseSelector({
} = useSchemas({
dbId: currentDb?.value,
onSuccess: data => {
onSchemasLoad?.(data);

if (data.length === 1) {
changeSchema(data[0]);
} else if (!data.find(schemaOption => schema === schemaOption.value)) {
Expand Down
4 changes: 0 additions & 4 deletions superset-frontend/src/components/TableSelector/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import RefreshLabel from 'src/components/RefreshLabel';
import CertifiedBadge from 'src/components/CertifiedBadge';
import WarningIconWithTooltip from 'src/components/WarningIconWithTooltip';
import { useToasts } from 'src/components/MessageToasts/withToasts';
import { SchemaOption } from 'src/SqlLab/types';
import { useTables, Table } from 'src/hooks/apiResources';
import {
getClientErrorMessage,
Expand Down Expand Up @@ -98,7 +97,6 @@ interface TableSelectorProps {
isDatabaseSelectEnabled?: boolean;
onDbChange?: (db: DatabaseObject) => void;
onSchemaChange?: (schema?: string) => void;
onSchemasLoad?: (schemaOptions: SchemaOption[]) => void;
onTablesLoad?: (options: Array<any>) => void;
readOnly?: boolean;
schema?: string;
Expand Down Expand Up @@ -160,7 +158,6 @@ const TableSelector: FunctionComponent<TableSelectorProps> = ({
isDatabaseSelectEnabled = true,
onDbChange,
onSchemaChange,
onSchemasLoad,
onTablesLoad,
readOnly = false,
onEmptyResults,
Expand Down Expand Up @@ -335,7 +332,6 @@ const TableSelector: FunctionComponent<TableSelectorProps> = ({
onDbChange={readOnly ? undefined : internalDbChange}
onEmptyResults={onEmptyResults}
onSchemaChange={readOnly ? undefined : internalSchemaChange}
onSchemasLoad={onSchemasLoad}
schema={currentSchema}
sqlLabMode={sqlLabMode}
isDatabaseSelectEnabled={isDatabaseSelectEnabled && !readOnly}
Expand Down