[GitHub] mistercrunch commented on issue #6942: [SQL Lab] Removing display limit

[GitBox <gi...@apache.org>]

mistercrunch commented on issue #6942: [SQL Lab] Removing display limit
URL: https://github.com/apache/incubator-superset/pull/6942#issuecomment-467257530
 
 
   There's been some back and forth on this and a history of regressions. I think I see a bad merge conflict between
   https://github.com/apache/incubator-superset/pull/4941 and https://github.com/apache/incubator-superset/pull/5866 
   
   My intention originally was to provide the administrator with a hard limit on 1- `SQL_MAX_ROW` how many rows can be exported out of the database and ultimately stored on the `results_backend` and 2- `DISPLAY_MAX_ROW ` how many rows will be returned to the user's browser.
   
   Now https://github.com/apache/incubator-superset/pull/4941 was created before https://github.com/apache/incubator-superset/pull/5866  and merged after it and appears to deprecate `DISPLAY_MAX_ROW` in favor of something new named `DEFAULT_SQLLAB_LIMIT`. That looks wrong to me. Naming-wise `DEFAULT_SQLLAB_LIMIT` should just be the default in the LIMIT component, and the user should be able to up it up until it reaches the admin-setup `DISPLAY_MAX_ROW` (which is now gone...).
   
   My point is we probably need all 3 configuration settings here. We need to prevent the user from shooting themselves in the foot and return enough row to crash their browser.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org