fix(trino): emit OFFSET before LIMIT for paginated queries

[eschutho]

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:

>>> 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

This is a query-generation fix, so the meaningful before/after is the SQL Superset dispatches for Drill to Detail page 2 (row_limit=50, row_offset=50) when the connection resolves to a driver that emits ANSI ordering (e.g. PyHive). Output below is the actual compiled SQL, reproduced end to end.

Before — invalid for Trino/Presto (fails with line N: mismatched input 'OFFSET'. Expecting: <EOF>):

SELECT "cleaned_sales_data"."order_number", "cleaned_sales_data"."sales"
FROM "cleaned_sales_data"
LIMIT 50 OFFSET 50

After — valid; page 2 loads:

SELECT
  "cleaned_sales_data"."order_number",
  "cleaned_sales_data"."sales"
FROM "cleaned_sales_data"
OFFSET 50
LIMIT 50

_{UI screenshots aren't included because reproducing the failure in the browser requires a live Trino/Presto cluster connected via the PyHive dialect; the SQL above is the exact statement that changes.}

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)
• Introduces new feature or API
• Removes existing feature or API

🤖 Generated with Claude Code

[bito-code-review]

Code Review Agent Run #42f9d6

Actionable Suggestions - 0

Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

• superset/db_engine_specs/base.py - 1

  • Misleading guard-condition comment · Line 1582-1587

Review Details

• Files reviewed - 5 · Commit Range: b3c2be0..b3c2be0

  • superset/db_engine_specs/base.py
  • superset/db_engine_specs/presto.py
  • superset/models/core.py
  • tests/unit_tests/db_engine_specs/test_trino.py
  • tests/unit_tests/models/core_test.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	b3c2be0
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6a7643763eaab6000891c38e
😎 Deploy Preview	https://deploy-preview-42899--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.37%. Comparing base (ae66b69) to head (b3c2be0).
⚠️ Report is 26 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/db_engine_specs/base.py	61.53%	4 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (ae66b69) and HEAD (b3c2be0). Click for more details.

HEAD has 5 uploads less than BASE

Flag	BASE (ae66b69)	HEAD (b3c2be0)
unit	6	1

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #42899      +/-   ##
==========================================
- Coverage   73.55%   66.37%   -7.18%     
==========================================
  Files        1928     2856     +928     
  Lines       83529   161153   +77624     
  Branches    27227    37063    +9836     
==========================================
+ Hits        61439   106969   +45530     
- Misses      22090    52160   +30070     
- Partials        0     2024    +2024     

Flag	Coverage Δ
hive	38.24% <26.66%> (?)
mysql	57.78% <40.00%> (?)
postgres	57.82% <40.00%> (?)
presto	40.20% <53.33%> (?)
python	59.22% <60.00%> (-40.78%)	⬇️
sqlite	57.45% <40.00%> (?)
unit	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[codeant-ai-for-open-source]

Suggestion: The flag is defined on PrestoBaseEngineSpec, but HiveEngineSpec inherits PrestoEngineSpec, and SparkEngineSpec and DatabricksHiveEngineSpec inherit HiveEngineSpec. Consequently, every compiled Hive, Spark, and Databricks Interactive Cluster query with LIMIT and OFFSET is reparsed and regenerated using that engine's sqlglot dialect, even though this change is intended only for Presto and Trino. This can rewrite valid engine-specific SQL or alter function semantics; define the flag only on PrestoEngineSpec and TrinoEngineSpec, or explicitly disable it on the Hive-family descendants. [api mismatch]

Severity Level: Major ⚠️

- ⚠️ Hive pagination queries receive unintended SQL reformatting.
- ⚠️ Spark pagination queries use the shared normalization path.
- ⚠️ Databricks interactive-cluster SQL is reparsed unnecessarily.

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** superset/db_engine_specs/presto.py
**Line:** 170:175
**Comment:**
	*Api Mismatch: The flag is defined on `PrestoBaseEngineSpec`, but `HiveEngineSpec` inherits `PrestoEngineSpec`, and `SparkEngineSpec` and `DatabricksHiveEngineSpec` inherit `HiveEngineSpec`. Consequently, every compiled Hive, Spark, and Databricks Interactive Cluster query with `LIMIT` and `OFFSET` is reparsed and regenerated using that engine's sqlglot dialect, even though this change is intended only for Presto and Trino. This can rewrite valid engine-specific SQL or alter function semantics; define the flag only on `PrestoEngineSpec` and `TrinoEngineSpec`, or explicitly disable it on the Hive-family descendants.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

[bito-code-review]

The flagged issue is correct. The offset_before_limit flag is currently defined on PrestoBaseEngineSpec, which is inherited by HiveEngineSpec and its descendants (SparkEngineSpec, DatabricksHiveEngineSpec). This causes unnecessary SQL re-parsing for these engines, which do not require the OFFSET before LIMIT reordering.

To resolve this, you should move the flag definition from PrestoBaseEngineSpec to PrestoEngineSpec and TrinoEngineSpec specifically, or explicitly set offset_before_limit = False in HiveEngineSpec.

Proposed Fix

1. In superset/db_engine_specs/presto.py: Remove the flag from PrestoBaseEngineSpec and add it to the specific engine classes.
2. In superset/db_engine_specs/hive.py: Explicitly set offset_before_limit = False to ensure descendants inherit the correct behavior.

Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well?

superset/db_engine_specs/presto.py

class PrestoBaseEngineSpec(BaseEngineSpec, metaclass=ABCMeta):
    # ...
    # Remove offset_before_limit = True from here

class PrestoEngineSpec(PrestoBaseEngineSpec):
    offset_before_limit = True

class TrinoEngineSpec(PrestoBaseEngineSpec):
    offset_before_limit = True