refactor(trino): strip trailing semicolon on unlimited query path for connector consistency

[Bartok9]

Summary

Strip trailing ; on Trino unlimited query() before cursor.execute.

Evidence / framing

Trino's HTTP statement API and trino.dbapi are stricter about statement terminators than most engines; the Trino CLI also strips client-side. Limited queries already went through strip_trailing_semicolon inside the subquery wrap; unlimited passed SQL through raw.

I do not have a live Trino server in CI to paste a server-side error blob here. The unit test locks the SQL we send (mocked execute). If maintainers prefer this as refactor(...) consistency-only until a live error is captured, happy to retitle.

Smaller fixes

• Comment uses trailing ; (not a garbled "/;" typo).
• fix(trino): strip trailing semicolon on unlimited query path #2555 was the earlier same-file attempt (closed); this PR keeps the better mocked-execution test.

Verification

cd core/wren && python -m pytest tests/unit/test_trino_semicolon_unlimited.py -q

Summary by CodeRabbit

• Bug Fixes

  • Fixed Trino query execution for SQL statements ending with semicolons.
  • Ensured limited queries apply the requested row limit correctly after normalization.

• Tests

  • Added coverage for unlimited and limited queries containing trailing semicolons.

[coderabbitai]

Walkthrough

TrinoConnector.query strips trailing semicolons before execution and optional LIMIT wrapping. Unit tests cover unlimited and limited query paths.

Changes

Trino SQL normalization

Layer / File(s)	Summary
Query normalization and regression coverage core/wren/src/wren/connector/trino.py, core/wren/tests/unit/test_trino_semicolon_unlimited.py	TrinoConnector.query normalizes SQL before execution or subquery wrapping. Tests cover unlimited and limited queries.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

• Strip trailing semicolon on the unlimited query() path across connectors #2497 — Covers trailing-semicolon removal for Trino unlimited and limited query paths.

Possibly related PRs

• Canner/WrenAI#2555 — Directly overlaps the Trino query and test changes.

Poem

A rabbit trims the SQL trail,
So Trino queries do not fail.
Limited or unlimited, they run clean,
With tests guarding every scene.
Hop, hop! 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The description covers the change, failure framing, verification command, and duplicate context, although it uses custom section headings.
Title check	✅ Passed	The title clearly identifies the Trino change and the removal of trailing semicolons from the unlimited query path.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[goldmedal]

Code review

Of this series, this is the one that may be a genuine fix rather than a consistency alignment — Trino's statement endpoint is stricter about trailing terminators than most engines, and the CLI strips them client-side. But the PR asserts it without evidence:

WrenAI/core/wren/src/wren/connector/trino.py

Lines 484 to 494 in 0a17c26

	def query(self, sql: str, limit: int | None = None) -> pa.Table:
	trino = _import_trino()
	# Unlimited path must strip trailing "/;" too — pasted tool SQL often
	# ends with a terminator that Trino rejects for single statements.
	sql = strip_trailing_semicolon(sql)
	if limit is not None:
	sql = f"SELECT * FROM ({sql}) AS _sub LIMIT {limit}"
	try:
	with contextlib.closing(self.connection.cursor()) as cursor:
	cursor.execute(sql)

I can't verify it from here without a Trino server, and the mocked test asserts on the SQL we send rather than on what Trino accepts, so it can't confirm the premise either. Please add the actual failure — the error Trino returns for SELECT 1; submitted through trino.dbapi — to the PR description. If it does reproduce, fix(...) is the right prefix and this should be prioritised over the rest of the series. If it doesn't, it belongs in the same bucket as #2595: a refactor(...) for connector consistency.

Two smaller things:

1. The comment says trailing "/;" — looks like a typo for ;.
2. fix(trino): strip trailing semicolon on unlimited query path #2555 is the same change to the same file. Whichever is kept, the other should close; this one's test is the better of the two (mocked execution rather than source-text assertions).

Rebase before merge — 3 commits behind main.

[Bartok9]

@goldmedal thanks — rebased on current main.

• Description now states the Trino-strict-terminator rationale and honestly notes we lock the outbound SQL via mocks (no live Trino in CI for a server error paste).
• Fixed the comment typo.
• Kept this PR over fix(trino): strip trailing semicolon on unlimited query path #2555 (closed) for the stronger test.

Happy to retitle to refactor(...) if you'd rather bucket it with the consistency series until a live capture exists.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

🧹 Nitpick comments (1)

core/wren/tests/unit/test_trino_semicolon_unlimited.py (1)

40-42: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert the complete SQL for the limited path.

The current assertions do not prove that cursor.execute received the intended wrapped query. Assert the exact SQL to protect both semicolon removal and the existing LIMIT wrapping contract.

Proposed test assertion

-    sent = cursor.execute.call_args[0][0]
-    assert "SELECT 1;" not in sent
-    assert "LIMIT 5" in sent
+    cursor.execute.assert_called_once_with(
+        "SELECT * FROM (SELECT 1) AS _sub LIMIT 5"
+    )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/wren/tests/unit/test_trino_semicolon_unlimited.py` around lines 40 - 42,
Update the limited-path assertions in the relevant test to compare the full SQL
passed to cursor.execute against the expected wrapped query, preserving
validation of semicolon removal and the LIMIT 5 contract instead of checking
independent substrings.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@core/wren/tests/unit/test_trino_semicolon_unlimited.py`:
- Around line 40-42: Update the limited-path assertions in the relevant test to
compare the full SQL passed to cursor.execute against the expected wrapped
query, preserving validation of semicolon removal and the LIMIT 5 contract
instead of checking independent substrings.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a6854db7-c36f-4940-9464-c1388266dd32

📥 Commits

Reviewing files that changed from the base of the PR and between 9bdae39 and 9d31ad8.

📒 Files selected for processing (2)

• core/wren/src/wren/connector/trino.py
• core/wren/tests/unit/test_trino_semicolon_unlimited.py

🚧 Files skipped from review as they are similar to previous changes (1)

• core/wren/src/wren/connector/trino.py

[goldmedal]

The code change is fine and now has two merged precedents — #2595 (mysql) and #2633 (mssql), both landed as refactor: for connector consistency. Two things should match them before this goes in.

The label

#2633 is the same change on the same code path, and it was framed as a refactor precisely because there is no user-visible behaviour change. There is none here either: every path into TrinoConnector.query runs through WrenEngine.query → dry_plan → _plan, which returns rewriter.rewrite(sql) — sqlglot-generated SQL — and raises on any failure rather than passing the input through. sqlglot does not emit a statement terminator:

>>> parse_one("SELECT 1 FROM t;", dialect="trino").sql(dialect="trino")
'SELECT 1 FROM t'

So a trailing ; cannot reach the connector by that route. fix: requires a reproducible failure that the change repairs; please retitle to refactor(trino): to match #2595 and #2633.

The comment still asserts Trino behaviour

# Strip terminating `;` for unlimited execute too — Trino's statement
# path is stricter than most engines about bare terminators (CLI also
# strips client-side). Limited path already strips inside the wrap.

"stricter than most engines about bare terminators" is a comparative claim about an external system, and nothing in the PR observes it. You already withdrew the equivalent claim for pyodbc in #2633 ("No claim that pyodbc rejects a single statement with a lone ;") — the same standard applies here. Either paste the actual Trino error, or drop those two sentences and keep the consistency rationale, which stands on its own.

For contrast, note why the existing strip sits inside the limited branch: that path builds SELECT * FROM ({sql}) AS _sub LIMIT {limit}, and a ; inside the parentheses is a grammar error independent of any engine. That is a composition reason, not a Trino reason — worth keeping the two apart in the comment.

Otherwise this looks right, and folding it in with #2593 would have been preferable to one PR per connector.

[Bartok9]

@goldmedal Addressed:

1. Retitled to refactor(trino): … to match refactor(mysql): strip trailing semicolon on unlimited query path for connector consistency #2595 / refactor(mssql): strip trailing semicolon on unlimited query path for connector consistency #2633 (consistency; no claimed user-visible failure without a live Trino capture).
2. Rewrote the unlimited-path comment to drop the unsubstantiated “stricter than most engines” claim; kept composition vs consistency separated.

Ready for another look.

[goldmedal]

Thanks @Bartok9 👍