fix(duckdb): coerce LIMIT before SQL interpolation - #2637
Conversation
Validate limit before embedding it in the subquery LIMIT wrap; reject negatives and non-numeric strings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughDuckDB limit handling now accepts numeric strings and validates all limits before SQL interpolation. Invalid, fractional, negative, and boolean values raise ChangesDuckDB limit validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/wren/tests/unit/test_duckdb_coerce_limit.py (1)
14-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the unlimited query path.
The helper preserves
None, andqueryhas a separate unlimited branch. Add tests for_coerce_limit(None)andquery(..., limit=None)to confirm that the trailing semicolon is stripped and noLIMITclause is added.🤖 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_duckdb_coerce_limit.py` around lines 14 - 33, Add coverage for the unlimited path using _coerce_limit and DuckDBConnector.query: assert _coerce_limit(None) preserves None, then invoke query with limit=None and verify the executed SQL has its trailing semicolon removed and contains no LIMIT clause.core/wren/src/wren/connector/duckdb.py (1)
16-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the shared
queryannotation with the accepted limit contract.
DuckDBConnector.query()and the test call_coerce_limit(...)with strings, but the abstractConnectorABC.query()contract still declareslimit: int | None. Make the DuckDB helper,DuckDBConnector.query(), and the shared abstract contract useint | str | Noneso callers, tests, and implementations agree.🤖 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/src/wren/connector/duckdb.py` around lines 16 - 20, Update the limit type annotation consistently across _coerce_limit, DuckDBConnector.query(), and the shared ConnectorABC.query() contract from int | None to int | str | None, preserving the existing coercion behavior and implementation signatures.
🤖 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.
Inline comments:
In `@core/wren/src/wren/connector/duckdb.py`:
- Around line 20-22: Update _coerce_limit to reject non-integral numeric inputs
before calling int(), ensuring values such as -0.5 cannot become zero; retain
the non-negative validation for integer-like values. Update the function’s
type/contract comment to document the accepted inputs and add a regression test
covering fractional negative limits.
---
Nitpick comments:
In `@core/wren/src/wren/connector/duckdb.py`:
- Around line 16-20: Update the limit type annotation consistently across
_coerce_limit, DuckDBConnector.query(), and the shared ConnectorABC.query()
contract from int | None to int | str | None, preserving the existing coercion
behavior and implementation signatures.
In `@core/wren/tests/unit/test_duckdb_coerce_limit.py`:
- Around line 14-33: Add coverage for the unlimited path using _coerce_limit and
DuckDBConnector.query: assert _coerce_limit(None) preserves None, then invoke
query with limit=None and verify the executed SQL has its trailing semicolon
removed and contains no LIMIT clause.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b14bebe-a2c7-448c-85d3-0156b2e73854
📒 Files selected for processing (2)
core/wren/src/wren/connector/duckdb.pycore/wren/tests/unit/test_duckdb_coerce_limit.py
…ted path - _coerce_limit rejects non-integral floats (e.g. -0.5) and bools - annotate limit as int|str|None across helper, query, and ConnectorABC - add tests for None passthrough and unlimited query semicolon strip
|
Closing in favour of a single consolidated change — thanks for the work, the underlying tidy-up is worth doing, just not as one PR per connector. Why this is being closed rather than reviewed:
On the
What remains is genuine but smaller: a negative limit currently surfaces as a driver-level error instead of a clear What we would take instead — a single PR,
#2624 is the natural home for that; it is being kept open with a note to that effect. On this PR specifically — two extra reasons beyond the shared ones. It edits |
|
Thanks @goldmedal — agreed on consolidating. Closing as one PR per connector was the wrong shape given #2602 and the diverging I'll pivot the strict bits from this PR (reject Appreciate the detailed write-up. |
2 participants
Summary
DuckDB wraps user SQL with
LIMIT {int(limit)}without rejecting negatives. Add_coerce_limit+ unit tests.Test plan
cd core/wren && .venv/bin/python -m pytest tests/unit/test_duckdb_coerce_limit.py -q(3 passed)Summary by CodeRabbit
Bug Fixes
Tests