viet-nv commented on code in PR #43578:
URL: https://github.com/apache/superset/pull/43578#discussion_r3869448998
##########
superset/sql/parse.py:
##########
@@ -1427,11 +1450,17 @@ def set_limit_value(
if not isinstance(self._parsed, exp.Query):
return
- if method == LimitMethod.FORCE_LIMIT:
+ # A ClickHouse `LIMIT ... BY` occupies the very `limit`/`offset` slot
that
+ # `FORCE_LIMIT` overwrites, so forcing a row cap in place would drop
the
+ # `BY` grouping and silently change what the query returns. The cap
can't
+ # be appended alongside it either -- sqlglot rejects ClickHouse's
native
+ # `LIMIT n BY x LIMIT m` with "Found multiple 'LIMIT' clauses" -- so it
+ # goes on a wrapping query instead, exactly as `WRAP_SQL` does.
+ if method == LimitMethod.FORCE_LIMIT and not self._has_limit_by():
self._parsed.args["limit"] = exp.Limit(
expression=exp.Literal(this=str(limit), is_string=False)
)
- elif method == LimitMethod.WRAP_SQL:
+ elif method in {LimitMethod.FORCE_LIMIT, LimitMethod.WRAP_SQL}:
self._parsed = exp.Select(
Review Comment:
Thanks — I checked this against a live ClickHouse (26.9.1.52) rather than
reasoning about it, and `WITH TOTALS` specifically is safe. The docs are
explicit that it works in subqueries ("You can use `WITH TOTALS` in subqueries,
including subqueries in the JOIN clause"), and empirically the wrapped form and
ClickHouse's own `LIMIT n BY x LIMIT m` return byte-identical responses for a
`GROUP BY … WITH TOTALS … LIMIT 2 BY k` query — same rows, same `"totals"`
block, same `rows_before_limit_at_least`. Same for `WITH ROLLUP`/`WITH CUBE`,
which are just extra rows in the data stream, and for inner `ORDER BY`. I've
added a `WITH TOTALS` + `LIMIT BY` test to pin that down.
Your underlying point was right, though — just not about that clause. Two
modifiers sqlglot models genuinely aren't subquery-safe, and 83cab07 fixes both
by preserving them at the outer level as you suggested:
- **`FORMAT`** was a real bug this PR introduced: `SELECT * FROM (… LIMIT 2
BY a FORMAT JSON) LIMIT 1001` is a syntax error in ClickHouse.
- **`SETTINGS`** was silently rescoped — a `SETTINGS` on a subquery binds to
that subquery only (verified: `SELECT * FROM (SELECT
getSetting('max_block_size') SETTINGS max_block_size=777)` returns `777`), so
top-level-only settings like `extremes` would have stopped applying.
Both now ride on the wrapper, which is where the original query had them.
Row-producing modifiers stay inside, since hoisting those would change the
result rather than preserve it.
On avoiding the rewrite entirely: I did try emitting ClickHouse's native
`LIMIT n BY x LIMIT m` (sqlglot *can* generate it via a nested
`exp.Limit(this=…)`, and it runs fine). It's a dead end because sqlglot can't
parse it back — `Found multiple 'LIMIT' clauses` — and Superset re-parses its
own `executed_sql` in the CSV export, streaming export, and `raise_for_access`
paths, so that form would break all three. Worth an upstream sqlglot fix, but
not something this PR can depend on.
--
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]
