fix(trino): strip trailing semicolon on unlimited query path - #2555
fix(trino): strip trailing semicolon on unlimited query path#2555Bartok9 wants to merge 1 commit into
Conversation
Always strip before LIMIT branch so bare execute matches dry_run and postgres/canner composition for client-pasted statement terminators.
Walkthrough
ChangesTrino semicolon 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://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/wren/tests/unit/test_trino_unlimited_semicolon.py (2)
28-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSkip test on import failure instead of testing a dummy implementation.
If the actual
strip_trailing_semicoloncannot be imported due to missing dependencies, defining a fallback regex here means the test is verifying its own local mock rather than the real application code. If the upstream regex inbase.pychanges in the future, this test will continue to blindly pass in fallback environments.Consider using a test-skipping mechanism rather than maintaining a duplicate implementation.
♻️ Proposed refactor to skip the test
except ModuleNotFoundError: - # Fallback: pure copy of the regex used by strip_trailing_semicolon. - trailing = re.compile(r"[;\s]+\Z") - - def _strip(sql: str) -> str: - return trailing.sub("", sql) - - return _strip + import pytest + pytest.skip("Missing dependencies to load base.py")🤖 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_trino_unlimited_semicolon.py` around lines 28 - 35, Replace the fallback implementation in the import setup for strip_trailing_semicolon with the test framework’s module-skip mechanism when ModuleNotFoundError occurs. Remove the local trailing regex and _strip helper so the tests only exercise the real application function when its dependencies are available.
44-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
astmodule to verify source code structure.Relying on exact string matching (
source.index(...)) to assert the order of operations is highly brittle. The test will break on any routine code formatting changes, such as moving function arguments to new lines or slight whitespace modifications.Instead of parsing raw text, you can use the standard library
astmodule to safely analyze the structural syntax tree of the file without needing to import the actual dependencies.♻️ Proposed refactor to use `ast` for structural testing
-def test_query_method_always_strips_before_limit_branch(): - source = TRINO_PATH.read_text(encoding="utf-8") - start = source.index("def query(self, sql: str, limit: int | None = None)") - end = source.index("def dry_run(self, sql: str)", start) - body = source[start:end] - assert "sql = strip_trailing_semicolon(sql)" in body - assert "SELECT * FROM ({sql}) AS _sub LIMIT {limit}" in body - strip_at = body.index("sql = strip_trailing_semicolon(sql)") - limit_at = body.index("if limit is not None:") - assert strip_at < limit_at +def test_query_method_always_strips_before_limit_branch(): + import ast + source = TRINO_PATH.read_text(encoding="utf-8") + tree = ast.parse(source) + + class_node = next(n for n in tree.body if isinstance(n, ast.ClassDef) and n.name == "TrinoConnector") + query_node = next(n for n in class_node.body if isinstance(n, ast.FunctionDef) and n.name == "query") + + strip_idx = limit_idx = -1 + for i, stmt in enumerate(query_node.body): + if isinstance(stmt, ast.Assign) and isinstance(stmt.value, ast.Call) and getattr(stmt.value.func, "id", "") == "strip_trailing_semicolon": + strip_idx = i + elif isinstance(stmt, ast.If) and isinstance(stmt.test, ast.Compare): + limit_idx = i + + assert strip_idx != -1, "strip_trailing_semicolon assignment not found" + assert limit_idx != -1, "limit check if-statement not found" + assert strip_idx < limit_idx, "strip_trailing_semicolon must occur before limit branch"🤖 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_trino_unlimited_semicolon.py` around lines 44 - 53, Refactor test_query_method_always_strips_before_limit_branch to parse TRINO_PATH source with Python’s ast module and inspect the query method’s statement structure. Locate the query function and verify that its body contains the strip_trailing_semicolon assignment before the limit-condition branch, while preserving the existing assertions for the limited-query wrapper behavior without relying on exact source-text positions.
🤖 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_trino_unlimited_semicolon.py`:
- Around line 28-35: Replace the fallback implementation in the import setup for
strip_trailing_semicolon with the test framework’s module-skip mechanism when
ModuleNotFoundError occurs. Remove the local trailing regex and _strip helper so
the tests only exercise the real application function when its dependencies are
available.
- Around line 44-53: Refactor
test_query_method_always_strips_before_limit_branch to parse TRINO_PATH source
with Python’s ast module and inspect the query method’s statement structure.
Locate the query function and verify that its body contains the
strip_trailing_semicolon assignment before the limit-condition branch, while
preserving the existing assertions for the limited-query wrapper behavior
without relying on exact source-text positions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f6ff3356-6cd4-427d-833a-332b89f67284
📒 Files selected for processing (2)
core/wren/src/wren/connector/trino.pycore/wren/tests/unit/test_trino_unlimited_semicolon.py
|
The fix itself is the most valuable one in this group — Trino's client protocol rejects a trailing statement terminator, so the unlimited path really is broken today, and the change matches what already landed for Athena in #2535. But I can't merge it with this test.
Please replace it with a mock-cursor test in the style of the Athena one that landed with #2535 ( |
Code reviewDuplicate of #2592 — same file, same behaviour change, opened six days apart. I'd suggest keeping #2592 and closing this one, on test quality. This PR's test reads WrenAI/core/wren/tests/unit/test_trino_unlimited_semicolon.py Lines 1 to 20 in c92c003 The connector is never executed, so the test passes independently of whether the code path works, and it breaks on reformatting. #2592 uses a mocked Trino module and asserts on the SQL actually handed to the cursor, which is what we want. Both also need a rebase — this branch is 17 commits behind |
2 participants
Summary
Always strip trailing statement terminators in TrinoConnector.query before the LIMIT branch so unlimited "SELECT 1;" pastes match dry_run and postgres/canner.
Motivation
LIMIT and dry_run already called strip_trailing_semicolon. Unlimited execute used the raw SQL string only.
Real behavior proof
no tests ran in 0.00s
Testустрои
Summary by CodeRabbit