feat: Added Rows Returned #13190

AAfghahi · 2021-02-17T22:06:13Z

SUMMARY

SQL Lab shows how many rows have been returned.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

These show if you have no Limit, if you have a Limit, or if your results are less than the max limit.

TEST PLAN

ADDITIONAL INFORMATION

Has associated issue: [sql_lab]Display Rows Returned / Clarify when LIMIT is reached #12436
[ X] Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API

eschutho · 2021-02-17T22:48:31Z

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

+  rowsReturned() {
+    return (
+      <div className="ReturnedRows">
+        {this.state.data && this.state.data.length >= 0 && (


you can use optional chaining here. Can the length ever be less than 0?

I think I could actually just remove the conditions, since a tighter version of it happens in the render.

eschutho · 2021-02-17T22:49:55Z

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

+      <div className="ReturnedRows">
+        {this.state.data && this.state.data.length >= 0 && (
+          <span>
+            {t(`%s rows returned`, this.props.query.results.data.length)}


it feels like this length should be referencing the same length as above?

Also, the query has a rows prop which may be easier than totaling.

I originally had this as a drawing from the query function, but when I did that I got an error that said that Query has no property called rows, which is what is being passed into props.query.

However, when I render with this.props.query.rows, it renders perfectly fine, and is responsive to how many lines the SQL result is. I changed it to this because I was tired of my terminal yelling at me.

eschutho

AAfghahi · 2021-02-19T20:55:22Z

This is what it would look like if you hit the max display rate:

Without:

codecov-io · 2021-02-19T21:08:56Z

Codecov Report

Merging #13190 (8dc1ffe) into master (892eef1) will decrease coverage by 4.12%.
The diff coverage is 77.03%.

@@            Coverage Diff             @@
##           master   #13190      +/-   ##
==========================================
- Coverage   77.05%   72.93%   -4.13%     
==========================================
  Files         897      597     -300     
  Lines       45710    21275   -24435     
  Branches     5497     5498       +1     
==========================================
- Hits        35224    15517   -19707     
+ Misses      10362     5631    -4731     
- Partials      124      127       +3

Flag	Coverage Δ
cypress	`57.60% <71.69%> (+0.01%)`	⬆️
hive	`?`
javascript	`62.76% <55.55%> (+0.26%)`	⬆️
mysql	`?`
postgres	`?`
presto	`?`
python	`?`
sqlite	`?`

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

Impacted Files	Coverage Δ
...erset-frontend/src/SqlLab/components/SouthPane.jsx	`82.05% <ø> (ø)`
...erset-frontend/src/SqlLab/components/SqlEditor.jsx	`55.86% <ø> (-0.15%)`	⬇️
...rset-frontend/src/components/ButtonGroup/index.tsx	`100.00% <ø> (ø)`
...et-frontend/src/components/ErrorBoundary/index.jsx	`95.45% <ø> (ø)`
...perset-frontend/src/components/FormLabel/index.tsx	`100.00% <ø> (ø)`
...-frontend/src/components/Select/Select.stories.tsx	`0.00% <0.00%> (ø)`
superset-frontend/src/components/Select/Select.tsx	`90.81% <ø> (ø)`
superset-frontend/src/components/Select/styles.tsx	`86.30% <ø> (ø)`
...et-frontend/src/components/Timer/Timer.stories.tsx	`0.00% <0.00%> (ø)`
superset-frontend/src/components/Timer/index.tsx	`95.83% <ø> (ø)`
... and 332 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 892eef1...ab89d5f. Read the comment docs.

eschutho

looking good! just a few small comments.

eschutho · 2021-02-19T21:09:58Z

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

+    }
+    return (
+      <div className="ReturnedRows">
+        {limitWarning}


a bit of a nit, but instead of making limitWarning a react component or null and using it as a boolean, it would be better to use displayLimitReached as the boolean in both cases and have limitWarning always be the icon.

It would read {displayLimitReached && limitWarning} for example and the reader would know that it only shows the limitWarning if the displayLimit is reached, rather than always showing the warning, which will be null if the display limit isn't reached. It's a small nuance, but will help.

eschutho · 2021-02-19T21:16:39Z

superset-frontend/src/SqlLab/main.less

+  }
+  .LimitMessage{
+    color: #8E94B0;
+  }


we're going clean out these less files soon. I'd suggest using the emotion styles instead.

@eschutho changed it to emotion, and also used your updated logic.

New Look (including Sophie's thoughts):

eschutho · 2021-02-23T02:09:31Z

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

@@ -481,6 +497,27 @@ export default class ResultSet extends React.PureComponent<
    return <div className="noControls" />;
  }

+  rowsReturned() {


nit, but I would rename this renderRowsReturned since it's a function, so thus the verb prefix. Also to be consistent with renderControls

eschutho · 2021-02-23T02:11:25Z

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

+      <ReturnedRows>
+        {query.results?.displayLimitReached && limitWarning}
+        <span>{t(`%s rows returned`, query.rows)}</span>
+        {query.results?.displayLimitReached && (


lets see if we can put this value into a variable since you're using it twice. You can also declare it in the destructuring on line 501.

eschutho · 2021-02-23T02:12:03Z

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

+              `It appears that the number of rows in the query results displayed
+           was limited on the server side to
+           the %s limit.`,
+              query.rows,


this is cool. I didn't know that you can do string interpolation in the translate function.

eschutho · 2021-02-23T02:14:07Z

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

+        {query.results?.displayLimitReached && limitWarning}
+        <span>{t(`%s rows returned`, query.rows)}</span>
+        {query.results?.displayLimitReached && (
+          <span className="LimitMessage">


we usually use camel-case (lowercase first letter) for class names.

eschutho · 2021-02-23T02:35:15Z

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

@@ -100,6 +100,22 @@ const MonospaceDiv = styled.div`
  white-space: pre-wrap;
 `;

+const ReturnedRows = styled.div`
+  margin-top: 48px;
+  margin-bottom: -32px;


there's some weirdness going on with the position: fixed styling of the controls, which is causing these weird margins. Let's see if can remove the position:fixed on the controls. I don't think they are supposed to scroll at all, so we may need to look for a container that has a scroll attribute on the overflow and remove it.

@eschutho been playing around with this a bit. I got rid of position fixed on the controls, and if do overflow:hidden on the overall container then it replicates the behavior that we want without there being negative margins (I kind of had a theory that you might not like negative margin :) )

What I am currently doing is looking at how to target the overall container.

ok, I think I fixed all of it with the last commit! This was a good dive into the various components in the South Pane.

Looks like:

yeah, that looks great! also, correct on the negative margins. :)

…o return-rows

eschutho · 2021-02-27T02:35:56Z

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

+  }
+  .limitMessage {
+    color: #8e94b0;
+    margin-left: 8px;


can we try to use the themeProvider here for the spacings and colors?

Done! I couldn't find the right orange color though, should I add it in?

Let's ask @Steejay if the existing orange works: FF7F44

@eschutho @AAfghahi existing works. nearly identical.

eschutho · 2021-02-27T02:37:56Z

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

@@ -481,6 +495,28 @@ export default class ResultSet extends React.PureComponent<
    return <div className="noControls" />;
  }

+  renderRowsReturned() {
+    const { results, rows } = this.props.query;


will this.props.query always have a value?

yup! It will only render if query.results exists.

eschutho · 2021-02-27T02:52:40Z

just so I don't forget, let's follow up on the display alert in this ticket.. does this PR include #10330 too?

AAfghahi · 2021-02-27T04:20:31Z

just so I don't forget, let's follow up on the display alert in this ticket.. does this PR include #10330 too?

Portions of it, the portions that are not included are the admin portions and the part that requires a migration.

eschutho

Code looks good! I'll let @yousoph or @Steejay do a visual review.

yousoph · 2021-03-03T04:40:39Z

Looked at this with @AAfghahi today, it was looking good to me 👍

pull-request-size bot added the size/S label Feb 17, 2021

AAfghahi changed the title ~~feature: Added Rows Returned~~ feat: Added Rows Returned Feb 17, 2021

AAfghahi changed the title ~~feat: Added Rows Returned~~ feat: Added Rows Returned Feb 17, 2021

AAfghahi force-pushed the return-rows branch from 0de21e7 to 26bc3ba Compare February 17, 2021 22:20

eschutho reviewed Feb 17, 2021

View reviewed changes

eschutho requested changes Feb 17, 2021

View reviewed changes

rows returned

4a35213

AAfghahi force-pushed the return-rows branch from 26bc3ba to 4a35213 Compare February 18, 2021 14:50

AAfghahi added 2 commits February 18, 2021 11:14

Merge branch 'master' into return-rows

bb3413a

preliminary changes

ba40293

pull-request-size bot added size/M and removed size/S labels Feb 19, 2021

return rows

3383694

AAfghahi force-pushed the return-rows branch from 80a7e7e to 3383694 Compare February 19, 2021 18:14

AAfghahi force-pushed the return-rows branch from e599a9c to 8c9c3e4 Compare February 19, 2021 20:56

added warning sign

f995d78

AAfghahi force-pushed the return-rows branch from 8c9c3e4 to f995d78 Compare February 19, 2021 21:00

eschutho requested changes Feb 19, 2021

View reviewed changes

AAfghahi force-pushed the return-rows branch 2 times, most recently from eb8a3cc to a9e293d Compare February 19, 2021 22:56

switched to emotion

4cc675d

AAfghahi force-pushed the return-rows branch from a9e293d to 4cc675d Compare February 19, 2021 22:59

eschutho reviewed Feb 23, 2021

View reviewed changes

AAfghahi added 9 commits February 23, 2021 11:04

rows returned

8327af0

preliminary changes

6767ec4

return rows

79b203b

added warning sign

4896bab

switched to emotion

f8405ab

margin work

3eaa8e4

Merge branch 'return-rows' of ssh://github.com/preset-io/superset int…

24ca014

…o return-rows

Merge branch 'master' into return-rows

cbd00f8

fixed margins

be5fdc6

eschutho reviewed Feb 27, 2021

View reviewed changes

AAfghahi added 2 commits February 26, 2021 23:21

made changes to theme

fb29187

Merge branch 'master' into return-rows

bd4114a

AAfghahi force-pushed the return-rows branch 2 times, most recently from ff73170 to e5ed831 Compare March 1, 2021 22:57

merged for Evan changes

ab89d5f

AAfghahi force-pushed the return-rows branch from e5ed831 to ab89d5f Compare March 1, 2021 23:17

eschutho approved these changes Mar 3, 2021

View reviewed changes

hughhhh self-requested a review March 3, 2021 15:17

hughhhh approved these changes Mar 3, 2021

View reviewed changes

hughhhh merged commit 9c9862f into apache:master Mar 3, 2021

hughhhh deleted the return-rows branch March 3, 2021 15:17

junlincc added the preset-io label Mar 18, 2021

allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021

feat: Added Rows Returned (apache#13190)

c70862b

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added Rows Returned #13190

feat: Added Rows Returned #13190

AAfghahi commented Feb 17, 2021

eschutho Feb 17, 2021

AAfghahi Feb 18, 2021

eschutho Feb 17, 2021

eschutho Feb 17, 2021

AAfghahi Feb 18, 2021

eschutho left a comment

AAfghahi commented Feb 19, 2021

codecov-io commented Feb 19, 2021 •

edited by codecov bot

Loading

eschutho left a comment

eschutho Feb 19, 2021

eschutho Feb 19, 2021

AAfghahi Feb 19, 2021 •

edited

Loading

eschutho Feb 23, 2021 •

edited

Loading

eschutho Feb 23, 2021

eschutho Feb 23, 2021

eschutho Feb 23, 2021

eschutho Feb 23, 2021

AAfghahi Feb 23, 2021

AAfghahi Feb 23, 2021

AAfghahi Feb 23, 2021

eschutho Feb 27, 2021

eschutho Feb 27, 2021

AAfghahi Feb 27, 2021

eschutho Mar 1, 2021

Steejay Mar 1, 2021

eschutho Feb 27, 2021

AAfghahi Feb 27, 2021

eschutho commented Feb 27, 2021

AAfghahi commented Feb 27, 2021

eschutho left a comment

yousoph commented Mar 3, 2021

feat: Added Rows Returned #13190

feat: Added Rows Returned #13190

Conversation

AAfghahi commented Feb 17, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

AAfghahi commented Feb 19, 2021

codecov-io commented Feb 19, 2021 • edited by codecov bot Loading

Codecov Report

eschutho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AAfghahi Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

eschutho Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho commented Feb 27, 2021

AAfghahi commented Feb 27, 2021

eschutho left a comment

Choose a reason for hiding this comment

yousoph commented Mar 3, 2021

codecov-io commented Feb 19, 2021 •

edited by codecov bot

Loading

AAfghahi Feb 19, 2021 •

edited

Loading

eschutho Feb 23, 2021 •

edited

Loading