rebenitez1802 commented on code in PR #42899:
URL: https://github.com/apache/superset/pull/42899#discussion_r4380192211
##########
superset/db_engine_specs/presto.py:
##########
@@ -167,6 +167,13 @@ class PrestoBaseEngineSpec(BaseEngineSpec,
metaclass=ABCMeta):
supports_dynamic_schema = True
supports_catalog = supports_dynamic_catalog =
supports_cross_catalog_queries = True
+ # Presto and Trino require OFFSET to appear before LIMIT in the SQL
grammar.
+ # Superset normalizes compiled SQL accordingly so pagination works even
when
+ # the connection resolves to a driver whose dialect emits the ANSI
+ # ``LIMIT ... OFFSET`` ordering (e.g. PyHive). See
+ # ``BaseEngineSpec.apply_offset_before_limit``.
+ offset_before_limit = True
Review Comment:
🔴 **High — `offset_before_limit` leaks into Hive, Spark and Databricks Hive**
`HiveEngineSpec` subclasses `PrestoEngineSpec` (`hive.py:104`), so Hive,
`SparkEngineSpec` and `DatabricksHiveEngineSpec` all inherit
`offset_before_limit = True` from this line. PyHive's Hive dialect emits `LIMIT
x OFFSET y`, so every page-2+ query on these engines gets fully re-rendered by
sqlglot, and the clause order stays the same.
The SQL still changes, though. Through `get_query_str` on `spark://` with a
filter value `C:\users\data`:
- page 1: `` `path` = 'C:\users\data' ``
- page 2: `` `path` = 'C:\\users\\data' ``
So two pages of the same Drill to Detail filter on different values. On Hive
the re-render also drops `/*+ MAPJOIN(...) */` hints (sqlglot logs "Hints are
not supported").
Fix: opt out in `HiveEngineSpec`, next to its existing
`supports_grouping_sets = False` override (`hive.py` isn't in the diff, so no
applyable suggestion):
```python
# Hive/Spark accept the ANSI ``LIMIT ... OFFSET`` order; don't inherit
# the Presto/Trino reordering (and the sqlglot re-render it triggers).
offset_before_limit = False
```
Plus a test that paginated Hive/Spark SQL comes back unchanged.
##########
tests/unit_tests/db_engine_specs/test_trino.py:
##########
@@ -3060,3 +3060,54 @@ def test_handle_cursor_stops_polling_after_time_limit(
assert query.progress == 50
assert mock_time.sleep.call_count == 1
mock_cancel_query.assert_not_called()
+
+
+def test_apply_offset_before_limit_reorders_ansi_ordering() -> None:
+ """
+ Trino requires OFFSET before LIMIT. Some SQLAlchemy drivers (e.g. PyHive)
+ compile paginated queries in the ANSI ``LIMIT ... OFFSET`` order, which
+ Trino rejects with ``mismatched input 'OFFSET'``. The engine spec must
+ normalize such SQL so OFFSET precedes LIMIT.
+ """
+ from superset.db_engine_specs.trino import TrinoEngineSpec
+
+ ansi_sql = "SELECT a, b\nFROM t\nLIMIT 50 OFFSET 50"
+ result = TrinoEngineSpec.apply_offset_before_limit(ansi_sql)
+
+ upper = result.upper()
+ assert "OFFSET" in upper
+ assert "LIMIT" in upper
+ assert upper.rfind("OFFSET") < upper.rfind(
+ "LIMIT"
+ ), f"OFFSET must precede LIMIT for Trino, got: {result!r}"
Review Comment:
🔴 **High — required `pre-commit (current)` check fails on ruff-format**
ruff 0.9.7 (pinned in `requirements/development.txt`) reformats this assert,
and `pre-commit (current)` is a required check in `.asf.yaml`.
```suggestion
assert upper.rfind("OFFSET") < upper.rfind("LIMIT"), (
f"OFFSET must precede LIMIT for Trino, got: {result!r}"
)
```
##########
tests/unit_tests/models/core_test.py:
##########
@@ -2940,3 +2940,51 @@ def
test_connection_does_not_re_exchange_a_refused_refresh_token(
isolated_session.assert_not_called()
start_dance.assert_called_once_with(database)
engine.raw_connection.assert_not_called()
+
+
+def test_compile_sqla_query_offset_before_limit_for_trino(
+ mocker: MockerFixture,
+) -> None:
+ """
+ ``compile_sqla_query`` must emit OFFSET before LIMIT for Trino/Presto even
+ when the underlying SQLAlchemy driver compiles paginated queries in the
+ ANSI ``LIMIT ... OFFSET`` order (as PyHive's Presto/Trino dialects do).
+
+ We drive a Trino ``Database`` but back it with a SQLite engine, whose
+ dialect emits ANSI ordering — standing in for a driver that lacks the
+ OFFSET-before-LIMIT override. The fix must reorder the compiled SQL so it
+ is valid Trino.
+ """
+ from contextlib import contextmanager
+
+ from sqlalchemy import create_engine
+
+ from superset.models.core import Database
+
+ database = Database(
+ database_name="trino_db",
+ sqlalchemy_uri="trino://user@localhost:8080/catalog",
+ )
+ # SQLite compiles `LIMIT x OFFSET y` (ANSI order), mimicking PyHive.
+ ansi_engine = create_engine("sqlite://")
+
+ @contextmanager
+ def _get_engine(catalog=None, schema=None, **kwargs):
+ yield ansi_engine
+
+ mocker.patch.object(database, "get_sqla_engine", new=_get_engine)
+
+ # Sanity: the engine spec resolves to Trino, which requires the reorder.
+ assert database.db_engine_spec.offset_before_limit is True
+
+ tbl = SqlalchemyTable("t", MetaData(), Column("a", Integer))
+ qry = select(tbl.c.a).limit(50).offset(50)
+
+ sql = database.compile_sqla_query(qry)
+
+ upper = sql.upper()
+ assert "OFFSET" in upper
+ assert "LIMIT" in upper
+ assert upper.rfind("OFFSET") < upper.rfind(
+ "LIMIT"
+ ), f"OFFSET must precede LIMIT for Trino, got: {sql!r}"
Review Comment:
🔴 **High — same ruff-format failure as in `test_trino.py`**
```suggestion
assert upper.rfind("OFFSET") < upper.rfind("LIMIT"), (
f"OFFSET must precede LIMIT for Trino, got: {sql!r}"
)
```
##########
superset/db_engine_specs/base.py:
##########
@@ -1810,6 +1820,48 @@ def get_limit_from_sql(cls, sql: str) -> int | None:
# not parseable in sqlglot 30+, which now requires an expression
arg.
return None
+ @classmethod
+ def apply_offset_before_limit(cls, sql: str) -> str:
+ """
+ Reorder a trailing ``LIMIT ... OFFSET ...`` into ``OFFSET ... LIMIT
...``
+ for engines whose grammar requires OFFSET to precede LIMIT.
+
+ This is a no-op unless ``offset_before_limit`` is set (Presto/Trino)
and
+ the compiled SQL is actually in the invalid ANSI order. Some drivers
+ (e.g. PyHive's Presto/Trino dialects) emit ``LIMIT ... OFFSET ...``,
+ which Presto and Trino reject with ``mismatched input 'OFFSET'``.
+ Re-rendering through sqlglot's dialect-aware generator puts the clauses
+ in the order the engine expects, so paginated queries stay valid no
+ matter which SQLAlchemy driver is installed.
+
+ :param sql: A single compiled SQL statement
+ :return: The statement with OFFSET before LIMIT when required
+ """
+ if not cls.offset_before_limit:
+ return sql
+
+ # Cheap textual gate: only re-render when both keywords are present and
+ # the last LIMIT precedes the last OFFSET (the invalid ordering).
+ # Drivers that already emit OFFSET first (e.g. the official ``trino``
+ # package) are left untouched, avoiding a needless reparse/reformat. A
+ # false positive here only triggers a harmless order-preserving
+ # reparse, never wrong SQL.
+ limit_matches = list(re.finditer(r"\bLIMIT\b", sql, re.IGNORECASE))
+ offset_matches = list(re.finditer(r"\bOFFSET\b", sql, re.IGNORECASE))
+ if (
+ not limit_matches
+ or not offset_matches
+ or limit_matches[-1].start() > offset_matches[-1].start()
+ ):
+ return sql
+
+ try:
+ return SQLScript(sql, engine=cls.engine).format()
Review Comment:
🟡 **Medium (non-blocking) — re-rendering the whole statement can silently
change Presto SQL**
When the gate fires on `presto://`, sqlglot regenerates the entire query,
not just the trailing clause. A custom WHERE `price >= DECIMAL '9.99'` comes
out on page 2 as `price >= CAST('9.99' AS DECIMAL)`. Presto/Trino read a bare
`DECIMAL` as `decimal(38,0)`, so page 2 effectively filters `>= 10` while page
1 filters `>= 9.99`. Master leaves user clauses as written for exactly this
reason (`sanitize_clause`, #36113), and the gate comment's "never wrong SQL"
doesn't hold.
A narrower option that avoids the round-trip: make PyHive's
`PrestoCompiler.limit_clause` emit `OFFSET` then `LIMIT` (as the official trino
dialect does), or rewrite only an anchored trailing `LIMIT n OFFSET m`. The
compiler route also fixes the OFFSET-only `LIMIT -1 OFFSET n` case and leaves
Hive/Spark untouched.
--
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]
