Copilot commented on code in PR #35969:
URL: https://github.com/apache/superset/pull/35969#discussion_r2496670649
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -25,7 +25,8 @@ import {
getClientErrorObject,
QueryFormData,
} from '@superset-ui/core';
-import { Loading } from '@superset-ui/core/components';
+import { Loading, Alert } from '@superset-ui/core/components';
Review Comment:
The `Alert` import should be verified. Based on the typical structure of
@superset-ui/core, Alert components are usually imported from a different path
(e.g., from the package that provides Ant Design components). Please verify
that this import path is correct for the Alert component.
```suggestion
import { Loading } from '@superset-ui/core/components';
import { Alert } from 'antd';
```
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -87,16 +89,26 @@ const ViewQueryModal: FC<Props> = ({ latestQueryFormData })
=> {
return (
<ViewQueryModalContainer>
- {result.map((item, index) =>
- item.query ? (
- <ViewQuery
- key={`query-${index}`}
- datasource={latestQueryFormData.datasource}
- sql={item.query}
- language="sql"
- />
- ) : null,
- )}
+ {result.map((item, index) => (
+ <Fragment key={index}>
+ {item.error && (
+ <Alert
+ key={`error-${index}`}
+ type="error"
+ message={item.error}
+ closable={false}
+ />
+ )}
+ {item.query && (
+ <ViewQuery
+ key={`query-${index}`}
+ datasource={latestQueryFormData.datasource}
+ sql={item.query}
+ language={item.language}
+ />
+ )}
+ </Fragment>
+ ))}
Review Comment:
Using `index` as the React key is an anti-pattern when the list order can
change or items can be added/removed. Consider using a more stable identifier,
such as combining it with a unique property from the item (e.g.,
`${item.query?.substring(0, 20) || item.error?.substring(0, 20)}-${index}`).
```suggestion
{result.map((item, index) => {
const stableKey =
(item.query?.substring(0, 20) || item.error?.substring(0, 20) ||
'') +
'-' +
index;
return (
<Fragment key={stableKey}>
{item.error && (
<Alert
key={`error-${stableKey}`}
type="error"
message={item.error}
closable={false}
/>
)}
{item.query && (
<ViewQuery
key={`query-${stableKey}`}
datasource={latestQueryFormData.datasource}
sql={item.query}
language={item.language}
/>
)}
</Fragment>
);
})}
```
##########
superset/common/query_actions.py:
##########
@@ -86,7 +86,15 @@ def _get_query(
try:
result["query"] = datasource.get_query_str(query_obj.to_dict())
except QueryObjectValidationError as err:
+ # Validation errors (missing required fields, invalid config)
+ # No SQL was generated
result["error"] = err.message
+ except SupersetParseError as err:
+ # Parsing errors (SQL optimization/parsing failed)
+ # SQL was generated but couldn't be optimized - show both
+ if err.error.extra:
+ result["query"] = err.error.extra.get("sql")
Review Comment:
The conditional check `if err.error.extra:` should also verify that the
'sql' key exists before attempting to get it. Consider using `if
err.error.extra and 'sql' in err.error.extra:` or check if the value is not
None after getting it to avoid setting `result['query']` to `None`.
```suggestion
if err.error.extra and "sql" in err.error.extra and
err.error.extra["sql"] is not None:
result["query"] = err.error.extra["sql"]
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
