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

[sqllab] remove limiting at the display level #5413

Merged

Conversation

timifasubaa
Copy link
Contributor

@timifasubaa timifasubaa commented Jul 17, 2018

Especially as we now enforce the limit into the query(#5295), there is no longer a need to reduce the payload before sending to the frontend. This PR removes that logic.

This should also improve performance as we no longer need to json.loads and dumps the data in the middle.
@john-bodley @mistercrunch

@timifasubaa timifasubaa changed the title remove limiting at the display level [sqllab] remove limiting at the display level Jul 17, 2018
return json_success(
json.dumps(
payload_json, default=utils.json_iso_dttm_ser, ignore_nan=True))
return json_success(utils.zlib_decompress_to_string(blob))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the default and ignore_nan arguments no longer valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the blob is written in celery, it is already set with those parameters.
https://github.com/apache/incubator-superset/blob/master/superset/sql_lab.py#L229

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added ignore-nan

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out ignore_nan is breaking this method. I believe it's a simplejson arg only

@timifasubaa timifasubaa force-pushed the remove_sqllab_frontend_limits branch 5 times, most recently from ceccdfd to 8489ae5 Compare July 19, 2018 23:53
@timifasubaa timifasubaa force-pushed the remove_sqllab_frontend_limits branch from 8489ae5 to ea9b12a Compare July 19, 2018 23:57
@codecov-io
Copy link

codecov-io commented Jul 20, 2018

Codecov Report

Merging #5413 into master will increase coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5413      +/-   ##
=========================================
+ Coverage   59.09%   59.1%   +0.01%     
=========================================
  Files         372     372              
  Lines       23747   23742       -5     
  Branches     2758    2758              
=========================================
  Hits        14033   14033              
+ Misses       9699    9694       -5     
  Partials       15      15
Impacted Files Coverage Δ
superset/sql_lab.py 71.6% <0%> (ø) ⬆️
superset/views/core.py 73.37% <0%> (+0.25%) ⬆️

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 6441f69...ea9b12a. Read the comment docs.

@timifasubaa timifasubaa merged commit 41447e8 into apache:master Jul 20, 2018
timifasubaa added a commit to airbnb/superset-fork that referenced this pull request Jul 25, 2018
@mistercrunch
Copy link
Member

mistercrunch commented Aug 30, 2018

I missed this a while back, but how do we allow for users to do largish CSV extracts when in async mode? @timifasubaa @john-bodley

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.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.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants