fix(sql): only force a LIMIT onto query expressions - #43097
Conversation
`apply_limit()` skips only *mutating* statements, so every read-only
non-query statement -- `SHOW`, `DESCRIBE`, `SET`, `USE`, `GRANT`, and
anything sqlglot falls back to parsing as an opaque `Command` -- reaches
`SQLStatement.set_limit_value()`, which has nowhere valid to put a limit.
`FORCE_LIMIT` branch, where MySQL/StarRocks reject the doubled `LIMIT`
keyword sqlglot renders ("Getting syntax error ... Unexpected input
'LIMIT'"). Two gaps remained:
- `WRAP_SQL` was left unguarded, so it rewrote any non-query statement
as `SELECT * FROM (SHOW DATABASES)`, discarding it quietly instead of
failing.
- A single node-type special-case stops holding as soon as another
statement type's generator learns to render a `limit` arg.
Guard on the node category at the top of the method instead: query
expressions (`SELECT`, `UNION`, subqueries) get a limit, everything else
is returned untouched, for every limit method.
`is_select()` would have been too narrow here -- `UNION` parses as
`exp.Union` and a parenthesized query as `exp.Subquery`, neither of which
is an `exp.Select`, and both must keep being limited.
Code Review Agent Run #9aecaaActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43097 +/- ##
==========================================
- Coverage 66.64% 66.63% -0.02%
==========================================
Files 2866 2866
Lines 162896 162852 -44
Branches 37525 37504 -21
==========================================
- Hits 108568 108512 -56
- Misses 52203 52214 +11
- Partials 2125 2126 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
thanks @villebro, description updated to mention the PR instead of the issue. |
|
Bito Automatic Review Skipped – PR Already Merged |
3 participants
SUMMARY
apply_limit()insuperset/sql_lab.pyskips only mutating statements, so every read-only non-query statement —SHOW,DESCRIBE,SET,USE,GRANT, and anything sqlglot falls back to parsing as an opaqueCommand— reachesSQLStatement.set_limit_value(), which has nowhere valid to put a limit.#36939 fixed the loudest case by special-casing
exp.Showinside theFORCE_LIMITbranch: on MySQL/StarRocks a forced limit renders aSHOWwith twoLIMITkeywords and the engine rejects it outright.Two gaps remained:
WRAP_SQLwas never guarded, so it rewrites any non-query statement asSELECT * FROM (SHOW DATABASES) LIMIT 1001, discarding the statement quietly instead of failing.limitarg.So this guards on the node category at the top of the method instead: query expressions (
SELECT,UNION, subqueries) get a limit, everything else is returned untouched, for every limit method.is_select()would have been too narrow —UNIONparses asexp.Unionand a parenthesized query asexp.Subquery, neither of which is anexp.Select, and both must keep being limited. I verifiedexp.Querycovers exactlySelect/Union/Subqueryand excludesShow/Describe/Use/Set/Grant/Command/Valueson both sqlglot 30.16.0.Found via querying starrocks, where
show databases from default_catalog;failed for every user. The same bug breaksSHOW DATABASESandSHOW CREATE FUNCTIONwhich are common operation for listing resources and udfs.TESTING INSTRUCTIONS
Reverting just the
parse.pyhunk turns 13 of these red, allWRAP_SQL— including the newDESCRIBE/USE/SET/GRANTcases, which #36939's guard does not cover.Manually, against a StarRocks or MySQL connection in SQL Lab with a row limit set, run
SHOW DATABASES;. Before: syntax error on the injectedLIMIT. After: the statement runs.Full local run:
pytest tests/unit_tests --ignore=tests/unit_tests/mcp_service→ 9143 passed, 0 failed.mcp_serviceis excluded only because it downloads tiktoken encodings at import and my environment has no egress; it is untouched by this change.ADDITIONAL INFORMATION