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.

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): Format sql #25344

Merged
merged 9 commits into from
Nov 3, 2023
Merged

Conversation

justinpark
Copy link
Member

@justinpark justinpark commented Sep 19, 2023

SUMMARY

This commit adds "Format SQL" feature in sql editor which helps to format sql code in the sql editor.
This commit also adds a new shortcut (ctrl + shift + f) for this feature.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

after--format-sql.mov

TESTING INSTRUCTIONS

Go to SQL Lab and type a long-run sql
Hit Ctrl + Shift + f or click "Format SQL" in the action dropdown
Check the formatted sql

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

@@ -144,8 +144,10 @@ const AceEditorWrapper = ({
};

const onChangeText = (text: string) => {
setSql(text);
onChange(text);
if (text !== sql) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This change prevents the overwrite the sql by setQueryEditorAndSaveSqlWithDebounce

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @justinpark! I left some first-pass comments.

superset/sqllab/api.py Outdated Show resolved Hide resolved
superset-frontend/src/SqlLab/actions/sqlLab.js Outdated Show resolved Hide resolved
superset/sqllab/api.py Outdated Show resolved Hide resolved
@michael-s-molina
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@michael-s-molina Ephemeral environment spinning up at http://34.220.51.49:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@justinpark justinpark force-pushed the feat--sqllab-format-sql branch 3 times, most recently from a61a3e0 to 881f020 Compare October 12, 2023 15:18
@justinpark
Copy link
Member Author

@michael-s-molina could you stamp this PR if the updates looks okay?

@justinpark
Copy link
Member Author

@michael-s-molina I also set up SQLLAB_FORMAT_SQL_OPTIONS in config to setup custom SQL format option.

@justinpark
Copy link
Member Author

I also set up SQLLAB_FORMAT_SQL_OPTIONS in config to setup custom SQL format option.

Revert this config option after offline discussion

@michael-s-molina
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@michael-s-molina Ephemeral environment spinning up at http://18.237.171.147:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Comment on lines +396 to +398
func: () => {
formatCurrentQuery();
},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func: () => {
formatCurrentQuery();
},
func: formatCurrentQuery,

action=lambda self, *args, **kwargs: f"{self.__class__.__name__}" f".format",
log_to_statsd=False,
)
def format(self) -> FlaskResponse:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename the endpoint to format_sql?

export function formatQuery(queryEditor) {
return function (dispatch, getState) {
const { sql } = getUpToDateQuery(getState(), queryEditor);
return SupersetClient.post({
Copy link
Member

Choose a reason for hiding this comment

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

why couldn't we just us a npm package to do this work vs. creating a whole new endpoint?

Copy link
Member Author

@justinpark justinpark Oct 31, 2023

Choose a reason for hiding this comment

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

why couldn't we just us a npm package to do this work vs. creating a whole new endpoint?

@hughhhh We chose server-side formatting over client-side formatting for several reasons:

  1. It reduces the need to maintain additional NPM dependencies.
  2. It reduces the package size.
  3. It ensures that SQL formatting is consistent across surfaces in "view query", using sqlparse.

If you and other community members believe that an NPM package solution is better, I am happy to make changes.

@betodealmeida @eschutho @michael-s-molina do you have an opinion on the frontend approach?

Copy link
Member

Choose a reason for hiding this comment

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

There seems to be a number of pros with having this on the backend including: custom formatting, dialect specific formatting, and consistency throughout the application.

@villebro has experience working with frontend SQL formatting and ran into a number of issues.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM

try:
model = self.format_model_schema.load(request.json)
result = sqlparse.format(
model.get("sql", ""), reindent=True, keyword_case="upper"
Copy link
Member

@john-bodley john-bodley Nov 2, 2023

Choose a reason for hiding this comment

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

If the sql field is required (per the schema) is model["sql"] suffice?

@justinpark justinpark merged commit 24a2213 into apache:master Nov 3, 2023
29 checks passed
Copy link
Contributor

github-actions bot commented Nov 3, 2023

Ephemeral environment shutdown and build artifacts deleted.

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants