viet-nv opened a new pull request, #43578:
URL: https://github.com/apache/superset/pull/43578
### SUMMARY
`SQLStatement.set_limit_value()` (the `FORCE_LIMIT` path) built a fresh
`exp.Limit` node and assigned it over `self._parsed.args["limit"]`,
discarding
whatever was already there.
In sqlglot's ClickHouse dialect, `LIMIT 2 BY id` parses into that *same*
`Limit`
node, with the `BY` columns living in its `expressions`. So applying the SQL
Lab
row limit rewrote `SELECT * FROM t ORDER BY id, val LIMIT 2 BY id` into
`SELECT * FROM t ORDER BY id, val LIMIT 1001` — 1001 rows overall instead of
2 rows per `id`. Different result set, no error, nothing in the logs.
`get_limit_value()` compounded it: it reported the per-group `2` as if it
were a
row cap, so `_set_query_limit()` in `superset/commands/sql_lab/execute.py`
took
`min(2, row_limit)` and clamped the query to 2 rows total.
The `LIMIT n OFFSET m BY x` and `LIMIT m, n BY x` spellings are hit too, and
worse: sqlglot hangs their `BY` columns off the **`Offset`** node rather than
`Limit`, so overwriting only `args["limit"]` produced `LIMIT 1001 OFFSET 1
BY id`
— the grouping survives but bound to the wrong number.
**Fix.** A new private `_has_limit_by()` checks the `expressions` of the root
`Limit` *and* `Offset` nodes, covering both AST shapes. Then:
- `get_limit_value()` returns `None` when a `LIMIT ... BY` is present — a
per-group cap is not a row cap, so it must not feed `_set_query_limit()`.
- `set_limit_value()` with `FORCE_LIMIT` **wraps** instead of overwriting:
`SELECT * FROM (<original>) LIMIT n`, reusing the existing `WRAP_SQL`
rewrite
verbatim.
**Design decision — wrap, not cap in place.** The obvious alternative is to
keep
the `BY` and append the cap alongside it, which is what ClickHouse itself
supports: `LIMIT 2 BY id LIMIT 1001`. That is not available here — sqlglot
cannot parse that form at all, in either order:
ParseError: Found multiple 'LIMIT' clauses. Line 1, Col: 47.
Emitting it would produce SQL that Superset can no longer reparse (and that
would break any later `SQLStatement` pass over the same text). Wrapping is
semantically equivalent, round-trips cleanly through sqlglot, and reuses code
that is already in this method. The same sqlglot limitation means a user
query
that *already* spells both clauses fails at parse time today — that is a
pre-existing upstream gap, not something this PR changes.
Non-ClickHouse dialects leave `Limit.expressions` empty (`Fetch` and
TSQL/Teradata
`TOP` too), so `_has_limit_by()` is always `False` for them and the in-place
path
is bit-for-bit unchanged — as it is for plain ClickHouse `LIMIT` / `LIMIT n
OFFSET m`.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not a UI change. The behavioral before/after, with `row_limit = 1001` on
ClickHouse:
| input | before | after |
|---|---|---|
| `SELECT * FROM t ORDER BY id, val LIMIT 2 BY id` | `... LIMIT 1001`
(grouping lost) | `SELECT * FROM (SELECT * FROM t ORDER BY id, val LIMIT 2 BY
id) LIMIT 1001` |
| `SELECT * FROM t ORDER BY id, val LIMIT 2 OFFSET 1 BY id` | `... LIMIT
1001 OFFSET 1 BY id` (wrong bound) | `SELECT * FROM (SELECT * FROM t ORDER BY
id, val LIMIT 2 OFFSET 1 BY id) LIMIT 1001` |
| `SELECT * FROM t ORDER BY id, val LIMIT 1, 2 BY id` | `... LIMIT 1001
OFFSET 1 BY id` (wrong bound) | `SELECT * FROM (SELECT * FROM t ORDER BY id,
val LIMIT 2 OFFSET 1 BY id) LIMIT 1001` |
| `SELECT * FROM t ORDER BY c LIMIT 555` | `... LIMIT 1001` | `... LIMIT
1001` (unchanged) |
`get_limit_value()` returned `2` for all three `BY` rows before; it returns
`None` now.
### TESTING INSTRUCTIONS
Unit tests:
```bash
pytest tests/unit_tests/sql/parse_tests.py -k limit
```
Or the whole file / suite: `pytest tests/unit_tests/sql/parse_tests.py`
(929 passed, 1 xfailed), `pytest tests/unit_tests` (13414 passed).
Manually, against a ClickHouse database in SQL Lab:
1. `CREATE TABLE limit_by (id Int32, val Int32) ENGINE = MergeTree ORDER BY
id;`
2. `INSERT INTO limit_by VALUES (1,1),(1,2),(1,3),(2,1),(2,2),(2,3);`
3. Run `SELECT * FROM limit_by ORDER BY id, val LIMIT 2 BY id` with the SQL
Lab
row-limit dropdown at 1000.
4. Expected: 4 rows — 2 per `id`. On master you get either 6 rows (the `BY`
is dropped and the 1000-row cap applies) or 2 rows (the per-group
2 is read as a row cap).
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
