jeffreythewang commented on issue #4941: Add separate limit setting for SqlLab URL: https://github.com/apache/incubator-superset/pull/4941#issuecomment-394854454 @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 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.
---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
