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

[Bartok9]

Summary

Strip trailing ; on Snowflake unlimited query() so it matches dry_run and other connectors (see merged #2595 mysql).

Motivation

Honest framing: this is connector consistency, not a proven snowflake-connector-python rejection of SELECT 1;. Limited path already stripped inside the subquery wrap; unlimited did not.

Review follow-through (@goldmedal)

1. Restored LIMIT pushdown / subquery / ORDER BY rationale comments (kept alongside the strip).
2. Softened rationale to alignment with dry_run + other connectors; retitled refactor(...).
3. Preserved coerce_limit from main.
4. Kept unit test for unlimited trailing-;.

Verification

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

Summary by CodeRabbit

• Bug Fixes

  • Improved Snowflake query execution by correctly handling SQL statements with trailing semicolons.
  • Ensured limited queries continue to apply limits reliably.

• Tests

  • Added coverage confirming semicolons are removed before executing unrestricted queries.

[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

Walkthrough

Snowflake query execution now strips trailing semicolons before limited and unlimited execution. The limit wrapper uses the normalized SQL, and a unit test verifies the unlimited execution statement.

Changes

Snowflake query execution

Layer / File(s)	Summary
Query preprocessing and validation core/wren/src/wren/connector/snowflake.py, core/wren/tests/unit/test_snowflake_limit_pushdown.py	SnowflakeConnector.query strips trailing semicolons before branching on limit, reuses the normalized SQL in the limit wrapper, and tests the unlimited execution path.

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

Possibly related issues

• Strip trailing semicolon on the unlimited query() path across connectors #2497 — Covers Snowflake trailing-semicolon removal on the unlimited query() path.

Possibly related PRs

• Canner/WrenAI#2456 — Updates Snowflake query semicolon stripping and limit handling.
• Canner/WrenAI#2556 — Modifies SnowflakeConnector.query to normalize SQL before execution.
• Canner/WrenAI#2592 — Applies the same unlimited-path semicolon normalization pattern.

Suggested reviewers: goldmedal

Poem

A rabbit trims SQL with care,
Snowflake receives cleaner queries there.
Limited paths wrap the stream,
Tests protect the execution scheme.
Hop, hop—semicolons disappear!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the Snowflake behavior change and its purpose of stripping trailing semicolons on unlimited queries.
Description check	✅ Passed	The description explains the change, motivation, review follow-through, and test command; the duplicate-check section is not explicit but the content is mostly complete.
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.}

[goldmedal]

Code review

Two issues.

1. This deletes a comment that explains a non-obvious design decision.

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

Lines 55 to 66 in 5d2637c

	self.connection = make_snowflake_connection(connection_info)
	def query(self, sql: str, limit: int | None = None) -> pa.Table:
	# Push LIMIT into Snowflake when requested so we do not download a
	# full result set only to slice it in Python. Wrap as a subquery so a
	# trailing semicolon in the user SQL cannot break composition, and so
	# statements that already contain an ORDER BY keep their ordering
	# under the outer LIMIT.
	executed = sql
	if limit is not None:
	# Place the user SQL on its own line so a trailing line comment
	# (`-- ...`) cannot swallow the closing paren, alias, or LIMIT.

The removed text records why the limited path wraps in a subquery — LIMIT pushdown to avoid downloading a full result set, and preserving an existing ORDER BY under the outer LIMIT. None of that is inferable from the code, and the replacement comment is about semicolons instead. The subquery wrap is still there after this PR, so its rationale should be too. Please keep both comments rather than swapping one for the other.

2. The new claim is unsubstantiated. "Snowflake rejects bare trailing terminators in some clients" — which clients, and under what conditions? snowflake-connector-python executes SELECT 1; without complaint as a single statement. If there's a real case, cite it; if the honest reason is alignment with bigquery, duckdb, redshift and mysql (#2595), say that and retitle to refactor(...), as #2595 was.

Also: #2556 is the same change to the same file. One of the two should close — this one's test is the better of the pair.

Rebase before merge — 3 commits behind main.

[Bartok9]

Rebased onto current main and addressed review:

1. Restored the LIMIT pushdown / subquery / ORDER BY rationale comments (kept alongside the unlimited ; strip).
2. Softened the semicolon rationale to alignment with dry_run + other connectors (not an unsubstantiated client claim).
3. Preserved coerce_limit from main.
4. Kept the unlimited trailing-; unit test.

Ready for another look.

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

[Bartok9]

@goldmedal rebased on current main and addressed your review:

1. Kept the LIMIT pushdown / subquery / ORDER BY comments (not swapped away).
2. Retitled to refactor(...) — consistency with dry_run + refactor(mysql): strip trailing semicolon on unlimited query path for connector consistency #2595-style connectors, not an unsubstantiated client claim.
3. Preserved coerce_limit.
4. Unlimited ; unit test retained.

Earlier CI reds on this branch looked like GitHub Actions “Service Unavailable” fetching actions, not test assertion failures — fresh push should re-run cleanly.

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

[goldmedal]

This one now matches the merged precedents — #2595 (mysql) and #2633 (mssql) — and the retitle to refactor(...) is the right call: there is no user-visible behaviour change, since every path into SnowflakeConnector.query goes through WrenEngine.query → dry_plan → _plan, whose output is sqlglot-generated and carries no terminator.

One leftover to tidy. The pre-existing comment above executed still reads:

Wrap as a subquery so a trailing semicolon in the user SQL cannot break composition, and so statements that already contain an ORDER BY keep their ordering under the outer LIMIT.

After this change the strip happens before the wrap, so the first half of that rationale no longer describes why the wrap exists — the semicolon is already gone by then. The ORDER BY half is still accurate. Please update that sentence so the comment matches the new ordering rather than leaving a documented rationale that the code has moved past.

With that, this is good to go from my side.

[Bartok9]

@goldmedal Updated the LIMIT-wrap comment so it no longer cites trailing-semicolon composition (strip already runs first). Kept the ORDER BY / pushdown rationale. Ready for another look.