You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "justinpark (via GitHub)" <gi...@apache.org> on 2025/05/02 00:21:30 UTC
[PR] chore(explore): Add format sql and view in SQL Lab option in View Query [superset]
justinpark opened a new pull request, #33341:
URL: https://github.com/apache/superset/pull/33341
### SUMMARY
The "View Query" feature was previously used to display SQL in a formatted manner. However, the prettier formatting was reverted in [this pull request](https://github.com/apache/superset/pull/30350) due to unintended changes in the original SQL. To minimize the impact while retaining the formatting option, this commit introduces a "Format SQL" feature in SQL Lab and an option to show the original SQL for comparison. Additionally, this commit adds a "View in SQL Lab" button, allowing users to conveniently connect to SQL Lab directly from the dashboard, which is particularly useful for users.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
https://github.com/user-attachments/assets/504b1e61-0dad-4c1a-8c57-d6c9b3c47842
### TESTING INSTRUCTIONS
Click "View Query" in the chart or explore and then check the toolbars above.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(explore): Add format sql and view in SQL Lab option in View Query [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #33341:
URL: https://github.com/apache/superset/pull/33341#issuecomment-3721633474
> I am testing this feature now. The slider for formatted vs. original is greyed out and nothing happens when I click on it. Is this intended behavior, maybe because my data warehouse is Microsoft SQL Server (MSSQL) and it lacks some features? As a user I am trying to understand this.
@sfirke I just saw your question now. To answer: the "format SQL" feature uses the formatting function provided by sqlglot. Currently, sqlglot does not have a dedicated dialect for MSSQL, so formatting is based on T-SQL. However, some syntax is not correctly recognized in this process, which causes parsing errors and may result in the SQL not being formatted/rendered properly.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(explore): Add format sql and view in SQL Lab option in View Query [superset]
Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #33341:
URL: https://github.com/apache/superset/pull/33341#issuecomment-3366764197
@justinpark I am testing this feature now. The slider for formatted vs. original is greyed out and nothing happens when I click on it. Is this intended behavior, maybe because my data warehouse is Microsoft SQL Server (MSSQL) and it lacks some features? As a user I am trying to understand this.
<img width="1283" height="377" alt="image" src="https://github.com/user-attachments/assets/ae96212c-31ac-46ae-8319-b04338e45da4" />
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(explore): Add format sql and view in SQL Lab option in View Query [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #33341:
URL: https://github.com/apache/superset/pull/33341#issuecomment-2848065254
Here is the updated design.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(explore): Add format sql and view in SQL Lab option in View Query [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #33341:
URL: https://github.com/apache/superset/pull/33341#discussion_r2073452741
##########
superset-frontend/src/explore/components/controls/ViewQuery.test.tsx:
##########
@@ -0,0 +1,158 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import {
+ screen,
+ render,
+ fireEvent,
+ waitFor,
+} from 'spec/helpers/testing-library';
+import fetchMock from 'fetch-mock';
+import copyTextToClipboard from 'src/utils/copy';
+import ViewQuery, { ViewQueryProps } from './ViewQuery';
+
+const mockHistoryPush = jest.fn();
+jest.mock('react-router-dom', () => ({
+ ...jest.requireActual('react-router-dom'),
+ useHistory: () => ({
+ push: mockHistoryPush,
+ }),
+}));
+
+jest.mock('src/utils/copy');
+
+function setup(props: ViewQueryProps) {
+ return render(<ViewQuery {...props} />, { useRouter: true, useRedux: true });
+}
+
+const mockProps = {
+ sql: 'select * from table',
+ datasource: '1__table',
+};
+
+const datasetApiEndpoint = 'glob:*/api/v1/dataset/1?**';
+const formatSqlEndpoint = 'glob:*/api/v1/sqllab/format_sql/';
+const formattedSQL = 'SELECT * FROM table;';
+
+beforeEach(() => {
+ fetchMock.get(datasetApiEndpoint, {
+ result: {
+ database: {
+ backend: 'sqlite',
+ },
+ },
+ });
+ fetchMock.post(formatSqlEndpoint, {
+ result: formattedSQL,
+ });
+});
+
+afterEach(() => {
+ jest.resetAllMocks();
+ fetchMock.restore();
+});
+
+const getFormatSwitch = () =>
+ screen.getByRole('switch', { name: 'Show original SQL' });
+
+it('renders the component with Formatted SQL and buttons', async () => {
Review Comment:
Can you use `test` instead of `it`?
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(explore): Add format sql and view in SQL Lab option in View Query [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark merged PR #33341:
URL: https://github.com/apache/superset/pull/33341
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(explore): Add format sql and view in SQL Lab option in View Query [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33341:
URL: https://github.com/apache/superset/pull/33341#discussion_r2070956796
##########
superset-frontend/src/explore/components/controls/ViewQuery.tsx:
##########
@@ -51,25 +53,92 @@ const StyledSyntaxContainer = styled.div`
flex-direction: column;
`;
+const StyledHeaderMenuContainer = styled.div`
+ display: flex;
+ flex-direction: row;
+ column-gap: ${({ theme }) => theme.gridUnit * 2}px;
+ margin-top: ${({ theme }) => -theme.gridUnit * 4}px;
+ align-items: baseline;
+`;
+
const StyledSyntaxHighlighter = styled(SyntaxHighlighter)`
flex: 1;
`;
const ViewQuery: FC<ViewQueryProps> = props => {
- const { sql, language = 'sql' } = props;
+ const { sql, language = 'sql', datasource } = props;
+ const datasetId = datasource.split('__')[0];
Review Comment:
### Unexplained Magic String Separator <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Magic string separator '__' is hardcoded without explanation of its significance in parsing the datasource identifier.
###### Why this matters
Future developers may not understand the format of datasource identifiers, making maintenance risky and debugging harder.
###### Suggested change ∙ *Feature Preview*
Extract the separator into a named constant with a descriptive name:
```typescript
const DATASOURCE_ID_SEPARATOR = '__';
const datasetId = datasource.split(DATASOURCE_ID_SEPARATOR)[0];
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fe9cd54-19ac-40da-bcd0-aa3443b507c5/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fe9cd54-19ac-40da-bcd0-aa3443b507c5?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fe9cd54-19ac-40da-bcd0-aa3443b507c5?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fe9cd54-19ac-40da-bcd0-aa3443b507c5?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fe9cd54-19ac-40da-bcd0-aa3443b507c5)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:949df9d7-c3e9-4e3c-a779-6d982bc2a357 -->
[](949df9d7-c3e9-4e3c-a779-6d982bc2a357)
##########
superset-frontend/src/explore/components/controls/ViewQuery.tsx:
##########
@@ -51,25 +53,92 @@ const StyledSyntaxContainer = styled.div`
flex-direction: column;
`;
+const StyledHeaderMenuContainer = styled.div`
+ display: flex;
+ flex-direction: row;
+ column-gap: ${({ theme }) => theme.gridUnit * 2}px;
+ margin-top: ${({ theme }) => -theme.gridUnit * 4}px;
+ align-items: baseline;
+`;
+
const StyledSyntaxHighlighter = styled(SyntaxHighlighter)`
flex: 1;
`;
const ViewQuery: FC<ViewQueryProps> = props => {
- const { sql, language = 'sql' } = props;
+ const { sql, language = 'sql', datasource } = props;
+ const datasetId = datasource.split('__')[0];
+ const [formattedSQL, setFormattedSQL] = useState<string>();
+ const [showFormatSQL, setShowFormatSQL] = useState(false);
+ const history = useHistory();
+ const currentSQL = (showFormatSQL ? formattedSQL : sql) ?? sql;
+
+ const formatCurrentQuery = useCallback(() => {
+ if (formattedSQL) {
+ setShowFormatSQL(val => !val);
+ } else {
+ const queryParams = rison.encode({
+ keys: ['none'],
+ columns: ['database.backend'],
+ });
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/${datasetId}?q=${queryParams}`,
+ }).then(({ json }) => {
+ SupersetClient.post({
+ endpoint: `/api/v1/sqllab/format_sql/`,
+ body: JSON.stringify({
+ sql,
+ engine: json.result.database.backend,
+ }),
+ headers: { 'Content-Type': 'application/json' },
+ }).then(({ json }) => {
+ setFormattedSQL(json.result);
+ setShowFormatSQL(true);
+ });
+ });
Review Comment:
### Missing Error Handling in API Calls <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The API calls lack error handling, which could lead to unhandled exceptions if the network requests fail or return unexpected responses.
###### Why this matters
Users won't receive any feedback if the SQL formatting fails, and the application might appear unresponsive.
###### Suggested change ∙ *Feature Preview*
Add error handling to the API calls:
```typescript
formatCurrentQuery = useCallback(() => {
if (formattedSQL) {
setShowFormatSQL(val => !val);
} else {
const queryParams = rison.encode({
keys: ['none'],
columns: ['database.backend'],
});
SupersetClient.get({
endpoint: `/api/v1/dataset/${datasetId}?q=${queryParams}`,
})
.then(({ json }) => {
return SupersetClient.post({
endpoint: '/api/v1/sqllab/format_sql/',
body: JSON.stringify({
sql,
engine: json.result.database.backend,
}),
headers: { 'Content-Type': 'application/json' },
});
})
.then(({ json }) => {
setFormattedSQL(json.result);
setShowFormatSQL(true);
})
.catch(error => {
// Handle the error appropriately, e.g., show a notification
console.error('Failed to format SQL:', error);
});
}
}, [sql, datasetId, formattedSQL]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/46e59f53-7820-400d-91ea-2754a37000a2/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/46e59f53-7820-400d-91ea-2754a37000a2?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/46e59f53-7820-400d-91ea-2754a37000a2?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/46e59f53-7820-400d-91ea-2754a37000a2?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/46e59f53-7820-400d-91ea-2754a37000a2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9886e005-bb5b-433d-83f2-debdb8ce548e -->
[](9886e005-bb5b-433d-83f2-debdb8ce548e)
##########
superset-frontend/src/explore/components/controls/ViewQuery.tsx:
##########
@@ -51,25 +53,92 @@ const StyledSyntaxContainer = styled.div`
flex-direction: column;
`;
+const StyledHeaderMenuContainer = styled.div`
+ display: flex;
+ flex-direction: row;
+ column-gap: ${({ theme }) => theme.gridUnit * 2}px;
+ margin-top: ${({ theme }) => -theme.gridUnit * 4}px;
+ align-items: baseline;
+`;
+
const StyledSyntaxHighlighter = styled(SyntaxHighlighter)`
flex: 1;
`;
const ViewQuery: FC<ViewQueryProps> = props => {
- const { sql, language = 'sql' } = props;
+ const { sql, language = 'sql', datasource } = props;
+ const datasetId = datasource.split('__')[0];
+ const [formattedSQL, setFormattedSQL] = useState<string>();
+ const [showFormatSQL, setShowFormatSQL] = useState(false);
+ const history = useHistory();
+ const currentSQL = (showFormatSQL ? formattedSQL : sql) ?? sql;
+
+ const formatCurrentQuery = useCallback(() => {
+ if (formattedSQL) {
+ setShowFormatSQL(val => !val);
+ } else {
+ const queryParams = rison.encode({
+ keys: ['none'],
+ columns: ['database.backend'],
+ });
Review Comment:
### Unexplained API Query Parameters <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Magic values 'none' and 'database.backend' are hardcoded without explanation of their purpose in the API query.
###### Why this matters
The meaning and necessity of these specific query parameters is unclear, making the code harder to maintain or modify correctly.
###### Suggested change ∙ *Feature Preview*
Extract these values into named constants with descriptive names:
```typescript
const DATASET_QUERY_PARAMS = {
KEYS: ['none'],
REQUIRED_COLUMNS: ['database.backend'],
};
const queryParams = rison.encode(DATASET_QUERY_PARAMS);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ff8b6658-d817-40eb-a6e8-acfb253f33d2/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ff8b6658-d817-40eb-a6e8-acfb253f33d2?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ff8b6658-d817-40eb-a6e8-acfb253f33d2?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ff8b6658-d817-40eb-a6e8-acfb253f33d2?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ff8b6658-d817-40eb-a6e8-acfb253f33d2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a61fa4a9-ef03-45c0-9190-371b8e594489 -->
[](a61fa4a9-ef03-45c0-9190-371b8e594489)
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(explore): Add format sql and view in SQL Lab option in View Query [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #33341:
URL: https://github.com/apache/superset/pull/33341#issuecomment-2847057372
@justinpark The `View in SQL Lab` button is a great addition!
About the "Format SQL" feature, I think a formatted SQL is better for users when visualizing a query. Given that https://github.com/apache/superset/pull/30350 states:
> When running a query we pass it through sqlglot to prettify the SQL, but sqlglot is doing some unwanted changes when the dialect is set to generic.
and the fact that the dialect will not be generic most of the time, I wonder if we should always format the SQL when visualizing the query. To be clear, I'm not proposing reverting @betodealmeida's changes but to always format the SQL when clicking on `View query`.
If the dialect is generic, then you could always show the non-formatted version of the query and warning saying that the SQL can't be formatted for that database type.
Pinging @betodealmeida, who probably has more context, to review too.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(explore): Add format sql and view in SQL Lab option in View Query [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #33341:
URL: https://github.com/apache/superset/pull/33341#issuecomment-2847790598
> and the fact that the dialect will not be generic most of the time, I wonder if we should always format the SQL when visualizing the query. To be clear, I'm not proposing reverting betodealmeida's changes but to always format the SQL when clicking on View query.
I like the idea too. Since the formatted SQL in this section doesn't modify the original SQL, it should be okay.
Additionally, I plan to display a "Show Original" button so that users can revert back to the original SQL if needed, allowing access to both versions.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org