feat(sqllab): Show sql in the current result #24787

justinpark · 2023-07-24T22:54:25Z

SUMMARY

Following #24329, SQLLab can run a portion of the sql editor without highlighted.
Like other 3rd party sql editor, it will be useful to display the current running sql in the result panel to identify the actual executed query.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After:

after--show-sql.mov

Before:

before--show-sql.mov

TESTING INSTRUCTIONS

Go to sqllab
Run a query and check the result panel

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

codecov · 2023-07-25T00:04:33Z

Codecov Report

Merging #24787 (f10ed73) into master (abb8e28) will increase coverage by 1.72%.
Report is 55 commits behind head on master.
The diff coverage is 76.77%.

❗ Current head f10ed73 differs from pull request most recent head 4a5ee98. Consider uploading reports for the commit 4a5ee98 to get more accurate results

@@            Coverage Diff             @@
##           master   #24787      +/-   ##
==========================================
+ Coverage   67.23%   68.95%   +1.72%     
==========================================
  Files        1902     1906       +4     
  Lines       73939    74126     +187     
  Branches     8176     8212      +36     
==========================================
+ Hits        49713    51117    +1404     
+ Misses      22113    20885    -1228     
- Partials     2113     2124      +11

Flag	Coverage Δ
javascript	`55.83% <72.86%> (+0.08%)`	⬆️
postgres	`?`

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

Files Changed	Coverage Δ
...d/packages/superset-ui-chart-controls/src/types.ts	`100.00% <ø> (ø)`
...s/legacy-plugin-chart-country-map/src/countries.ts	`100.00% <ø> (ø)`
...s/legacy-plugin-chart-heatmap/src/controlPanel.tsx	`57.14% <ø> (ø)`
.../legacy-plugin-chart-heatmap/src/transformProps.js	`0.00% <ø> (ø)`
.../legacy-plugin-chart-world-map/src/controlPanel.ts	`25.00% <ø> (ø)`
...egacy-plugin-chart-world-map/src/transformProps.js	`0.00% <ø> (ø)`
...et-chart-deckgl/src/layers/Geojson/controlPanel.ts	`50.00% <ø> (ø)`
...egacy-preset-chart-deckgl/src/layers/Path/Path.jsx	`0.00% <ø> (ø)`
...reset-chart-deckgl/src/layers/Path/controlPanel.ts	`50.00% <ø> (ø)`
...preset-chart-deckgl/src/layers/Polygon/Polygon.jsx	`0.00% <ø> (ø)`
... and 220 more

... and 82 files with indirect coverage changes

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

michael-s-molina · 2023-07-25T13:20:16Z

I'll leave the design review for @kasiazjc but I think we can truncate the query (add ...) to indicate that we're only showing part of the query.

kasiazjc · 2023-07-25T14:47:57Z

I'll leave the design review for @kasiazjc but I think we can truncate the query (add ...) to indicate that we're only showing part of the query.

Thanks @michael-s-molina and thanks @justinpark for the PR!

I have a few suggestions:

in other places in the app we actually have a number of rows or the right side of the table and filters/title etc on the left side; I think it would make sense to switch the order in here too, because the sql line acts as a title. Below a screenshot for reference (from explore)

The sql field could have a max width and if the text (first line) doesn't fill that space, the sql input could shrink a little bit
if there is another line, more text that is longer than the max width of the container (as Michael said) there should be a truncation rule with "..." - this is a default truncation behavior we use across the app
and the last one - this is more of a question - I'm wondering if instead of showing a pop up with the sql query, would it be more useful to scroll and highlight the query in the sql query area at the top? Not sure if that would be even possible, but curious to hear your thoughts @michael-s-molina and @justinpark

michael-s-molina · 2023-07-25T15:06:27Z

in other places in the app we actually have a number of rows or the right side of the table and filters/title etc on the left side; I think it would make sense to switch the order in here too, because the sql line acts as a title. Below a screenshot for reference (from explore)

Wow, that's a great suggestion. @justinpark Could we also display the number of rows using the Label (pill) component?

and the last one - this is more of a question - I'm wondering if instead of showing a pop up with the sql query, would it be more useful to scroll and highlight the query in the sql query area at the top? Not sure if that would be even possible, but curious to hear your thoughts @michael-s-molina and @justinpark

@justinpark If this is possible it would be an awesome UX!

justinpark · 2023-07-26T19:21:35Z

and the last one - this is more of a question - I'm wondering if instead of showing a pop up with the sql query, would it be more useful to scroll and highlight the query in the sql query area at the top?

@kasiazjc this is a good idea but it might be broken if a user modify the sql content which is not same from the ran query.

justinpark · 2023-07-26T19:36:55Z

Could we also display the number of rows using the Label (pill) component?

Sure. I'll update accordingly

kasiazjc · 2023-07-27T14:53:59Z

and the last one - this is more of a question - I'm wondering if instead of showing a pop up with the sql query, would it be more useful to scroll and highlight the query in the sql query area at the top?

@kasiazjc this is a good idea but it might be broken if a user modify the sql content which is not same from the ran query.

True, so I think for now let's keep the modal. Thanks!

justinpark · 2023-08-04T21:22:42Z

I have a few suggestions:

in other places in the app we actually have a number of rows or the right side of the table and filters/title etc on the left side; I think it would make sense to switch the order in here too, because the sql line acts as a title. Below a screenshot for reference (from explore)
image
The sql field could have a max width and if the text (first line) doesn't fill that space, the sql input could shrink a little bit

if there is another line, more text that is longer than the max width of the container (as Michael said) there should be a truncation rule with "..." - this is a default truncation behavior we use across the app

@kasiazjc I updated the layout as you suggested. I also replaces the existing limit message by the tooltip (+ exclamation indicator when exists). Feel free to give any feedback at your convenience.

update--current-sql.mov

justinpark · 2023-08-09T19:56:29Z

@kasiazjc circle back related to the update

justinpark · 2023-08-16T20:48:12Z

@kasiazjc @michael-s-molina could you help a review again?

michael-s-molina · 2023-08-16T20:49:46Z

/testenv up

github-actions · 2023-08-16T20:51:53Z

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

michael-s-molina · 2023-08-16T20:52:46Z

superset-frontend/src/SqlLab/components/ResultSet/index.tsx

+          >
+            <Label
+              css={css`
+                line-height: 17px;


Could you use theme object to calculate the line-height (16 is fine), font-size and margin-right?

@michael-s-molina updated by theme variables.

michael-s-molina

LGTM

justinpark · 2023-09-01T18:53:20Z

@kasiazjc I will remain open until next Tuesday and will merge it if you don't have any concerns.

github-actions · 2023-09-06T16:49:13Z

Ephemeral environment shutdown and build artifacts deleted.

* fix: Issue apache#24493; Resolved report selection menu in chart and dashboard page (apache#25157) * fix: DML failures in SQL Lab (apache#25190) * fix: All values being selected in Select (apache#25202) * docs: fix wrong type in PREFERRED_DATABASES example (apache#25200) Signed-off-by: cmontemuino <1761056+cmontemuino@users.noreply.github.com> * docs: add CVEs for 2.1.1 (apache#25206) * chore: back port 2.1.1 doc changes (apache#25165) * feat(sqllab): Show sql in the current result (apache#24787) * docs(FAQ): add answer re: necessary specs, copy-edit existing answer (apache#24992) * fix: `is_select` (apache#25189) * fix: Cypress test to force mouseover (apache#25209) * fix(sqllab): Force trino client async execution (apache#24859) * fix: granularity_sqla and GENERIC_CHART_AXES (apache#25213) * chore: Convert deckgl class components to functional (apache#25177) * fix: Cypress test to force mouseover (follow-up) (apache#25223) * fix(docs): Fixing a typo in README.md (apache#25216) * chore(read_csv): remove deprecated argument (apache#25226) * chore(trino): remove unnecessary index checks (apache#25211) --------- Signed-off-by: cmontemuino <1761056+cmontemuino@users.noreply.github.com> Co-authored-by: Sandeep Patel <33354423+suicide11@users.noreply.github.com> Co-authored-by: Hugh A. Miles II <hughmil3s@gmail.com> Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> Co-authored-by: Carlos M <1761056+cmontemuino@users.noreply.github.com> Co-authored-by: Daniel Vaz Gaspar <danielvazgaspar@gmail.com> Co-authored-by: Elizabeth Thompson <eschutho@gmail.com> Co-authored-by: JUST.in DO IT <justin.park@airbnb.com> Co-authored-by: Sam Firke <sfirke@users.noreply.github.com> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: Rob Moore <giftig@users.noreply.github.com> Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com> Co-authored-by: yousoph <sophieyou12@gmail.com> Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

jinghua-qa · 2023-09-13T22:41:02Z

I filed this cosmetic issue for the yellow banner of the query limit, it looks like the yellow banner is now on the edge of the query filed.
#25288

This reverts commit 2d4de51.

pull-request-size bot added the size/M label Jul 24, 2023

justinpark requested review from michael-s-molina, betodealmeida, john-bodley and kasiazjc July 24, 2023 22:54

john-bodley changed the title ~~chore(sqllab): Show sql in the current result~~ feat(sqllab): Show sql in the current result Jul 25, 2023

michael-s-molina reviewed Aug 16, 2023

View reviewed changes

justinpark added 4 commits August 24, 2023 11:26

chore(sqllab): Show sql in the current result

5a2ca09

layout change for row count and sql block

dcf48e3

remove extra margin on return row

f4a3913

constantize font-size and spacing

6a174e6

justinpark force-pushed the chore--add-current-execute-sql branch from 4a5ee98 to 6a174e6 Compare August 24, 2023 18:48

michael-s-molina approved these changes Aug 31, 2023

View reviewed changes

justinpark merged commit 2d4de51 into apache:master Sep 6, 2023
26 checks passed

darwinsubramaniam pushed a commit to darwinsubramaniam/superset that referenced this pull request Sep 7, 2023

feat(sqllab): Show sql in the current result (apache#24787)

dc1ce7b

justinpark mentioned this pull request Sep 14, 2023

fix(sqllab): misplaced limit warning alert #25306

Merged

9 tasks

jinghua-qa added a commit to preset-io/superset that referenced this pull request Sep 18, 2023

Revert "feat(sqllab): Show sql in the current result (apache#24787)"

2ddaf91

This reverts commit 2d4de51.

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

feat(sqllab): Show sql in the current result (apache#24787)

5de0829

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sqllab): Show sql in the current result #24787

feat(sqllab): Show sql in the current result #24787

justinpark commented Jul 24, 2023

codecov bot commented Jul 25, 2023 •

edited

Loading

michael-s-molina commented Jul 25, 2023

kasiazjc commented Jul 25, 2023 •

edited

Loading

michael-s-molina commented Jul 25, 2023

justinpark commented Jul 26, 2023

justinpark commented Jul 26, 2023

kasiazjc commented Jul 27, 2023

justinpark commented Aug 4, 2023 •

edited

Loading

justinpark commented Aug 9, 2023

justinpark commented Aug 16, 2023

michael-s-molina commented Aug 16, 2023

github-actions bot commented Aug 16, 2023

michael-s-molina Aug 16, 2023

justinpark Aug 28, 2023

michael-s-molina left a comment

justinpark commented Sep 1, 2023

github-actions bot commented Sep 6, 2023

jinghua-qa commented Sep 13, 2023

feat(sqllab): Show sql in the current result #24787

feat(sqllab): Show sql in the current result #24787

Conversation

justinpark commented Jul 24, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 25, 2023 • edited Loading

Codecov Report

michael-s-molina commented Jul 25, 2023

kasiazjc commented Jul 25, 2023 • edited Loading

michael-s-molina commented Jul 25, 2023

justinpark commented Jul 26, 2023

justinpark commented Jul 26, 2023

kasiazjc commented Jul 27, 2023

justinpark commented Aug 4, 2023 • edited Loading

justinpark commented Aug 9, 2023

justinpark commented Aug 16, 2023

michael-s-molina commented Aug 16, 2023

github-actions bot commented Aug 16, 2023

michael-s-molina Aug 16, 2023

Choose a reason for hiding this comment

justinpark Aug 28, 2023

Choose a reason for hiding this comment

michael-s-molina left a comment

Choose a reason for hiding this comment

justinpark commented Sep 1, 2023

github-actions bot commented Sep 6, 2023

jinghua-qa commented Sep 13, 2023

codecov bot commented Jul 25, 2023 •

edited

Loading

kasiazjc commented Jul 25, 2023 •

edited

Loading

justinpark commented Aug 4, 2023 •

edited

Loading