feat: add global max row limit #16683

villebro · 2021-09-13T08:56:54Z

SUMMARY

Assume SQL_MAX_ROW as the global maximum row count for the following queries:

Chart data result requests
Chart data sample requests
SQL Lab queries

In addition, the old VIZ_ROW_LIMIT parameter is removed, as it has been replaced by ROW_LIMIT everywhere else except in the histogram chart, hence is not really used any longer.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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

zhaoyongjie

LGTM

superset/config.py

etr2460

Instead of adding yet another sql limit, shouldn't we be using SQL_MAX_ROW to limit the max rows, regardless of what the user provides?

villebro · 2021-09-14T05:31:12Z

@etr2460 well, I did remove one, so the change is net neutral 😄 Joking aside, that's not a bad idea - so should we apply SQL_MAX_ROW as the global max for any type of query, i.e. in this case use that instead of MAX_GLOBAL_ROW_LIMIT?

etr2460 · 2021-09-14T14:44:12Z

@villebro I'm a fan of that approach personally, I think it's how it's actually supposed to work

villebro · 2021-09-15T06:00:59Z

@villebro I'm a fan of that approach personally, I think it's how it's actually supposed to work

Perfect, I'll update accordingly!

etr2460 · 2021-09-15T16:50:56Z

superset/utils/core.py

@@ -1762,3 +1762,29 @@ def parse_boolean_string(bool_str: Optional[str]) -> bool:
        return bool(strtobool(bool_str.lower()))
    except ValueError:
        return False
+
+
+def apply_max_row_limit(max_limit: Optional[int], limit: int) -> int:


instead of needing to pass in the max row limit, should we access the config value here? Seems like the first argument is always set to the same thing where this function is used

You know you're absolutely right, this current sig is really an artifact from earlier iterations of this PR. Will update accordingly 👍

codecov · 2021-09-16T05:01:59Z

Codecov Report

Merging #16683 (cb311dc) into master (a839649) will decrease coverage by 0.20%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #16683      +/-   ##
==========================================
- Coverage   76.97%   76.76%   -0.21%     
==========================================
  Files        1007     1007              
  Lines       54163    54168       +5     
  Branches     7463     7374      -89     
==========================================
- Hits        41690    41582     -108     
- Misses      12233    12346     +113     
  Partials      240      240

Flag	Coverage Δ
hive	`?`
mysql	`81.73% <100.00%> (+0.02%)`	⬆️
postgres	`81.79% <100.00%> (+0.02%)`	⬆️
presto	`?`
python	`81.88% <100.00%> (-0.41%)`	⬇️
sqlite	`81.36% <100.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
superset/common/query_actions.py	`92.53% <ø> (-0.43%)`	⬇️
superset/config.py	`91.44% <ø> (-0.03%)`	⬇️
superset/common/query_context.py	`90.74% <100.00%> (ø)`
superset/common/query_object.py	`94.44% <100.00%> (+3.81%)`	⬆️
superset/utils/core.py	`89.84% <100.00%> (-0.05%)`	⬇️
superset/utils/sqllab_execution_context.py	`92.43% <100.00%> (+0.06%)`	⬆️
superset/views/core.py	`76.29% <100.00%> (+0.01%)`	⬆️
superset/viz.py	`57.91% <100.00%> (+0.05%)`	⬆️
superset/db_engines/hive.py	`0.00% <0.00%> (-82.15%)`	⬇️
superset/db_engine_specs/hive.py	`69.80% <0.00%> (-16.87%)`	⬇️
... and 7 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 a839649...cb311dc. Read the comment docs.

dpgaspar

LGTM

superset/common/query_object.py

superset/config.py

etr2460

lgtm, thanks for iterating and the tests!

* feat: add global max limit * fix lint and tests * leave SAMPLES_ROW_LIMIT unchanged * fix sample rowcount test * replace max global limit with existing sql max row limit * fix test * make max_limit optional in util * improve comments (cherry picked from commit 4e3d4f6)

* feat: add global max limit * fix lint and tests * leave SAMPLES_ROW_LIMIT unchanged * fix sample rowcount test * replace max global limit with existing sql max row limit * fix test * make max_limit optional in util * improve comments

nickrbclark · 2022-04-29T13:07:38Z

in https://domain.com/api/v1/dashboard/7/datasets I was getting a type mismatch error.
in apply_max_row_limit line 1833,
min(max_limit, limit)
I was getting a
'<' not supported between instances of 'str' and 'int'
I wasn't really sure what was going on, but I was able to fix it with
min(max_limit, int(limit))

I have no idea if there's something wrong with my config, or if the particular usage of the function through this api call was bad.

superset-github-bot bot added the preset-io label Sep 13, 2021

pull-request-size bot added the size/L label Sep 13, 2021

villebro changed the title ~~feat: add global max limit~~ feat: add global max row limit Sep 13, 2021

zhaoyongjie self-requested a review September 13, 2021 09:44

zhaoyongjie approved these changes Sep 13, 2021

View reviewed changes

dpgaspar reviewed Sep 13, 2021

View reviewed changes

superset/config.py Outdated Show resolved Hide resolved

etr2460 requested changes Sep 14, 2021

View reviewed changes

villebro added 5 commits September 15, 2021 09:02

feat: add global max limit

0db8721

fix lint and tests

5446620

leave SAMPLES_ROW_LIMIT unchanged

ab4a107

fix sample rowcount test

8181d7b

replace max global limit with existing sql max row limit

769bf3e

villebro force-pushed the villebro/max-limit branch 3 times, most recently from c1de713 to dac8dfa Compare September 15, 2021 08:03

fix test

095fb8c

villebro force-pushed the villebro/max-limit branch from dac8dfa to 095fb8c Compare September 15, 2021 08:36

villebro requested a review from etr2460 September 15, 2021 14:48

etr2460 reviewed Sep 15, 2021

View reviewed changes

villebro added 2 commits September 16, 2021 06:54

Merge branch 'master' into villebro/max-limit

a93d3aa

make max_limit optional in util

203c6cf

villebro force-pushed the villebro/max-limit branch from 05a6759 to 203c6cf Compare September 16, 2021 04:54

dpgaspar approved these changes Sep 16, 2021

View reviewed changes

superset/common/query_object.py Outdated Show resolved Hide resolved

superset/config.py Outdated Show resolved Hide resolved

villebro added 2 commits September 16, 2021 14:33

Merge branch 'master' into villebro/max-limit

07c3d7e

improve comments

cb311dc

etr2460 approved these changes Sep 16, 2021

View reviewed changes

villebro merged commit 4e3d4f6 into apache:master Sep 16, 2021

villebro deleted the villebro/max-limit branch September 16, 2021 16:33

villebro added the v1.3 label Sep 16, 2021

villebro removed the v1.3 label Sep 22, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add global max row limit #16683

feat: add global max row limit #16683

villebro commented Sep 13, 2021 •

edited

Loading

zhaoyongjie left a comment

etr2460 left a comment

villebro commented Sep 14, 2021

etr2460 commented Sep 14, 2021

villebro commented Sep 15, 2021

etr2460 Sep 15, 2021

villebro Sep 15, 2021

codecov bot commented Sep 16, 2021 •

edited

Loading

dpgaspar left a comment

etr2460 left a comment

nickrbclark commented Apr 29, 2022

feat: add global max row limit #16683

feat: add global max row limit #16683

Conversation

villebro commented Sep 13, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

zhaoyongjie left a comment

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

villebro commented Sep 14, 2021

etr2460 commented Sep 14, 2021

villebro commented Sep 15, 2021

etr2460 Sep 15, 2021

Choose a reason for hiding this comment

villebro Sep 15, 2021

Choose a reason for hiding this comment

codecov bot commented Sep 16, 2021 • edited Loading

Codecov Report

dpgaspar left a comment

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

nickrbclark commented Apr 29, 2022

villebro commented Sep 13, 2021 •

edited

Loading

codecov bot commented Sep 16, 2021 •

edited

Loading