[sqllab] Fix limit parsing bug when using limit-offset comma notation #7912
Conversation
superset/sql_parse.py
Outdated
| @@ -182,7 +182,7 @@ def _extract_limit_from_query(self, statement): | |||
| _, token = statement.token_next(idx=idx) | |||
| if token: | |||
| if isinstance(token, IdentifierList): | |||
| _, token = token.token_next(idx=-1) | |||
| token = token.tokens[-1] | |||
There was a problem hiding this comment.
Could you use token_next(..., reverse=True). This skips comments, whitespace etc. and thus is probably more robust.
There was a problem hiding this comment.
I agree using the native methods would be preferable, and I actually tried to go down that path, but it seems the reverse argument is in fact private (_reverse), and didn't seem to work similar to sorted(..., reverse=True). Will see if I can come up with a more robust solution.
There was a problem hiding this comment.
Can you check my new proposal @john-bodley ?
Codecov Report
@@ Coverage Diff @@
## master #7912 +/- ##
==========================================
- Coverage 73.67% 65.91% -7.77%
==========================================
Files 111 465 +354
Lines 11695 22257 +10562
Branches 0 2425 +2425
==========================================
+ Hits 8616 14670 +6054
- Misses 3079 7466 +4387
- Partials 0 121 +121
Continue to review full report at Codecov.
|
| @@ -182,7 +182,10 @@ def _extract_limit_from_query(self, statement): | |||
| _, token = statement.token_next(idx=idx) | |||
| if token: | |||
| if isinstance(token, IdentifierList): | |||
| _, token = token.token_next(idx=-1) | |||
| # In case of "LIMIT <offset>, <limit>", find comma and extract | |||
There was a problem hiding this comment.
@villebro this seems good. I just checked the source code to verify that the IdentifierList contains the , punctuation and thus idx will not be None.
…apache#7912) * Fix limit parsing bug when using limit-offset comma notation * Use native sqlparse semantics to find limit * black
mistercrunch
added
🏷️ bot
CATEGORY
Choose one
SUMMARY
Currently parsing of limit from query with limit-offset comma notation (
LIMIT <offset>, <limit>) incorrectly assumes reversed order (LIMIT <limit>, <offset>). This fixes the parsing logic and accompanying unit tests.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
REVIEWERS
@john-bodley (I checked git blame and saw that you had worked on this recently)