[Bartok9]

Summary

• DataFusion unlimited query() and dry_run() strip terminating semicolon/whitespace runs before ctx.query / ctx.dry_run.
• Aligns with the existing limited-query subquery wrap sanitization.

Motivation

Limited queries already strip before wrap. Unlimited/dry_run still forwarded raw client SQL ending in ;, which DataFusion rejects for single-statement execution depending on path. Reuse the connector's terminating-run helper.

Verification

• core/wren/.venv/bin/python -m pytest tests/unit/test_datafusion_semicolon.py -q — all passed
• query("SELECT 1;") without limit sends SELECT 1.

Apache path: core/wren/**.

Summary by CodeRabbit

• Bug Fixes

  • Improved SQL execution by consistently removing trailing semicolons before running statements, including cases without a row limit.
  • Ensured dry-run validation applies the same SQL normalization as normal query execution.

• Tests

  • Updated and expanded unit tests to verify trailing semicolon stripping for both query execution (no limit) and dry-run behavior.

[coderabbitai]

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: 4f357812-6014-4f1d-a247-db536e2202f0

📥 Commits

Reviewing files that changed from the base of the PR and between 4e204af and e9e4a81.

📒 Files selected for processing (2)

• core/wren/src/wren/connector/datafusion.py
• core/wren/tests/unit/test_datafusion_semicolon.py

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

• core/wren/tests/unit/test_datafusion_semicolon.py

---

Walkthrough

DataFusion query and dry-run paths now strip trailing semicolons and whitespace before execution. Unit tests update no-limit query expectations and add coverage for no-limit queries and dry runs.

Changes

DataFusion SQL normalization

Layer / File(s)	Summary
Normalize query and dry-run SQL core/wren/src/wren/connector/datafusion.py, core/wren/tests/unit/test_datafusion_semicolon.py	Query and dry-run calls normalize SQL with _strip_trailing_semicolon; tests verify semicolons are removed before context execution.

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

Possibly related PRs

• Canner/WrenAI#2407: Applies matching semicolon normalization across another connector’s query and dry-run paths.
• Canner/WrenAI#2420: Applies matching trailing-semicolon handling to the Redshift connector.
• Canner/WrenAI#2430: Extends trailing-semicolon normalization across DataFusion query paths.

Poem

A bunny trims the SQL with care,
No trailing semicolons hopping there.
Query and dry-run both run neat,
Tests guard each tidy treat.
“Snip, snip!” says Bunny, “clean and sweet!”

🚥 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
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and accurately summarizes the main DataFusion change: stripping trailing semicolons for unlimited query and dry_run.
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_datafusion_semicolon.py (1)

64-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

test_query_without_limit_strips_trailing_semicolon duplicates test_query_without_limit_is_unwrapped.

Both tests call connector.query("SELECT 1;") and assert sent == "SELECT 1" — identical coverage. Consider consolidating, or differentiate one to cover an additional case (e.g., trailing whitespace + semicolon: "SELECT 1; ").

Additionally, in test_dry_run_strips_trailing_semicolon (Lines 73-74), the if not hasattr(ctx, "dry_run") guard is dead code because ctx is a MagicMock, which auto-creates any attribute on access. The mock can be set unconditionally.

🤖 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_datafusion_semicolon.py` around lines 64 - 68, The
semicolon tests duplicate coverage, and the dry-run mock setup contains an
ineffective attribute guard. Differentiate
test_query_without_limit_strips_trailing_semicolon by covering trailing
whitespace after the semicolon, and update
test_dry_run_strips_trailing_semicolon to assign ctx.dry_run unconditionally
without checking hasattr.

🤖 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_datafusion_semicolon.py`:
- Around line 64-68: The semicolon tests duplicate coverage, and the dry-run
mock setup contains an ineffective attribute guard. Differentiate
test_query_without_limit_strips_trailing_semicolon by covering trailing
whitespace after the semicolon, and update
test_dry_run_strips_trailing_semicolon to assign ctx.dry_run unconditionally
without checking hasattr.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d07a192e-d6e9-4bb2-8b20-12e872c6fba9

📥 Commits

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

📒 Files selected for processing (2)

• core/wren/src/wren/connector/datafusion.py
• core/wren/tests/unit/test_datafusion_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. 🙏

[goldmedal]

@Bartok9, there are some conflicts.

[Bartok9]

Closing as superseded — datafusion unlimited/dry_run paths on main already call strip_trailing_semicolon (shared base helper from the semicolon campaign). Branch is DIRTY; behavior is present upstream.