Skip to content

fix(wren): multiline subquery wrap to prevent trailing comment swallowing (#2733) - #2754

Merged
goldmedal merged 2 commits into
Canner:mainfrom
FrancescoCastaldi:fix/subquery-wrap-line-comment
Oct 7, 2026
Merged

goldmedal merged 2 commits into
Canner:mainfrom
FrancescoCastaldi:fix/subquery-wrap-line-comment

Conversation

@FrancescoCastaldi

@FrancescoCastaldi FrancescoCastaldi commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Closes #2733.

When user SQL ends in a --\ line comment, wrapping the SQL in a single-line subquery (\SELECT * FROM ({sql}) AS ...) causes the closing parenthesis, table alias, and LIMIT clause to be swallowed by the line comment.

This was previously fixed for \postgres\ (#2728), \oracle,
edshift, and \ rino\ (#2736), and \�igquery\ / \duckdb. This PR finishes the remaining connectors identified in #2733:

  • \canner.py\ (\query\ and \dry_run)
  • \clickhouse.py\ (\query\ and \dry_run)
  • \databricks.py\ (\dry_run)
  • \datafusion.py\ (\query)

Changes

  • Updated \canner.py, \clickhouse.py, \databricks.py, and \datafusion.py\ to use multiline wrapping (\SELECT * FROM (\n{sql}\n) AS ...).
  • Updated existing unit test assertions to match multiline wrap.
  • Added test cases verifying query/dry_run survive trailing line comments in:
    • \ est_canner_semicolon.py\
    • \ est_clickhouse_helpers.py\
    • \ est_databricks_semicolon.py\
    • \ est_datafusion_semicolon.py\

Verification

  • Ran .venv\Scripts\python -m pytest\ on all affected unit tests: 75/75 passed.
  • Ran
    uff check\ and
    uff format --check: all clean.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed query execution and dry-run errors caused by trailing line comments when a row limit is applied.
    • Kept generated query wrappers valid across supported connectors while preserving trailing comments.
  • Tests
    • Added coverage for trailing comments in limited queries and dry runs across multiple connectors.

@github-actions github-actions Bot added python Pull requests that update Python code core labels Sep 21, 2026
@coderabbitai

coderabbitai Bot commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: f42e003e-884b-4084-8aac-46d63b18c8ac
📥 Commits

Reviewing files that changed from the base of the PR and between 5492391 and 0701e30.

📒 Files selected for processing (1)
  • core/wren/src/wren/connector/databricks.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

Canner, ClickHouse, Databricks, and DataFusion now place inner SQL on separate lines in the affected subquery wrappers. Tests verify the formatting and cover trailing line comments in limited-query and dry-run paths.

Changes

SQL wrapper fix

Layer / File(s) Summary
Connector wrapper formatting
core/wren/src/wren/connector/canner.py, clickhouse.py, databricks.py, datafusion.py
The affected query and dry-run paths place inner SQL on separate lines inside subquery wrappers. Existing limits and execution behavior remain unchanged.
Wrapper regression coverage
core/wren/tests/unit/test_canner_semicolon.py, test_clickhouse_helpers.py, test_databricks_semicolon.py, test_datafusion_semicolon.py
Tests update expected wrapper formatting and cover trailing line comments in limited-query and dry-run paths. ClickHouse tests also reformat two URL strings. Other test edits reorder imports and remove a stale mock-cursor line.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 0701e

The affected wrappers separate trailing line comments from closing syntax, and the regression tests cover those cases. No actionable merge risk remains in the reviewed changes.

🚥 Pre-merge checks | ✅ 2 | ❌ 1 | ❓ 2

❌ Failed checks (1 warning, 2 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 8 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Linked Issues check ❓ Inconclusive The linked issue assessment could not be completed. Retry the assessment when the required review evidence is available.
Out of Scope Changes check ❓ Inconclusive The linked issue assessment could not be completed. Retry the assessment when the required review evidence is available.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the multiline subquery change and the trailing-comment problem it fixes.
Description check ✅ Passed The description covers the change, affected connectors, tests, and verification. It explains the failure but does not include the required reproduction and actual error output, and it omits the duplic…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

A rabbit checks the SQL line,
And moves the wrap to make it fine.
A comment rests, then ends in place,
The closing bracket keeps its space.
Limits wait beyond the line,
While tests confirm the new design.

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

@goldmedal

Copy link
Copy Markdown
Collaborator

@FrancescoCastaldi, thanks for working on this. There are some conflicts. Could you resolve them?

@FrancescoCastaldi
FrancescoCastaldi force-pushed the fix/subquery-wrap-line-comment branch from 85eee1d to 5492391 Compare October 6, 2026 07:42
@FrancescoCastaldi

Copy link
Copy Markdown
Contributor Author

@goldmedal Rebased on latest main and resolved the conflicts in canner.py and databricks.py. All 75 connector unit tests are passing cleanly.

@goldmedal goldmedal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @FrancescoCastaldi 👍

@goldmedal
goldmedal merged commit eab640c into Canner:main Oct 7, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LIMIT pushdown and dry-run subquery wraps break on SQL ending in a line comment (9 connectors)

2 participants