[sqllab] force limit queries only when there is no existing limit #5023

timifasubaa · 2018-05-17T02:20:40Z

This PR (#4947) implemented a more efficient way to limit queries by attaching the limit statement to the statement. However, it always attaches the default limit to all queries even when a fifferent (possibly smaller) limit is specified in the query. This is because query.limit is always set to the default value, never the specified limit value.
In this PR, I do some query parsing to make sure query.limit is correctly specified upstream. Then the query augnmenting logic is only called when there no limit specified.

@mistercrunch @john-bodley @michellethomas

codecov-io · 2018-05-17T04:01:22Z

Codecov Report

Merging #5023 into master will increase coverage by 0.02%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #5023      +/-   ##
==========================================
+ Coverage    77.5%   77.52%   +0.02%     
==========================================
  Files          44       44              
  Lines        8720     8728       +8     
==========================================
+ Hits         6758     6766       +8     
  Misses       1962     1962

Impacted Files	Coverage Δ
superset/views/core.py	`74.66% <ø> (ø)`	⬆️
superset/sql_lab.py	`75.4% <100%> (+0.13%)`	⬆️
superset/db_engine_specs.py	`53.98% <90%> (+0.54%)`	⬆️

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 f611797...a9d7faf. Read the comment docs.

mistercrunch · 2018-05-17T16:52:53Z

superset/sql_lab.py

@@ -171,6 +171,7 @@ def handle_error(msg):
    # Limit enforced only for retrieving the data, not for the CTA queries.
    superset_query = SupersetQuery(rendered_query)
    executed_sql = superset_query.stripped()
+    SQL_MAX_ROWS = int(app.config.get('SQL_MAX_ROW', None))


None is implicit on the get method and will make int raise

In [1]: int(None) TypeError: int() argument must be a string, a bytes-like object or a number, not 'NoneType'

I'll send out the fir for this.

mistercrunch · 2018-05-17T16:55:58Z

superset/sql_lab.py

@@ -185,7 +186,8 @@ def handle_error(msg):
                query.user_id, start_dttm.strftime('%Y_%m_%d_%H_%M_%S'))
        executed_sql = superset_query.as_create_table(query.tmp_table_name)
        query.select_as_cta_used = True
-    elif (query.limit and superset_query.is_select()):
+    elif (not query.limit and superset_query.is_select() and SQL_MAX_ROWS):


Wait I think you remove all handling of query.limit where it's defined

@mistercrunch I tested it and it works correctly.

I changed the place where query.limit is defined upstream and it is None only when it is not specified. It is only in those cases that we append the default limit to the end.

mistercrunch · 2018-05-17T19:32:19Z

superset/utils.py

+    tokens = sql.split()
+    try:
+        if 'limit' in tokens:
+            limit_pos = tokens.index('limit') + 1


Oh that looks brittle here, for example limit can exist within a subquery, or there can be line breaks or tabs as separators instead of spaces. We should use the same method / regex approach as the one I defined. Maybe that method in db_engine_spec can receive max_limit and only apply alter if the max
_limit is lower

Ah, true. I hadn't thought of the subquery case. But split can handle the tabs and newlines just fine. I'll go ahead to reuse your regex approach as that is more robust.
And your suggestion of adding the logic within the checking function makes sense.

timifasubaa · 2018-05-19T01:07:29Z

@mistercrunch That makes sense. I took out your regex and made it a helper method in utils. Now I apply the limit upstream if it exists then we modify the query with the max value if the specified value is more than the max value or there is no specified value. Lmk what you think.

timifasubaa · 2018-05-22T01:07:41Z

PING

mistercrunch

I think it'd be simpler to just make apply_limit_to_sql extract the LIMIT number, check if <, and replace conditionally, all using pretty much the same one regex.

Then add unit tests similar to the existing ones confirming that it works as expected.

mistercrunch · 2018-05-22T03:49:38Z

superset/utils.py

+                    "limit specified was not an integer: '{}'".format(specified_limit))
+                specified_limit = None
+            return specified_limit
+    return None


return None is implicit in python

mistercrunch · 2018-05-22T03:53:17Z

superset/utils.py

+    sql_without_limit = get_query_without_limit(sql)
+    if sql_without_limit != sql:
+        limit_clause = sql.partition(sql_without_limit)[2]
+        limit_match = re.search('limit\s(\d+)', limit_clause, re.IGNORECASE)


We should use the same regex as above to extract the limit number.

mistercrunch · 2018-05-22T03:53:38Z

superset/utils.py

+                """, '', sql)
+
+
+def get_limit_from_sql(sql):


This only works for databases that uses the LIMIT syntax. Say if you take MS SQL Server that goes for the SELECT TOP 100 * FROM ... this piece of code doesn't apply.

This calls for moving this logic within db_engine_spec. I'm thinking apply_limit_to_sql should be be expected to apply a limit only if needed.

@mistercrunch As is, we will append limit <default_limit> to MS SQL Server queries.
I moved the get_limit_from_query to db_engine_spec and that should allow us override the method for dbs with different ways of specifying the limit.

mistercrunch · 2018-05-22T04:05:01Z

superset/sql_lab.py

@@ -171,6 +171,8 @@ def handle_error(msg):
    # Limit enforced only for retrieving the data, not for the CTA queries.
    superset_query = SupersetQuery(rendered_query)
    executed_sql = superset_query.stripped()
+    SQL_MAX_ROWS = app.config.get('SQL_MAX_ROW', None)
+    SQL_MAX_ROWS = int(SQL_MAX_ROWS) if SQL_MAX_ROWS else None


Can we not assume that SQL_MAX_ROWS is an int, why casting?

john-bodley

So I agree that this PR resolves a regression introduced in #4947 but I think (per my comment there) that adding a LIMIT to a query without the user's knowledge isn't ideal and thus it seems more sense to revert #4947. Additionally the work in #4834 with pre-fetching results future dilutes the need to define an explicit limit.

john-bodley · 2018-05-22T22:47:20Z

superset/sql_lab.py

@@ -171,6 +171,8 @@ def handle_error(msg):
    # Limit enforced only for retrieving the data, not for the CTA queries.
    superset_query = SupersetQuery(rendered_query)
    executed_sql = superset_query.stripped()
+    SQL_MAX_ROWS = app.config.get('SQL_MAX_ROW', None)


Also app.config.get('SQL_MAX_ROW', None) is equivalent to app.config.get('SQL_MAX_ROW') as the second slot is only used to return a non-None value if the key doesn't exist.

john-bodley · 2018-05-22T22:50:14Z

superset/utils.py

+        limit_match = re.search('limit\s(\d+)', limit_clause, re.IGNORECASE)
+        if limit_match:
+            specified_limit = limit_match.group(1)
+            try:


This try/except is unnecessary as the regex explicitly matched a an integer. If the value was a non-integer limit_match would be None.

True. Taken out completely.

john-bodley · 2018-05-22T22:53:52Z

superset/utils.py

+        if limit_match:
+            specified_limit = limit_match.group(1)
+            try:
+                specified_limit = int(specified_limit) if specified_limit else None


Not that it's applicable because the try/except is unnecessary but there's no need to do the if/else logic as the specified_limit is assigned to None on line 870.

john-bodley · 2018-05-22T22:55:07Z

tests/celery_tests.py

@@ -196,13 +196,11 @@ def test_run_async_query(self):
        self.assertEqual([{'name': 'Admin'}], df.to_dict(orient='records'))
        self.assertEqual(QueryStatus.SUCCESS, query.status)
        self.assertTrue('FROM tmp_async_1' in query.select_sql)
-        self.assertTrue('LIMIT 666' in query.select_sql)


Why were these checks removed?

john-bodley · 2018-05-29T23:00:47Z

superset/db_engine_specs.py

+
+    @classmethod
+    def get_limit_from_sql(cls, sql):
+        limit_pattern = re.compile(r"""


I realize that this logic existed previously but shouldn't we use something like sqlparse to obtain the limit rather than using a regex?

I explored the sqlparse option but there were no nice way to just get the limit without recursively parsing through the query.

timifasubaa · 2018-05-30T18:02:09Z

@mistercrunch I saw the notification of your comment over email but I don't see it here.
The reason why it's duplicates is that I need to put () around the \d+ to select for it. The previous regex does not have the selector.

mistercrunch

Do we have a unit test that makes sure that the lower limit won't be replaced (the original issue we're tackling in this PR)?

timifasubaa · 2018-05-30T18:49:36Z

On it!

timifasubaa · 2018-05-30T20:14:02Z

@john-bodley @mistercrunch Ready for stamp

backto17 · 2018-06-08T02:15:11Z

superset/db_engine_specs.py

@@ -104,15 +104,32 @@ def apply_limit_to_sql(cls, sql, limit, database):
            )
            return database.compile_sqla_query(qry)
        elif LimitMethod.FORCE_LIMIT:
-            no_limit = re.sub(r"""
+            sql_without_limit = cls.get_query_without_limit(sql)
+            return '{sql_without_limit} LIMIT {limit}'.format(**locals())


this logic can generate sql like

SELECT id, name FROM main.ab_permission LIMIT 100 OFFSET 0 LIMIT 5000

which is wrong

Which engine is this on?

[sqllab] force limit queries only when there is no existing limit

timifasubaa force-pushed the fix_sqllab_commit branch 6 times, most recently from 2894e8a to 71ca5cc Compare May 17, 2018 03:46

mistercrunch requested changes May 17, 2018

View reviewed changes

mistercrunch reviewed May 17, 2018

View reviewed changes

timifasubaa force-pushed the fix_sqllab_commit branch 2 times, most recently from 8c86fa1 to 04725b0 Compare May 19, 2018 01:05

timifasubaa force-pushed the fix_sqllab_commit branch 3 times, most recently from ee75b9f to d7eb638 Compare May 19, 2018 01:45

mistercrunch requested changes May 22, 2018

View reviewed changes

john-bodley reviewed May 22, 2018

View reviewed changes

timifasubaa force-pushed the fix_sqllab_commit branch 2 times, most recently from 34313e0 to 3ffe373 Compare May 24, 2018 04:01

force limit only when there is no existing limit

1aced9b

timifasubaa force-pushed the fix_sqllab_commit branch from 3ffe373 to a4c9e76 Compare May 25, 2018 21:55

reuse_regex_logic

d38315a

timifasubaa force-pushed the fix_sqllab_commit branch 7 times, most recently from bab9b23 to 1c5dc79 Compare May 29, 2018 22:51

john-bodley reviewed May 29, 2018

View reviewed changes

timifasubaa force-pushed the fix_sqllab_commit branch from 1c5dc79 to cf3641f Compare May 29, 2018 23:29

mistercrunch approved these changes May 30, 2018

View reviewed changes

mistercrunch requested changes May 30, 2018

View reviewed changes

timifasubaa force-pushed the fix_sqllab_commit branch 5 times, most recently from 9ddcb28 to e264f12 Compare May 30, 2018 19:49

add tests

a9d7faf

timifasubaa force-pushed the fix_sqllab_commit branch from e264f12 to a9d7faf Compare May 30, 2018 19:50

john-bodley approved these changes May 31, 2018

View reviewed changes

timifasubaa merged commit cefc206 into apache:master May 31, 2018

backto17 reviewed Jun 8, 2018

View reviewed changes

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018

Merge pull request apache#5023 from timifasubaa/fix_sqllab_commit

6e432c2

[sqllab] force limit queries only when there is no existing limit

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sqllab] force limit queries only when there is no existing limit #5023

[sqllab] force limit queries only when there is no existing limit #5023

timifasubaa commented May 17, 2018 •

edited

Loading

codecov-io commented May 17, 2018 •

edited

Loading

mistercrunch May 17, 2018

timifasubaa May 17, 2018

mistercrunch May 17, 2018

timifasubaa May 17, 2018

mistercrunch May 17, 2018

timifasubaa May 18, 2018

timifasubaa commented May 19, 2018

timifasubaa commented May 22, 2018 •

edited

Loading

mistercrunch left a comment

mistercrunch May 22, 2018

mistercrunch May 22, 2018

mistercrunch May 22, 2018

mistercrunch May 22, 2018

timifasubaa May 30, 2018

mistercrunch May 22, 2018

john-bodley left a comment

john-bodley May 22, 2018

timifasubaa May 24, 2018

john-bodley May 22, 2018

timifasubaa May 24, 2018

john-bodley May 22, 2018

john-bodley May 22, 2018

timifasubaa May 29, 2018

john-bodley May 29, 2018

timifasubaa May 30, 2018

timifasubaa commented May 30, 2018

mistercrunch left a comment

timifasubaa commented May 30, 2018

timifasubaa commented May 30, 2018

backto17 Jun 8, 2018

mistercrunch Jun 8, 2018

[sqllab] force limit queries only when there is no existing limit #5023

[sqllab] force limit queries only when there is no existing limit #5023

Conversation

timifasubaa commented May 17, 2018 • edited Loading

codecov-io commented May 17, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timifasubaa commented May 19, 2018

timifasubaa commented May 22, 2018 • edited Loading

mistercrunch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timifasubaa commented May 30, 2018

mistercrunch left a comment

Choose a reason for hiding this comment

timifasubaa commented May 30, 2018

timifasubaa commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timifasubaa commented May 17, 2018 •

edited

Loading

codecov-io commented May 17, 2018 •

edited

Loading

timifasubaa commented May 22, 2018 •

edited

Loading