rusackas commented on code in PR #42588:
URL: https://github.com/apache/superset/pull/42588#discussion_r3685777284
##########
superset/sql/parse.py:
##########
@@ -1274,6 +1274,13 @@ def set_limit_value(
Modify the `LIMIT` or `TOP` value of the SQL statement inplace.
"""
if method == LimitMethod.FORCE_LIMIT:
+ # `SHOW` statements (`SHOW TABLES`, `SHOW DATABASES`, `SHOW CREATE
+ # TABLE`, etc.) have no real `LIMIT` slot in sqlglot's expression
+ # tree: setting `args["limit"]` doesn't get rejected, it renders a
+ # malformed statement with two `LIMIT` keywords, which engines
+ # like StarRocks reject outright. Leave them untouched.
+ if isinstance(self._parsed, exp.Show):
Review Comment:
Good catch, and I traced it down. Root cause is in sqlglot itself: MySQL's
(and StarRocks') generator has a method that renders `args['limit']` and then
wraps that output in its own `LIMIT` text, expecting a bare value there. We're
setting a full `exp.Limit` node, which already renders as `LIMIT 1000` on its
own, so MySQL/StarRocks double-wraps into the `LIMIT LIMIT 1000` that started
this whole thing. Snowflake's generator just does `self.sql(expression,
"limit")` with no extra wrapping, so the exact same value shape happens to
render fine there. It's not really 'some dialects support it and some
don'\''t', sqlglot's own dialects just disagree on whether that arg should be a
bare value or a full Limit node, and we can only pick one shape.
Restricting the guard by dialect would mean hardcoding an allowlist against
an internal sqlglot detail that could shift under us, so I'd rather not.
Practically it's low-stakes too, SHOW TABLES/DATABASES/CREATE TABLE results are
bounded by schema size, not by query volume, so losing the LIMIT clamp on
Snowflake specifically isn't much of a regression, just a lost optimization on
an already-small result set.
If we want it precise rather than just safe, the real fix is probably
upstream in sqlglot, making that arg's shape consistent across dialects. Happy
to open that if it'd help, but I don't think it should hold up this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
