fix(canner): strip trailing semicolon on unlimited query path - #2488
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCanner query processing now removes trailing semicolons before SQL execution or LIMIT wrapping. New unit tests cover helper behavior and both query paths. ChangesCanner SQL handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/wren/tests/unit/test_canner_semicolon.py (1)
36-42: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPatch
_build_arrow_tableviamonkeypatchorpatchto avoid leaking global state.Both tests directly assign
canner_mod._build_arrow_table = _fake_table(lines 37, 61) without restoring the original afterward. This mutates the module globally and persists for the rest of the test session, potentially breaking other tests that rely on the real function. Usemonkeypatch.setattrorunittest.mock.patchas a context manager/decorator to ensure automatic cleanup.Additionally, the fallback to
_build_pg_arrow_table(lines 38-39, 62-63) is likely dead code —query()calls_build_arrow_table(canner.py line 276).♻️ Proposed refactor using monkeypatch
def test_query_unlimited_strips_trailing_semicolon(monkeypatch): connector = CannerConnector.__new__(CannerConnector) connector.connection = MagicMock() cursor = MagicMock() connector.connection.cursor.return_value.__enter__.return_value = cursor import pyarrow as pa - import wren.connector.canner as canner_mod - - def _fake_table(cur): - import pyarrow as pa - return pa.table({"x": [1]}) - - if hasattr(canner_mod, "_build_arrow_table"): - canner_mod._build_arrow_table = _fake_table # type: ignore - elif hasattr(canner_mod, "_build_pg_arrow_table"): - canner_mod._build_pg_arrow_table = _fake_table # type: ignore - else: - raise AssertionError(dir(canner_mod)[:50]) + monkeypatch.setattr( + "wren.connector.canner._build_arrow_table", + lambda cur: pa.table({"x": [1]}), + ) connector.query("SELECT 1 AS x;") cursor.execute.assert_called_once_with("SELECT 1 AS x")Also applies to: 60-63
🤖 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_canner_semicolon.py` around lines 36 - 42, Update both test setup blocks around the fake table injection to use pytest monkeypatch.setattr or unittest.mock.patch, ensuring the original builder is automatically restored after each test. Target _build_arrow_table directly, remove the fallback assignment to _build_pg_arrow_table and its related hasattr branching, and retain a clear failure if the expected symbol is unavailable.
🤖 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_canner_semicolon.py`:
- Around line 36-42: Update both test setup blocks around the fake table
injection to use pytest monkeypatch.setattr or unittest.mock.patch, ensuring the
original builder is automatically restored after each test. Target
_build_arrow_table directly, remove the fallback assignment to
_build_pg_arrow_table and its related hasattr branching, and retain a clear
failure if the expected symbol is unavailable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 28ae6a08-1e96-4c06-b560-c920d01d6565
📒 Files selected for processing (2)
core/wren/src/wren/connector/canner.pycore/wren/tests/unit/test_canner_semicolon.py
Unlimited query() previously executed raw SQL including a trailing terminator; keep the same strip used before LIMIT subquery wrapping.
Unit-test image lacks psycopg; query() imports it at call time so the two query tests failed with ModuleNotFoundError. Provide a psycopg stub fixture (with errors.QueryCanceled) and switch the _build_arrow_table override to monkeypatch.setattr so module state is restored and the dead _build_pg_arrow_table fallback is dropped (CodeRabbit nit).
7d21a2f to
2290ddf
Compare
CI unit collection failed: test imported private _strip_trailing_semicolon from canner, but canner only reuses the public helper from base.
2 participants
Summary
;in Cannerquery()(limited and unlimited).Motivation
Limited path already stripped before subquery wrap; unlimited pasted
SELECT …;with extra terminator whitespace differed. Align execute path.Verification
;(test fail)pytest tests/unit/test_canner_semicolon.py→ 3 passedcore/**Files
Summary by CodeRabbit