[sql lab] a better approach at limiting queries #4947

mistercrunch · 2018-05-08T05:11:32Z

Currently there are two mechanisms that we use to enforce the row
limiting constraints, depending on the database engine:

use dbapi's cursor.fetchmany()
wrap the SQL into a limiting subquery

Method 1 isn't great as it can result in the database server storing
larger than required result sets in memory expecting another fetch
command while we know we don't need that.

Method 2 has a positive side of working with all database engines,
whether they use LIMIT, ROWNUM, TOP or whatever else since sqlalchemy
does the work as specified for the dialect. On the downside though
the query optimizer might not be able to optimize this as much as an
approach that doesn't use a subquery.

Since most modern DBs use the LIMIT syntax, this adds a regex approach
to modify the query and force a LIMIT clause without using a subquery
for the database that support this syntax and uses method 2 for all
others.

Currently there are two mechanisms that we use to enforce the row limiting constraints, depending on the database engine: 1. use dbapi's `cursor.fetchmany()` 2. wrap the SQL into a limiting subquery Method 1 isn't great as it can result in the database server storing larger than required result sets in memory expecting another fetch command while we know we don't need that. Method 2 has a positive side of working with all database engines, whether they use LIMIT, ROWNUM, TOP or whatever else since sqlalchemy does the work as specified for the dialect. On the downside though the query optimizer might not be able to optimize this as much as an approach that doesn't use a subquery. Since most modern DBs use the LIMIT syntax, this adds a regex approach to modify the query and force a LIMIT clause without using a subquery for the database that support this syntax and uses method 2 for all others.

codecov-io · 2018-05-10T21:19:37Z

Codecov Report

Merging #4947 into master will increase coverage by 0.02%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master    #4947      +/-   ##
==========================================
+ Coverage    77.1%   77.13%   +0.02%     
==========================================
  Files          44       44              
  Lines        8644     8649       +5     
==========================================
+ Hits         6665     6671       +6     
+ Misses       1979     1978       -1

Impacted Files	Coverage Δ
superset/sql_parse.py	`98.79% <100%> (-0.06%)`	⬇️
superset/models/core.py	`86.47% <100%> (-0.08%)`	⬇️
superset/sql_lab.py	`75.27% <100%> (+0.95%)`	⬆️
superset/db_engine_specs.py	`53.74% <84.61%> (+0.87%)`	⬆️

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 b391676...706d860. Read the comment docs.

betodealmeida

LGTM, but I have a few comments.

betodealmeida · 2018-05-10T23:11:46Z

superset/db_engine_specs.py

+            )
+            return database.compile_sqla_query(qry)
+        elif LimitMethod.FORCE_LIMIT:
+            no_limit = re.sub(r'(?i)\s+LIMIT\s+\d+;?(\s|;)*$', '', sql)


Scary! :-O

BTW, you can use backreferences here to replace the LIMIT number in one pass (untested):

sql = re.sub(r'(?i)(.*\s+LIMIT\s+)\d+\s*;?\s*$', r'\1 {0}'.format(limit), sql)

I barely understand what I wrote here :/

Some people, when confronted with a problem, think "I know, I'll use regular expressions." Now they have two problems.

Yeah, I just realized my example does not work when there's no LIMIT clause. :-/

You know about verbose regexes? Eg:

no_limit = re.sub(r""" (?ix) # case insensitive, verbose \s+ # whitespace LIMIT\s+\d+ # LIMIT $ROWS ;? # optional semi-colon (\s|;)*$ # any number of trailing whitespace or semi-colons, til the end """, '', sql)

Though in this case I think it's pretty straightforward. You might wanna rstrip whitespace and semicolons from the end of sql, that would simplify the regex, no?

betodealmeida · 2018-05-10T23:13:22Z

superset/models/core.py

-            ).limit(limit)
-        )
-        return self.compile_sqla_query(qry)
+        return self.db_engine_spec.apply_limit_to_sql(sql, limit, self)


Better update the method name here.

I mean, rename wrap_sql_limit to something else.

went with apply_limit_to_sql

betodealmeida · 2018-05-10T23:19:17Z

tests/db_engine_specs_test.py

+        db = Database(sqlalchemy_uri='mysql://localhost')
+        limited = MySQLEngineSpec.apply_limit_to_sql(sql, 1000, db)
+        expected = 'SELECT * FROM (SELECT * FROM a LIMIT 10) LIMIT 1000'
+        self.assertEquals(expected, limited)


I'm nervous about the regular expression, I'd add a few more unit tests covering some weird cases, eg:

SELECT 'LIMIT 1000' AS a , b FROM table LIMIT 1000 <EOF>

And maybe some stuff with extra space at the end:

... LIMIT 1000 ; <EOF>

mistercrunch · 2018-05-14T19:44:00Z

Addressed comments, merging

john-bodley · 2018-05-19T01:22:56Z

@betodealmeida @mistercrunch are you sure we want to be augmenting a query that someone authors? This seems overly magical to me, i.e., if you believe you're running the SELECT * FROM schema.table query under the covers you're really running SELECT * FROM schema.table LIMIT n. This can be somewhat confusing if one tries to download and analyze a CSV file assuming all the rows are there or if one was trying to track said query using the audit logs etc.

SQL is a powerful language and it seems like educating users about how best to use SQL rather than adding somewhat opaque handrails.

* [sql lab] a better approach at limiting queries Currently there are two mechanisms that we use to enforce the row limiting constraints, depending on the database engine: 1. use dbapi's `cursor.fetchmany()` 2. wrap the SQL into a limiting subquery Method 1 isn't great as it can result in the database server storing larger than required result sets in memory expecting another fetch command while we know we don't need that. Method 2 has a positive side of working with all database engines, whether they use LIMIT, ROWNUM, TOP or whatever else since sqlalchemy does the work as specified for the dialect. On the downside though the query optimizer might not be able to optimize this as much as an approach that doesn't use a subquery. Since most modern DBs use the LIMIT syntax, this adds a regex approach to modify the query and force a LIMIT clause without using a subquery for the database that support this syntax and uses method 2 for all others. * Fixing build * Fix lint * Added more tests * Fix tests

mistercrunch · 2018-05-30T01:08:36Z

I think we agree that we need a way to prevent users from running infinite billion rows query that will hurt the db server, network, web server and ultimately crash their browser.

Now we've been doing this two ways depending on the DB engine, fetchmany, which I think puts too much strain on the database server. It's rough on Presto/Hive for instance and in many cases is a similar internal cost as running the whole thing from the db's perspective. Then there's the wrap-in-a-subquery approach, that's better and prefer that to fetchmany, though I think many db optimizer won't do as good with that as with the LIMIT in the main core query. It requires more complex query editing but is preferable to the subquery approach as it may result in a simpler query plan.

I also looked at how other tools seem to be doing this, and they edit the user query much like we do here, likely for the reasons listed above.

* [sql lab] a better approach at limiting queries Currently there are two mechanisms that we use to enforce the row limiting constraints, depending on the database engine: 1. use dbapi's `cursor.fetchmany()` 2. wrap the SQL into a limiting subquery Method 1 isn't great as it can result in the database server storing larger than required result sets in memory expecting another fetch command while we know we don't need that. Method 2 has a positive side of working with all database engines, whether they use LIMIT, ROWNUM, TOP or whatever else since sqlalchemy does the work as specified for the dialect. On the downside though the query optimizer might not be able to optimize this as much as an approach that doesn't use a subquery. Since most modern DBs use the LIMIT syntax, this adds a regex approach to modify the query and force a LIMIT clause without using a subquery for the database that support this syntax and uses method 2 for all others. * Fixing build * Fix lint * Added more tests * Fix tests

mistercrunch added 2 commits May 10, 2018 11:12

Fixing build

08ff644

mistercrunch force-pushed the limits branch from 89ce1d8 to 08ff644 Compare May 10, 2018 20:59

Fix lint

94b939d

betodealmeida reviewed May 10, 2018

View reviewed changes

mistercrunch force-pushed the limits branch from 96bfd22 to 4940afe Compare May 13, 2018 18:34

Added more tests

7e81913

mistercrunch force-pushed the limits branch from 4940afe to 7e81913 Compare May 13, 2018 18:38

Fix tests

706d860

mistercrunch force-pushed the limits branch from 2f86b95 to 706d860 Compare May 14, 2018 19:02

mistercrunch merged commit b839608 into apache:master May 14, 2018

mistercrunch deleted the limits branch May 14, 2018 19:44

timifasubaa mentioned this pull request May 17, 2018

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

Merged

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.26.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sql lab] a better approach at limiting queries #4947

[sql lab] a better approach at limiting queries #4947

mistercrunch commented May 8, 2018

codecov-io commented May 10, 2018 •

edited

Loading

betodealmeida left a comment

betodealmeida May 10, 2018

mistercrunch May 11, 2018

betodealmeida May 11, 2018

betodealmeida May 10, 2018

betodealmeida May 10, 2018

mistercrunch May 13, 2018

betodealmeida May 10, 2018

mistercrunch commented May 14, 2018

john-bodley commented May 19, 2018 •

edited

Loading

mistercrunch commented May 30, 2018

[sql lab] a better approach at limiting queries #4947

[sql lab] a better approach at limiting queries #4947

Conversation

mistercrunch commented May 8, 2018

codecov-io commented May 10, 2018 • edited Loading

Codecov Report

betodealmeida left a comment

Choose a reason for hiding this comment

betodealmeida May 10, 2018

Choose a reason for hiding this comment

mistercrunch May 11, 2018

Choose a reason for hiding this comment

betodealmeida May 11, 2018

Choose a reason for hiding this comment

betodealmeida May 10, 2018

Choose a reason for hiding this comment

betodealmeida May 10, 2018

Choose a reason for hiding this comment

mistercrunch May 13, 2018

Choose a reason for hiding this comment

betodealmeida May 10, 2018

Choose a reason for hiding this comment

mistercrunch commented May 14, 2018

john-bodley commented May 19, 2018 • edited Loading

mistercrunch commented May 30, 2018

codecov-io commented May 10, 2018 •

edited

Loading

john-bodley commented May 19, 2018 •

edited

Loading