Add separate limit setting for SqlLab #4941

jeffreythewang · 2018-05-07T17:01:14Z

Summary

Add functionality to wrap SQL Lab queries with a limit

Description

A common problem people have is that they input a query in SqlLab to preview their data before hitting visualize to create a table, and then it takes forever to run. This can be a pain (on them and potentially on the database).

Currently one workaround is to manually enter a limit in the query editor, visualizing (which creates the datasource), and then later removing it from the base query (by editing the created datasource). With this in mind, an additional/alternative solution might be to allow users to edit the sub-query in the visualize modal before creating their datasource.

So here's a way to have a limit only in the context of SQL editor. The limit is only applied as a wrapper for select queries, and not saved in any persistent database.

Examples

Related PR

#4834 - I like this idea, but not sure if every database supports prefetching. If going with this route, I'd have a configurable page size.

Related Issues

#4588

mistercrunch · 2018-05-07T17:30:21Z

I don't have time to really review now but wanted to make sure you've taken this https://github.com/apache/incubator-superset/blob/master/superset/assets/src/SqlLab/components/SqlEditor.jsx#L195 into consideration

jeffreythewang · 2018-05-07T17:37:06Z

Yeah I have. I think that should still show up as it indicates whether the limit was hit. It should retain the current behavior of not showing up if the number of rows returned is less than the limit applied. (or are you referring to some other behavior?)

codecov-io · 2018-05-07T17:38:26Z

Codecov Report

Merging #4941 into master will decrease coverage by 0.01%.
The diff coverage is 69.23%.

@@            Coverage Diff            @@
##           master   #4941      +/-   ##
=========================================
- Coverage   77.01%     77%   -0.02%     
=========================================
  Files          64      64              
  Lines        9508    9516       +8     
=========================================
+ Hits         7323    7328       +5     
- Misses       2185    2188       +3

Impacted Files	Coverage Δ
superset/views/base.py	`67.44% <ø> (ø)`	⬆️
superset/config.py	`92.36% <100%> (ø)`	⬆️
superset/views/core.py	`73.74% <57.14%> (-0.04%)`	⬇️
superset/sql_lab.py	`71.34% <80%> (-0.26%)`	⬇️

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 69e8df4...9b886af. Read the comment docs.

mistercrunch · 2018-06-04T16:34:41Z

Sorry about the delay. Let's get this through. Mind rebasing?

jeffreythewang · 2018-06-05T20:46:50Z

@mistercrunch There are a few things to consider given the current changes in master.

Since now you are fetching the limit if it exists in the query, should the query's written limit override the one specified in the LimitControl field? Or should it just add wrap on top of it? In the current implementation (of this un-rebased branch), the limit is applied as a wrap, and I think that makes more sense, as it separates the underlying query component from the limiting component, and it is clear that the limiting only happens in the SQL Lab context. But in the to-be-rebased version it seems like I have to choose between the SQL editor limit (that is fetched with regex) vs. the LimitControl field's limit.

For example, if I have a query like:

SELECT * FROM birth_names LIMIT 1000

and the LimitControl field has a value of 100, the current version of the code (in my un-rebased branch) would generate:

SELECT * FROM (SELECT * FROM birth_names LIMIT 1000) as subquery LIMIT 100

whereas in the to-be-rebased version, I have to apply either 1000 or 100, unless I change some functionality in db_engine_specs, or have database-specific configurations for wrapping vs forcing a limit.

This can lead to even more confusion if we have a query with multiple limits, for example:

SELECT * FROM (SELECT * FROM birth_names WHERE gender LIKE 'boy' LIMIT 10)
UNION ALL
SELECT * FROM birth_names WHERE gender LIKE 'girl' LIMIT 5

Does the user know that only the 5 will be fetched with regex? This should return 15 rows but instead it only returns 5 because of how the limit is applied. And then what if I apply a LimitControl limit on top of that?

(As an aside, for the query:

SELECT * FROM (SELECT * FROM birth_names WHERE gender LIKE 'boy' LIMIT 10)
UNION ALL
SELECT * FROM (SELECT * FROM birth_names WHERE gender LIKE 'girl' LIMIT 5)

Neither of these limits are fetched with the regex.)

I think whichever route we do end up going with, the LimitControl field's limit should be applied as a wrap.

Let me know if I'm misunderstanding how implementation of limiting (in current master) works, or if there is a better way to resolve this conflict that makes more sense.

timifasubaa · 2018-10-15T17:48:31Z

@jeffreythewang a few recommendations for us get this through.

Let's set the UI limit_query to a default of 10K and make this configurable on the backend (and the value gets propagated to the frontend).
Let's send the SQL_MAX_ROW from backend to the frontend and do some validation to prevent the user from exceeding that value in the UI.
Let's change the logic for overriding the limit to compare the value passed from the frontend and the existing limit specified in the query (instead of SQL_MAX_ROW). (https://github.com/apache/incubator-superset/blob/dd9eeda03e078ad053ddc2eb7170b7e281047a49/superset/sql_lab.py#L155)

With the three changes here, it should work. I'm happy to help out with any of the parts.

jeffreythewang

If we're going with having the UI box be the central point of all limit information for the user, then #5774 can be abandoned, and code related to that can be removed.

jeffreythewang · 2018-10-17T21:02:09Z

superset/views/core.py

        query = Query(
            database_id=int(database_id),
-            limit=mydb.db_engine_spec.get_limit_from_sql(sql),
+            limit=min(lim for lim in limits if lim is not None),


I built this with the assumption that we are going to do limiting via query replacement within the context of SQL Lab, so only the smaller limit is chosen.

timifasubaa · 2018-10-18T02:54:03Z

superset/assets/spec/javascripts/sqllab/fixtures.js

@@ -366,6 +366,8 @@ export const initialState = {
    workspaceQueries: [],
    queriesLastUpdate: 0,
    activeSouthPaneTab: 'Results',
+    defaultQueryLimit: 100,
+    maxRow: 10000,


Can we make these defaults get passed from the backend's config.py to the frontend?

These defaults are solely for unit tests.

timifasubaa · 2018-10-18T02:55:15Z

superset/assets/src/SqlLab/components/LimitControl.jsx

+          style={{ cursor: 'pointer' }}
+          onClick={this.handleToggle.bind(this)}
+        >
+          LIMIT {this.props.value || this.props.maxRow || '∞'}


Why is infinity still a possibility?

timifasubaa · 2018-10-18T03:00:29Z

superset/config.py

@@ -282,6 +282,9 @@
 # in the results backend. This also becomes the limit when exporting CSVs
 SQL_MAX_ROW = 100000

+# Default row limit for SQL Lab queries
+DEFAULT_SQLLAB_LIMIT = 10000
+
 # Limit to be returned to the frontend.
 DISPLAY_MAX_ROW = 1000


Can we change this to 10K or make it equal to DEFAULT_SQLLAB_LIMIT?

timifasubaa · 2018-10-22T18:45:53Z

LGTM, thanks for working on this Jeff. I've asked @graceguo-supercat to take a second look in case I missed anything.

graceguo-supercat · 2018-10-23T19:13:29Z

superset/assets/spec/javascripts/sqllab/LimitControl_spec.jsx

+    const value = 100;
+    wrapper = shallow(factory({ ...defaultProps, value }));
+    wrapper.find(Label).first().simulate('click');
+    setTimeout(() => {


setTimeout may cause flaky when running in Travis CI, do you really need it? also I saw in the ListControl you have Overlay, Popover, but you test on Tooltip component?

I replaced Tooltip assertions with assertions on validationErrors instead.

jeffreythewang · 2018-10-24T02:57:36Z

Note that this is not ready to be merged until we find a way to load in the default values without clearing the existing state of the redux store.

graceguo-supercat · 2018-10-24T17:51:24Z

hi @jeffreythewang Superset use redux-localstorage store sql lab user queries. Currently we sync sql_lab Redux's complete store state with localStorage. check persistState method, it allow you only sync with some parts.

It makes sense that sql_lab configuration should be set from server-side (by passing through bootstrap data), and only user data(mostly queries) are stored in localStorage.

graceguo-supercat · 2018-10-25T17:50:50Z

hi @jeffreythewang I talked this feature with @kristw. He has a nice suggestion: can we allow query limit for each different tab?

if there is no settings for a tab, use default value from server-side config.
if user changed/updated the limit for an existed tab, we sync the limit into localStorage and it will be loaded for the next time.

jeffreythewang · 2018-10-25T18:44:18Z

@graceguo-supercat Yup that is how it currently works in my implementation. Each tab has it's own queryLimit, saved into localStorage.

kristw · 2018-10-25T18:37:29Z

superset/assets/src/SqlLab/components/LimitControl.jsx

+  }
+
+  setValueAndClose(val) {
+    this.setState({ textValue: val }, this.submitAndClose.bind(this));


do the binding in constructor so it won't create new function every time this gets called.

kristw · 2018-10-25T18:38:35Z

superset/assets/src/SqlLab/components/LimitControl.jsx

+
+  isValidLimit(limit) {
+    const value =  parseInt(limit, 10);
+    return !(isNaN(value) || value <= 0 || (this.props.maxRow && value > this.props.maxRow));


Use Number.isNaN instead of isNaN

kristw · 2018-10-25T18:40:15Z

superset/assets/src/SqlLab/components/LimitControl.jsx

+              bsStyle="primary"
+              className="float-left ok"
+              disabled={!isValid}
+              onClick={this.submitAndClose.bind(this)}


bind in constructor once.

kristw · 2018-10-25T18:40:41Z

superset/assets/src/SqlLab/components/LimitControl.jsx

+        <Overlay
+          rootClose
+          show={this.state.showOverlay}
+          onHide={this.handleHide.bind(this)}


bind in constructor.

kristw · 2018-10-25T18:40:49Z

superset/assets/src/SqlLab/components/LimitControl.jsx

+      <div>
+        <Label
+          style={{ cursor: 'pointer' }}
+          onClick={this.handleToggle.bind(this)}


bind in constructor

kristw · 2018-10-25T18:43:14Z

superset/assets/src/SqlLab/components/LimitControl.jsx

+    const errorMsg = 'Row limit must be positive integer' +
+      (this.props.maxRow ? ` and below ${this.props.maxRow}` : '');
+    return (
+      <Popover id="sqllab-limit-results">


Is the id necessary?

This is primarily for getting rid of the warning. Do we want to remove it?

kristw · 2018-10-25T18:46:33Z

superset/assets/src/SqlLab/components/LimitControl.jsx

+    const textValue = this.state.textValue;
+    const isValid = this.isValidLimit(textValue);
+    const errorMsg = 'Row limit must be positive integer' +
+      (this.props.maxRow ? ` and below ${this.props.maxRow}` : '');


and below => not greater than

condition is value <= maxRow ?

jeffreythewang · 2018-10-29T18:49:27Z

I've updated this with a working implementation for updating localStorage.

How I tested:

Check out master and run SQL Lab to ensure the existing state is properly loaded.
Check out this branch and ensure that defaultQueryLimit and maxRow are loaded.
Update backend config values DEFAULT_SQLLAB_LIMIT, and SQL_MAX_ROW and ensure that they are properly updated on the frontend.

@graceguo-supercat I initially tried using persistState on creation of the store, but this only seemed to work for updating existing keys already loaded into the store (though this may have been affected by the structure of the object).

Use separate param for wrap sql Get query limit from config unit tests for limit control rendering in sql editor py unit test pg tests Add max rows limit Remove concept of infinity, always require defined limits consistency Assert on validation errors instead of tooltip fix unit tests attempt persist state pr comments and linting

graceguo-supercat · 2018-11-07T21:12:20Z

superset/config.py

-# Limit to be returned to the frontend.
-DISPLAY_MAX_ROW = 1000
+# Default row limit for SQL Lab queries
+DEFAULT_SQLLAB_LIMIT = 10000


@mistercrunch Sql Lab used to only render 1000 rows for each query, but fetch a much bigger number of rows of results. After this feature, sql lab will fetch and render same number of results.
Do you have any suggestion on this default query limit number?

graceguo-supercat

LGTM!

* Add separate limit setting for SqlLab Use separate param for wrap sql Get query limit from config unit tests for limit control rendering in sql editor py unit test pg tests Add max rows limit Remove concept of infinity, always require defined limits consistency Assert on validation errors instead of tooltip fix unit tests attempt persist state pr comments and linting * load configs in via common param * default to 1k

timifasubaa mentioned this pull request Oct 15, 2018

[sqllab] How can we make large Superset queries load faster? #4588

Closed

jeffreythewang force-pushed the jeffreyw/sqllab-limit branch from 7945995 to a49771e Compare October 17, 2018 19:55

jeffreythewang commented Oct 17, 2018

View reviewed changes

jeffreythewang force-pushed the jeffreyw/sqllab-limit branch from 73745cd to a49771e Compare October 18, 2018 14:07

timifasubaa reviewed Oct 18, 2018

View reviewed changes

timifasubaa approved these changes Oct 22, 2018

View reviewed changes

graceguo-supercat reviewed Oct 23, 2018

View reviewed changes

jeffreythewang force-pushed the jeffreyw/sqllab-limit branch from 6b57ccc to c955053 Compare October 23, 2018 23:46

kristw reviewed Oct 25, 2018

View reviewed changes

graceguo-supercat mentioned this pull request Nov 2, 2018

Add sql lab localStorage config #6257

Merged

jeffreythewang added 2 commits November 7, 2018 12:03

load configs in via common param

dd66a8e

jeffreythewang force-pushed the jeffreyw/sqllab-limit branch from 8e90fa7 to dd66a8e Compare November 7, 2018 19:31

graceguo-supercat reviewed Nov 7, 2018

View reviewed changes

default to 1k

9b886af

graceguo-supercat approved these changes Nov 7, 2018

View reviewed changes

graceguo-supercat merged commit 0584e36 into apache:master Nov 7, 2018

jeffreythewang mentioned this pull request Nov 15, 2018

[sqllab][bugfix] Set default limit on creation of new QE and on run #6400

Merged

mistercrunch mentioned this pull request Feb 26, 2019

[SQL Lab] Removing display limit #6942

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add separate limit setting for SqlLab #4941

Add separate limit setting for SqlLab #4941

jeffreythewang commented May 7, 2018

mistercrunch commented May 7, 2018

jeffreythewang commented May 7, 2018 •

edited

Loading

codecov-io commented May 7, 2018 •

edited

Loading

mistercrunch commented Jun 4, 2018

jeffreythewang commented Jun 5, 2018 •

edited

Loading

timifasubaa commented Oct 15, 2018 •

edited

Loading

jeffreythewang left a comment

jeffreythewang Oct 17, 2018

timifasubaa Oct 18, 2018

jeffreythewang Oct 18, 2018

timifasubaa Oct 18, 2018

timifasubaa Oct 18, 2018

timifasubaa commented Oct 22, 2018

graceguo-supercat Oct 23, 2018 •

edited

Loading

jeffreythewang Oct 23, 2018 •

edited

Loading

jeffreythewang commented Oct 24, 2018

graceguo-supercat commented Oct 24, 2018 •

edited

Loading

graceguo-supercat commented Oct 25, 2018 •

edited

Loading

jeffreythewang commented Oct 25, 2018

kristw Oct 25, 2018

kristw Oct 25, 2018

kristw Oct 25, 2018

kristw Oct 25, 2018

kristw Oct 25, 2018

kristw Oct 25, 2018

jeffreythewang Oct 29, 2018

kristw Oct 25, 2018

jeffreythewang commented Oct 29, 2018 •

edited

Loading

graceguo-supercat Nov 7, 2018 •

edited

Loading

graceguo-supercat left a comment

Add separate limit setting for SqlLab #4941

Add separate limit setting for SqlLab #4941

Conversation

jeffreythewang commented May 7, 2018

Summary

Description

Examples

Related PR

Related Issues

mistercrunch commented May 7, 2018

jeffreythewang commented May 7, 2018 • edited Loading

codecov-io commented May 7, 2018 • edited Loading

Codecov Report

mistercrunch commented Jun 4, 2018

jeffreythewang commented Jun 5, 2018 • edited Loading

timifasubaa commented Oct 15, 2018 • edited Loading

jeffreythewang left a 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

timifasubaa commented Oct 22, 2018

graceguo-supercat Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

jeffreythewang Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

jeffreythewang commented Oct 24, 2018

graceguo-supercat commented Oct 24, 2018 • edited Loading

graceguo-supercat commented Oct 25, 2018 • edited Loading

jeffreythewang commented Oct 25, 2018

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

jeffreythewang commented Oct 29, 2018 • edited Loading

graceguo-supercat Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

graceguo-supercat left a comment

Choose a reason for hiding this comment

jeffreythewang commented May 7, 2018 •

edited

Loading

codecov-io commented May 7, 2018 •

edited

Loading

jeffreythewang commented Jun 5, 2018 •

edited

Loading

timifasubaa commented Oct 15, 2018 •

edited

Loading

graceguo-supercat Oct 23, 2018 •

edited

Loading

jeffreythewang Oct 23, 2018 •

edited

Loading

graceguo-supercat commented Oct 24, 2018 •

edited

Loading

graceguo-supercat commented Oct 25, 2018 •

edited

Loading

jeffreythewang commented Oct 29, 2018 •

edited

Loading

graceguo-supercat Nov 7, 2018 •

edited

Loading