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): add shortcut for run current sql #24329

Merged
merged 2 commits into from
Jul 11, 2023

Conversation

justinpark
Copy link
Member

SUMMARY

The manual selection of the current query is time-consuming. (Longer the query more time it takes to select manually)
This commit creates a new shortcut(ctrl + shift + enter) which automatically detects and selects the range of the current query and then run the selection in order to reduce the manual selection work.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After:

current.query.mov

Before:

before--current-sql.mov

TESTING INSTRUCTIONS

Hit "Ctrl+Shift+Enter" on sql editor

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

@michael-s-molina
Copy link
Member

This is awesome! I'll review it tomorrow.

@michael-s-molina
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2023

@michael-s-molina Container image not yet published for this PR. Please try again when build is complete.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2023

@michael-s-molina Ephemeral environment creation failed. Please check the Actions logs for details.

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #24329 (9cce503) into master (d041648) will decrease coverage by 0.15%.
The diff coverage is 25.24%.

❗ Current head 9cce503 differs from pull request most recent head ea9beb7. Consider uploading reports for the commit ea9beb7 to get more accurate results

@@            Coverage Diff             @@
##           master   #24329      +/-   ##
==========================================
- Coverage   69.09%   68.95%   -0.15%     
==========================================
  Files        1906     1907       +1     
  Lines       74108    74177      +69     
  Branches     8165     8190      +25     
==========================================
- Hits        51207    51148      -59     
- Misses      20778    20906     +128     
  Partials     2123     2123              
Flag Coverage Δ
hive ?
javascript 55.72% <15.90%> (-0.07%) ⬇️
mysql ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...plugins/legacy-plugin-chart-heatmap/src/Heatmap.js 0.00% <ø> (ø)
.../legacy-plugin-chart-heatmap/src/transformProps.js 0.00% <0.00%> (ø)
...gins/legacy-plugin-chart-world-map/src/WorldMap.js 0.00% <ø> (ø)
...egacy-plugin-chart-world-map/src/transformProps.js 0.00% <0.00%> (ø)
...rts/src/BigNumber/BigNumberTotal/transformProps.ts 0.00% <ø> (ø)
...BigNumber/BigNumberWithTrendline/transformProps.ts 48.57% <ø> (ø)
.../plugin-chart-echarts/src/Funnel/transformProps.ts 73.46% <ø> (ø)
...s/plugin-chart-echarts/src/Gauge/transformProps.ts 78.57% <ø> (ø)
...ins/plugin-chart-echarts/src/Pie/transformProps.ts 56.33% <ø> (ø)
...lugin-chart-echarts/src/Sunburst/transformProps.ts 0.00% <0.00%> (ø)
... and 20 more

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@apache apache deleted a comment from github-actions bot Jun 8, 2023
@michael-s-molina
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2023

@michael-s-molina Container image not yet published for this PR. Please try again when build is complete.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2023

@michael-s-molina Ephemeral environment creation failed. Please check the Actions logs for details.

@michael-s-molina
Copy link
Member

@justinpark Ephemeral environments are not working anymore.

@michael-s-molina
Copy link
Member

If the cursor is at the end of the line, it will jump to the next statement.

Screen.Recording.2023-06-09.at.14.44.03.mov

@michael-s-molina
Copy link
Member

Should we ignore comments?

Screen.Recording.2023-06-09.at.14.47.42.mov

@cploonker
Copy link

To me it should be any query delimited by semi-colon OR (start/end of file) around the cursor should be picked.

If the cursor is just after the semicolon, next query should be picked.

If I am editing a specific query my cursor is likely going to be inside that query,

@justinpark
Copy link
Member Author

If the cursor is at the end of the line, it will jump to the next statement.

Screen.Recording.2023-06-09.at.14.44.03.mov

As Chandraprakash mentioned, this is intended. If the cursor is the next to the semi column, it will jump to the next statement.

@justinpark
Copy link
Member Author

justinpark commented Jun 12, 2023

Should we ignore comments?

Screen.Recording.2023-06-09.at.14.47.42.mov

As we faced the regression of handling comments in FE, ignoring comments makes more complex and can be an issue in some edge cases. I'd rather make simple (including comments) to make features functional.

@cploonker
Copy link

Is it possible to send the delimited query for execution without actually selecting the query in the UI?
Can we NOT skip this step off selecting it in the UI?
I don't have a demo to play with, but I was thinking that the selection will actually move my cursor and if I need to further edit then I still need to use the mouse to go back to the location of edit.

If not i guess this can be a stepping stone to end goal some day.

@michael-s-molina
Copy link
Member

Is it possible to send the delimited query for execution without actually selecting the query in the UI?

I think this is a great suggestion from an usability perspective. It's quite common to edit the statement after a failed query. That's actually the default behavior of many database tools. Here's an example of DBeaver:

Screen.Recording.2023-06-14.at.08.44.56.mov

@cploonker
Copy link

We can try to pass an additional parameter into runQueryFromSqlEditor e.g. currentQuery=true with default value as false. And then move the calc to find the query inside that function.

https://github.com/apache/superset/blob/c1d882e9153624046a450257a6e19e36a1f96e9d/superset-frontend/src/SqlLab/actions/sqlLab.js#L390

@cploonker
Copy link

cploonker commented Jun 20, 2023

Can we please merge this change.
We can always iterate on how ctrl + shift + enter works.
For the most part the end user experience will remain the same

@michael-s-molina
Copy link
Member

Can we please merge this change.
We can always iterate on how ctrl + shift + enter works.
For the most part the end user experience will remain the same

The iterative approach makes sense given that the current behavior already offers a valuable feature.

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

@EugeneTorap
Copy link
Contributor

EugeneTorap commented Jul 7, 2023

@justinpark Thanks for this nice PR!
@michael-s-molina Can you rerun the cancelled CI test and merge the PR?

@justinpark
Copy link
Member Author

cc: @cploonker @michael-s-molina @EugeneTorap

As suggested, I updated the selection back to the current cursor position.

without.selection.mov

@cploonker
Copy link

cc: @cploonker @michael-s-molina @EugeneTorap

As suggested, I updated the selection back to the current cursor position.

without.selection.mov

ooohhh nice

@EugeneTorap
Copy link
Contributor

@justinpark Merge it?

@justinpark justinpark merged commit 1473d97 into apache:master Jul 11, 2023
26 checks passed
@michael-s-molina michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jul 12, 2023
john-bodley pushed a commit to airbnb/superset-fork that referenced this pull request Jul 19, 2023
Co-authored-by: Justin Park <justinpark@apache.org>
(cherry picked from commit 1473d97)
michael-s-molina pushed a commit that referenced this pull request Jul 26, 2023
Co-authored-by: Justin Park <justinpark@apache.org>
(cherry picked from commit 1473d97)
@villebro
Copy link
Member

This is awesome! I'll review it tomorrow.

Agreed, this has been long overdue, thanks for this work @justinpark ! ❤️

@mistercrunch mistercrunch added 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ 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
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/M v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants