fix(databricks): strip trailing semicolon before query execute

[Bartok9]

Summary

• Apply _strip_trailing_semicolon on the Databricks query path before cursor.execute, matching dry_run.
• Regression test asserts SELECT 1;; reaches the driver as SELECT 1.

Motivation

Asymmetry with dry_run left multi-terminator SQL only partially hardened.

Apache-2.0 path: core/wren/**.

Verification

.....                                                                    [100%]
5 passed in 0.17s

Duplicates

No open PR for query-path strip (prior #2422 was dry_run only).

Summary by CodeRabbit

• Bug Fixes
  • Fixed Databricks query execution when user SQL includes trailing semicolons and surrounding whitespace.
  • Queries now remove trailing semicolons before being sent for execution (matching existing dry-run behavior).
• Tests
  • Added a unit test to confirm trailing semicolons are stripped prior to calling cursor execution.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5ff16404-2a34-454f-b68c-e321152e28d1

📥 Commits

Reviewing files that changed from the base of the PR and between 3e3fa32 and eead988.

📒 Files selected for processing (1)

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

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

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

---

Walkthrough

Databricks query execution now strips trailing semicolons and whitespace before calling cursor.execute. A unit test verifies that SELECT 1;; is executed as SELECT 1.

Changes

Databricks SQL handling

Layer / File(s)	Summary
Normalize query SQL before execution core/wren/src/wren/connector/databricks.py, core/wren/tests/unit/test_databricks_semicolon.py	DatabricksConnector.query() normalizes SQL before execution, with a mocked-cursor test asserting the stripped statement.

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

Possibly related PRs

• Canner/WrenAI#2420: Adds related trailing-semicolon preprocessing for connector SQL execution and subquery wrapping.
• Canner/WrenAI#2488: Applies equivalent semicolon stripping in another connector’s query() path with focused tests.
• Canner/WrenAI#2489: Adds matching query normalization and execution assertions.

Suggested reviewers: goldmedal

Poem

A bunny trimmed SQL’s tail,
So trailing semicolons fail.
SELECT 1 runs bright,
Tests hop into sight,
Clean queries now prevail.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: stripping trailing semicolons before Databricks query execution.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✨ 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.}

[coderabbitai]

🧹 Nitpick comments (1)

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

48-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove unused variable and simplify exception handling.

mconn_cursor = cursor (line 53) is assigned but never referenced. The try/except block (lines 55-59) is also unnecessary — cursor.fetchall_arrow.return_value = None causes query to return None, not raise. If a real error occurred, the bare except would swallow it, making the test silently pass when it should fail. Removing both clarifies intent and ensures real failures surface.

♻️ Proposed cleanup

 def test_query_strips_trailing_semicolon_before_execute() -> None:
     connector, cursor = _make_mock_connector()
     cursor.fetchall_arrow.return_value = None
     cursor.fetchmany_arrow.return_value = None
-    # Anything truthy so limit branch not needed
-    mconn_cursor = cursor
-    # execute should observe stripped SQL - fetchall path when limit None
-    try:
-        connector.query("SELECT 1;;")
-    except Exception:
-        # mock may return None from fetchall_arrow — only care about execute SQL
-        pass
+    connector.query("SELECT 1;;")
     (sent,), _ = cursor.execute.call_args
     assert sent == "SELECT 1"

🤖 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_databricks_semicolon.py` around lines 48 - 61,
Remove the unused mconn_cursor assignment from
test_query_strips_trailing_semicolon_before_execute and call
connector.query("SELECT 1;;") directly without a try/except block, allowing
genuine failures to surface before asserting the SQL passed to cursor.execute.

🤖 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_databricks_semicolon.py`:
- Around line 48-61: Remove the unused mconn_cursor assignment from
test_query_strips_trailing_semicolon_before_execute and call
connector.query("SELECT 1;;") directly without a try/except block, allowing
genuine failures to surface before asserting the SQL passed to cursor.execute.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 277bc968-d2e5-4d2a-93c5-bcf0bec58269

📥 Commits

Reviewing files that changed from the base of the PR and between bdaefa9 and a5ca95f.

📒 Files selected for processing (2)

• core/wren/src/wren/connector/databricks.py
• core/wren/tests/unit/test_databricks_semicolon.py

[goldmedal]

The shared CI failure here (maturin failed → cannot update the lock file core/wren-core-py/Cargo.lock because --locked was passed) came from the Rust core 0.2.0 bump in #2467 leaving core/wren-core-py/Cargo.lock out of sync. That's now fixed on main by #2478 — please rebase onto latest main to pick up the lockfile fix and turn CI green. 🙏

[Bartok9]

Done — rebased onto latest main to pick up the core/wren-core-py/Cargo.lock fix from #2478. CI should go green now. Thanks @goldmedal! 🙏

[Bartok9]

Thanks @goldmedal! Rebased onto latest main (which now includes #2478's core/wren-core-py/Cargo.lock fix from #2467). CI should be green now — appreciate the heads-up on the shared maturin --locked failure.