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

Setting default row_limit to 50K for line chart #6244

Merged

Conversation

michellethomas
Copy link
Contributor

This PR sets default row_limit to null for line chart, bar chart, compare chart, and area chart. Because row_limit is a new field on timeseries charts, existing charts may be affected if they have over 10,000 rows.

@mistercrunch what do you think about setting the row_limit default to null? We could also set the default to 50,000 instead, it seems less likely that a chart with 50,000 rows would render very well anyways.

@mistercrunch
Copy link
Member

mistercrunch commented Oct 31, 2018

I think we should put some upper limit as people can crash their browser if they don't. The upper limit should be lower than the number at which we know the browser will crash for sure. This may vary quite a bit based on the viz type, number of series, ...

I also think we should allow users to crash their browsers if they want to clear or bump up the limit. They might know something we don't know, have a quantum computer or something.

Notice that the label that shows the number or rows in the explore view should turn yellow and warn "you've reached the limit" to make sure users knows that data is missing. We may want to bring more attention to this and have a clear tooltip that explains the tradeoffs.

@mistercrunch
Copy link
Member

screen shot 2018-10-31 at 2 56 36 pm

@mistercrunch
Copy link
Member

Here, I've been meaning to do this for a while #6252

@codecov-io
Copy link

Codecov Report

Merging #6244 into master will decrease coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6244      +/-   ##
==========================================
- Coverage   76.85%   76.74%   -0.11%     
==========================================
  Files          47       47              
  Lines        9393     9393              
==========================================
- Hits         7219     7209      -10     
- Misses       2174     2184      +10
Impacted Files Coverage Δ
superset/db_engine_specs.py 54.91% <0%> (-0.85%) ⬇️
superset/connectors/sqla/models.py 80% <0%> (-0.75%) ⬇️

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 af38d25...fd2fcd5. Read the comment docs.

@michellethomas
Copy link
Contributor Author

Sounds good thanks Max. I did some testing on what the upper limit should be for different viz types, I think the only one we should increase is the line chart, I set it to 50000. This is still pretty slow but usable. The bar chart and area chart get pretty slow at 10K, they could be a little larger but this seems fine. The compare chart doesn't have a way of hiding the legend so there's no chance anyone is using it even at 10K currently.

@mistercrunch
Copy link
Member

+1

@michellethomas michellethomas changed the title Setting default row_limit to null for timeseries charts Setting default row_limit to 50K for line chart Nov 1, 2018
@michellethomas michellethomas merged commit 2fd18ee into apache:master Nov 1, 2018
@michellethomas michellethomas deleted the removing_default_row_limit_line branch November 1, 2018 18:01
graceguo-supercat pushed a commit to graceguo-supercat/superset that referenced this pull request Nov 1, 2018
Setting default row_limit to 50K for line chart

(cherry picked from commit 2fd18ee)
bipinsoniguavus pushed a commit to ThalesGroup/incubator-superset that referenced this pull request Dec 26, 2018
Setting default row_limit to 50K for line chart
@mistercrunch 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
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 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants