eschutho opened a new pull request, #42899:
URL: https://github.com/apache/superset/pull/42899
### SUMMARY
Drill to Detail pagination fails on **Trino/Presto**: navigating to page 2
of the results produces a query that Trino rejects with a syntax error:
```
line 7:11: mismatched input 'OFFSET'. Expecting: <EOF>
```
This was reported when using Trino as the query engine. The same query runs
fine in SQL Lab, because SQL Lab never emits an `OFFSET` — only the automated
pagination path does.
**Problem**
Superset builds paginated queries with SQLAlchemy's dialect-aware
construction (`qry.limit(row_limit).offset(row_offset)` in `models/helpers.py`)
and compiles them in `Database.compile_sqla_query`. In SQLAlchemy the
*compiled* clause order is decided entirely by the driver's dialect
(`limit_clause`), not by the order the methods are called.
Trino and Presto require `OFFSET` to appear **before** `LIMIT` (unlike the
ANSI `LIMIT ... OFFSET` ordering). Whether Superset emits valid SQL therefore
depends on which driver the connection resolves to:
- The official `trino` package and the legacy `sqlalchemy-trino` package
both override `limit_clause` to emit `OFFSET ... LIMIT` → correct.
- **PyHive's** Presto/Trino dialects (`PrestoCompiler` / `TrinoCompiler`) do
**not** override `limit_clause`; they inherit SQLAlchemy's ANSI `LIMIT ...
OFFSET` → **rejected by Presto/Trino**.
So any connection that resolves to a PyHive dialect — including any vanilla
`presto://` connection on stock Superset — produces invalid SQL as soon as an
offset is applied (Drill to Detail page 2+, server-side pagination, etc.).
Verified directly:
```python
>>> from sqlalchemy import table, column, select
>>> from pyhive.sqlalchemy_presto import PrestoDialect
>>> q = select([table('t', column('a')).c.a]).limit(50).offset(50)
>>> str(q.compile(dialect=PrestoDialect(), compile_kwargs={'literal_binds':
True}))
'SELECT "t"."a" FROM "t" LIMIT 50 OFFSET 50' # ← invalid for Presto/Trino
```
**Fix**
Guarantee the ordering in Superset rather than depending on the driver:
- Add an `offset_before_limit` engine-spec flag (default `False`, set to
`True` on `PrestoBaseEngineSpec`, inherited by both Presto and Trino).
- Add `BaseEngineSpec.apply_offset_before_limit(sql)`, which re-renders the
statement through sqlglot's dialect-aware generator (which orders `OFFSET`
before `LIMIT` for these dialects). A cheap textual gate means it only runs
when the flag is set **and** the SQL is actually in the invalid order, so
drivers that already emit `OFFSET` first are left byte-for-byte untouched.
- Call it once at the end of `Database.compile_sqla_query`, the single choke
point where datasource queries are turned into SQL.
This fixes every automated-offset path (Drill to Detail, samples, server
pagination) for Presto and Trino, independent of the installed SQLAlchemy
driver.
### BEFORE/AFTER
**Before** (page 2, PyHive-backed Trino/Presto): `... LIMIT 50 OFFSET 50` →
`mismatched input 'OFFSET'`.
**After**: `... OFFSET 50 LIMIT 50` → valid, page 2 loads.
### TESTING INSTRUCTIONS
1. Connect a Trino (or Presto) database whose connection uses a driver that
emits ANSI `LIMIT ... OFFSET` (e.g. PyHive).
2. Open any chart → Drill to Detail → go to page 2 of the results.
3. Before this change the query fails with `mismatched input 'OFFSET'`;
after it, the page loads.
Automated coverage added:
- `tests/unit_tests/db_engine_specs/test_trino.py` —
`apply_offset_before_limit` reorders ANSI ordering, leaves
already-correct/limit-only SQL untouched, and is a no-op for engines that don't
set the flag.
- `tests/unit_tests/models/core_test.py` — end-to-end through
`compile_sqla_query`, driving a Trino `Database` backed by a dialect that emits
ANSI ordering and asserting the compiled SQL puts `OFFSET` before `LIMIT`.
```
pytest tests/unit_tests/db_engine_specs/test_trino.py
tests/unit_tests/db_engine_specs/test_presto.py \
tests/unit_tests/db_engine_specs/test_base.py
tests/unit_tests/models/core_test.py
```
### 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))
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
