korbit-ai[bot] commented on code in PR #35077:
URL: https://github.com/apache/superset/pull/35077#discussion_r2335539365
##########
superset-frontend/packages/superset-core/src/api/sqlLab.ts:
##########
@@ -111,6 +111,215 @@ export interface Tab {
panels: Panel[];
}
+// Keep in sync with superset/errors.py
+export type ErrorLevel = 'info' | 'warning' | 'error';
+
+/**
+ * Superset error object structure.
+ * Contains details about an error that occurred within Superset.
+ */
+export type SupersetError<ExtraType = Record<string, any> | null> = {
+ /**
+ * Error types, see enum of SupersetErrorType in superset/errors.py
+ */
+ error_type: string;
+
+ /**
+ * Extra properties based on the error types
+ */
+ extra: ExtraType;
+
+ /**
+ * Level of the error type
+ */
+ level: ErrorLevel;
+
+ /**
+ * Detail description for the error
+ */
+ message: string;
+};
+
+/**
+ * Generic data types, see enum of the same name in superset/utils/core.py.
+ */
+enum GenericDataType {
+ Numeric = 0,
+ String = 1,
+ Temporal = 2,
+ Boolean = 3,
+}
+
+/**
+ * Column metadata returned in query results.
+ */
+export type QueryColumn = {
+ /**
+ * Label of the column
+ */
+ name?: string;
+
+ /**
+ * Column name defined
+ */
+ column_name: string;
+
+ /**
+ * Type of the column value
+ */
+ type: string | null;
+
+ /**
+ * Generic data type format
+ */
+ type_generic: GenericDataType;
+
+ /**
+ * True if the column is date format
+ */
+ is_dttm: boolean;
+};
+
+/**
+ * Possible states of a query during its lifecycle.
+ */
+enum QueryState {
+ Started = 'started',
+ Stopped = 'stopped',
+ Failed = 'failed',
+ Pending = 'pending',
+ Running = 'running',
+ Scheduled = 'scheduled',
+ Success = 'success',
+ Fetching = 'fetching',
+ TimedOut = 'timed_out',
+}
+
+export enum CTASMethod {
+ Table = 'TABLE',
+ View = 'VIEW',
+}
+
+export interface CTAS {
+ /**
+ * Create method for CTAS creation request
+ */
+ method: CTASMethod;
+
+ /**
+ * Temporary table name for creation using a CTAS query
+ */
+ tempTable: string | null;
+}
+
+export interface QueryRequestContext {
+ /**
+ * Unique query ID on client side
+ */
+ id: string;
+
+ /**
+ * Contains CTAS if the query requests table creation
+ */
+ ctas: CTAS | null;
+
+ /**
+ * The SQL editor instance associated with the query
+ */
+ editor: Editor;
+
+ /**
+ * Requested row limit for the query
+ */
+ queryLimit: number | null;
+
+ /**
+ * True if the query execution result will be/was delivered asynchronously
+ */
+ runAsync?: boolean;
Review Comment:
### Optional Async Flag Performance Overhead <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The QueryRequestContext interface has an optional runAsync flag without a
default value, requiring additional runtime checks.
###### Why this matters
Each time the runAsync property is accessed, a runtime check for undefined
must be performed, adding unnecessary CPU cycles.
###### Suggested change ∙ *Feature Preview*
Make the flag non-optional with a default value to avoid runtime checks:
```typescript
runAsync: boolean = false;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa330eb8-cc22-4432-93b1-f10a0cf7fb03/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa330eb8-cc22-4432-93b1-f10a0cf7fb03?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa330eb8-cc22-4432-93b1-f10a0cf7fb03?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa330eb8-cc22-4432-93b1-f10a0cf7fb03?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa330eb8-cc22-4432-93b1-f10a0cf7fb03)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a49208cc-5052-46ed-b438-1174172060ab -->
[](a49208cc-5052-46ed-b438-1174172060ab)
##########
superset-frontend/src/core/sqlLab.ts:
##########
@@ -66,34 +81,168 @@ const predicate = (actionType: string):
AnyListenerPredicate<RootState> => {
};
export const onDidQueryRun: typeof sqlLabType.onDidQueryRun = (
- listener: (editor: sqlLabType.Editor) => void,
+ listener: (editor: sqlLabType.QueryRequestContext) => void,
+ thisArgs?: any,
+): Disposable =>
+ createActionListener(
+ predicate(START_QUERY),
+ listener,
+ (action: ReturnType<typeof startQuery>) => {
+ const { query } = action;
+ const {
+ id,
+ dbId,
+ catalog,
+ schema,
+ sql,
+ startDttm,
+ ctas_method: ctasMethod,
+ runAsync,
+ tempTable,
+ templateParams,
+ } = query;
+ const editor = new Editor(sql, dbId, catalog, schema);
+ const panels: Panel[] = []; // TODO: Populate panels
+ const tab = new Tab(query.sqlEditorId, query.tab, editor, panels);
Review Comment:
### Query Context Creation Code Duplication <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
This code block is duplicated across onDidQueryRun, onDidQuerySuccess,
onDidQueryStop, and onDidQueryFail functions with minimal variation.
###### Why this matters
Code duplication increases maintenance burden and risk of inconsistencies
when changes are needed across all query-related functions.
###### Suggested change ∙ *Feature Preview*
Extract the common editor and tab creation logic into a shared utility
function:
```typescript
const createQueryContext = (query: QueryType) => {
const editor = new Editor(query.sql, query.dbId, query.catalog,
query.schema);
const panels: Panel[] = [];
return {
editor,
tab: new Tab(query.sqlEditorId, query.tab, editor, panels)
};
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0c95f045-111c-47b8-9ef8-d91c3d7f48e4/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0c95f045-111c-47b8-9ef8-d91c3d7f48e4?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0c95f045-111c-47b8-9ef8-d91c3d7f48e4?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0c95f045-111c-47b8-9ef8-d91c3d7f48e4?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0c95f045-111c-47b8-9ef8-d91c3d7f48e4)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5e32b6ff-54a1-485e-be3c-b4aa6a7fb5e9 -->
[](5e32b6ff-54a1-485e-be3c-b4aa6a7fb5e9)
##########
superset-frontend/packages/superset-core/src/api/sqlLab.ts:
##########
@@ -111,6 +111,215 @@
panels: Panel[];
}
+// Keep in sync with superset/errors.py
+export type ErrorLevel = 'info' | 'warning' | 'error';
+
+/**
+ * Superset error object structure.
+ * Contains details about an error that occurred within Superset.
+ */
+export type SupersetError<ExtraType = Record<string, any> | null> = {
+ /**
+ * Error types, see enum of SupersetErrorType in superset/errors.py
+ */
+ error_type: string;
+
+ /**
+ * Extra properties based on the error types
+ */
+ extra: ExtraType;
+
+ /**
+ * Level of the error type
+ */
+ level: ErrorLevel;
+
+ /**
+ * Detail description for the error
+ */
+ message: string;
+};
+
+/**
+ * Generic data types, see enum of the same name in superset/utils/core.py.
+ */
+enum GenericDataType {
+ Numeric = 0,
+ String = 1,
+ Temporal = 2,
+ Boolean = 3,
+}
+
+/**
+ * Column metadata returned in query results.
+ */
+export type QueryColumn = {
+ /**
+ * Label of the column
+ */
+ name?: string;
+
+ /**
+ * Column name defined
+ */
+ column_name: string;
+
+ /**
+ * Type of the column value
+ */
+ type: string | null;
+
+ /**
+ * Generic data type format
+ */
+ type_generic: GenericDataType;
+
+ /**
+ * True if the column is date format
+ */
+ is_dttm: boolean;
+};
+
+/**
+ * Possible states of a query during its lifecycle.
+ */
+enum QueryState {
+ Started = 'started',
+ Stopped = 'stopped',
+ Failed = 'failed',
+ Pending = 'pending',
+ Running = 'running',
+ Scheduled = 'scheduled',
+ Success = 'success',
+ Fetching = 'fetching',
+ TimedOut = 'timed_out',
+}
Review Comment:
### Unused Query State Validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
QueryState enum is defined but never used in any of the interfaces or event
handlers that deal with query states.
###### Why this matters
Without using the QueryState enum in the relevant interfaces, there's no
type safety ensuring that query state transitions are valid, potentially
allowing invalid states to be set.
###### Suggested change ∙ *Feature Preview*
Add a state property using the QueryState enum to the relevant interfaces:
```typescript
export interface QueryRequestContext {
state: QueryState;
// ... other properties
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a7385bf-839b-4006-b158-46fffd2fc897/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a7385bf-839b-4006-b158-46fffd2fc897?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a7385bf-839b-4006-b158-46fffd2fc897?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a7385bf-839b-4006-b158-46fffd2fc897?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a7385bf-839b-4006-b158-46fffd2fc897)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:23ab0060-6070-4be3-bd6d-e5456122d9ff -->
[](23ab0060-6070-4be3-bd6d-e5456122d9ff)
##########
superset-frontend/src/core/sqlLab.ts:
##########
@@ -66,34 +81,168 @@
};
export const onDidQueryRun: typeof sqlLabType.onDidQueryRun = (
- listener: (editor: sqlLabType.Editor) => void,
+ listener: (editor: sqlLabType.QueryRequestContext) => void,
+ thisArgs?: any,
+): Disposable =>
+ createActionListener(
+ predicate(START_QUERY),
+ listener,
+ (action: ReturnType<typeof startQuery>) => {
+ const { query } = action;
+ const {
+ id,
+ dbId,
+ catalog,
+ schema,
+ sql,
+ startDttm,
+ ctas_method: ctasMethod,
+ runAsync,
+ tempTable,
+ templateParams,
+ } = query;
+ const editor = new Editor(sql, dbId, catalog, schema);
+ const panels: Panel[] = []; // TODO: Populate panels
+ const tab = new Tab(query.sqlEditorId, query.tab, editor, panels);
+ return new QueryRequestContext(id, editor, tab, runAsync, startDttm, {
+ ctasMethod,
+ tempTable,
+ templateParams,
+ });
+ },
+ thisArgs,
+ );
+
+export const onDidQuerySuccess: typeof sqlLabType.onDidQuerySuccess = (
+ listener: (query: sqlLabType.QueryResultContext) => void,
thisArgs?: any,
): Disposable =>
createActionListener(
predicate(QUERY_SUCCESS),
listener,
(action: ReturnType<typeof querySuccess>) => {
+ const { query, results } = action;
+ const {
+ id,
+ dbId,
+ catalog,
+ schema,
+ sql,
+ startDttm,
+ ctas_method: ctasMethod,
+ runAsync,
+ templateParams,
+ } = query;
+ const {
+ query_id: queryId,
+ columns,
+ data,
+ query: { endDttm, executedSql, tempTable, limit, limitingFactor },
+ } = results;
+ const editor = new Editor(sql, dbId, catalog, schema);
+ const panels: Panel[] = []; // TODO: Populate panels
+ const tab = new Tab(query.sqlEditorId, query.tab, editor, panels);
+ return new QueryResultContext(
+ id,
+ queryId,
+ executedSql ?? sql,
+ columns,
+ data,
+ editor,
+ tab,
+ runAsync,
+ startDttm,
+ endDttm,
+ {
+ ctasMethod,
+ tempTable,
+ templateParams,
+ limit,
+ limitingFactor,
+ },
+ );
+ },
+ thisArgs,
+ );
+
+export const onDidQueryStop: typeof sqlLabType.onDidQueryStop = (
+ listener: (query: sqlLabType.QueryRequestContext) => void,
+ thisArgs?: any,
+): Disposable =>
+ createActionListener(
+ predicate(STOP_QUERY),
+ listener,
+ (action: ReturnType<typeof stopQuery>) => {
const { query } = action;
- const { dbId, catalog, schema, sql } = query;
- return new Editor(sql, dbId, catalog, schema);
+ const {
+ id,
+ dbId,
+ catalog,
+ schema,
+ sql,
+ startDttm,
+ ctas_method: ctasMethod,
+ runAsync,
+ tempTable,
+ templateParams,
+ } = query;
+ const editor = new Editor(sql, dbId, catalog, schema);
+ const panels: Panel[] = []; // TODO: Populate panels
+ const tab = new Tab(query.sqlEditorId, query.tab, editor, panels);
+ return new QueryRequestContext(id, editor, tab, runAsync, startDttm, {
+ ctasMethod,
+ tempTable,
+ templateParams,
+ });
},
thisArgs,
);
export const onDidQueryFail: typeof sqlLabType.onDidQueryFail = (
- listener: (e: string) => void,
+ listener: (query: sqlLabType.QueryErrorResultContext) => void,
thisArgs?: any,
): Disposable =>
createActionListener(
- predicate(QUERY_FAILED),
+ action => action.type === QUERY_FAILED,
listener,
Review Comment:
### Inconsistent Query Event Handling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The onDidQueryFail event handler doesn't use the same predicate function as
other query handlers, potentially causing inconsistent event filtering behavior.
###### Why this matters
Query failure events may be handled differently from other query events,
leading to inconsistent state management and potential race conditions in the
UI.
###### Suggested change ∙ *Feature Preview*
Update the onDidQueryFail handler to use the same predicate function:
```typescript
export const onDidQueryFail: typeof sqlLabType.onDidQueryFail = (
listener: (query: sqlLabType.QueryErrorResultContext) => void,
thisArgs?: any,
): Disposable =>
createActionListener(
predicate(QUERY_FAILED),
listener,
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1daf4e93-8fd2-4216-91d5-1ee5de60dcbd/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1daf4e93-8fd2-4216-91d5-1ee5de60dcbd?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1daf4e93-8fd2-4216-91d5-1ee5de60dcbd?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1daf4e93-8fd2-4216-91d5-1ee5de60dcbd?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1daf4e93-8fd2-4216-91d5-1ee5de60dcbd)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a313c6d2-e1fc-45d6-aaae-9d83a2f4a441 -->
[](a313c6d2-e1fc-45d6-aaae-9d83a2f4a441)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
