fix(sqlbuilder): scope set-op branch ORDER BY/LIMIT/OFFSET with parentheses - #46
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughSet operations now scope ChangesSet-operation clause scoping
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR fixes a long-standing bug where set-operation branches (UNION, UNION ALL, INTERSECT, EXCEPT) emitted no parentheses, causing the left branch's ORDER BY / LIMIT / OFFSET to leak onto the enclosing statement and producing invalid SQL with duplicate clauses. It also adds SQL operator-precedence grouping so mixed-kind flat chains like
Confidence Score: 4/5Functionally safe to merge; the two new paren-injection mechanisms are correct in isolation and their interleaved interaction is also correct, but that combined path goes untested. The core logic is sound after tracing all key paths: paren counts balance when both wrapOpen > 0 and leftParen = true, setOpBranchNeedsParens correctly handles the captured-scope case via len(b.setOps) > 0, and the precedence-grouping algorithm produces the right associativity for all chain shapes examined. The only gap is that no test exercises a left branch with ORDER BY/LIMIT combined with a mixed-kind operator chain that triggers wrapClose. Files Needing Attention: The interaction between setOpLeft in select.go and the wrapOpen/wrapClose precedence machinery is the untested path worth a second look before merging.
|
| Filename | Overview |
|---|---|
| sqlbuilder/set_op.go | Adds branchScope struct, setOpBranchNeedsParens, and setOpPrec helpers. Logic is correct: setOpBranchNeedsParens covers the captured-scope case implicitly via len(b.setOps) > 0. |
| sqlbuilder/select.go | Core change: addSetOp, captureSetOpLeftScope, setOpWrapPoints, and updated Build/writeSetOps. The leftParen + wrapOpen paren-balance accounting is correct but the interaction has no test coverage. |
| sqlbuilder/select_test.go | 13 new tests cover setOpLeft and wrapClose independently and combined nested/chain scenarios, but missing a test for setOpLeft + wrapClose together (left branch has ORDER BY + mixed-kind chain). |
| CHANGELOG.md | Adds 0.27.0 entry describing both fixes accurately, including the SQLite limitation note. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["addSetOp(kind, other)"] --> B["captureSetOpLeftScope()"]
B --> C{First set op AND\nhas ORDER BY/LIMIT/OFFSET?}
C -- Yes --> D["Snapshot into setOpLeft\nClear live orderBy/limit/offset"]
C -- No --> E["No-op"]
D --> F["buildSelectPostgres(other)"]
E --> F
F --> G["setOpBranchNeedsParens(other)"]
G --> H{other has ORDER BY\nor LIMIT/OFFSET\nor own setOps?}
H -- Yes --> I["parenWrap = true"]
H -- No --> J["parenWrap = false"]
I --> K["Append setOp{kind, sql, parenWrap}"]
J --> K
K --> L["Build()"]
L --> M["setOpWrapPoints()"]
M --> N{"UNION/EXCEPT to INTERSECT transition?"}
N -- Yes --> O["wrapClose[i]=true, open++"]
N -- No --> P["wrapClose[i]=false"]
O --> Q["Emit leading parens before base branch"]
P --> Q
Q --> R{setOpLeft != nil?}
R -- Yes --> S["Wrap branch in parens with captured ORDER BY/LIMIT/OFFSET"]
R -- No --> T["Write SELECT branch normally"]
S --> U["writeSetOps: close precedence parens at wrapClose points"]
T --> U
Reviews (2): Last reviewed commit: "fix(sqlbuilder): scope and group set ope..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@sqlbuilder/select.go`:
- Around line 447-456: The set-operation chaining flow in SelectBuilder.addSetOp
currently loses grouping when consecutive operations use different setOpKind
values. Add coverage documenting the intended mixed Union/Intersect/Except
behavior, then update addSetOp and related set-operation rendering to either
preserve explicit parentheses around mixed-kind segments or reject cross-kind
chaining consistently; keep same-kind chaining behavior unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7351f8b4-bd42-42e8-8e38-4356c3b0fa74
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!CHANGELOG.md
📒 Files selected for processing (3)
sqlbuilder/select.gosqlbuilder/select_test.gosqlbuilder/set_op.go
Set operations (Union, UnionAll, Intersect, Except) mishandled branch scope and chain grouping, producing invalid or silently wrong SQL. Branch scope: each branch was concatenated with no parentheses, and the left (receiver) branch's ORDER BY/LIMIT/OFFSET leaked onto the enclosing statement. A union whose branches each carried their own ordering/limiting emitted two ORDER BYs and two LIMITs (a syntax error), and the right branch's clauses bound to the whole union instead of that branch. Any branch carrying ORDER BY/LIMIT/OFFSET is now wrapped in parentheses, and the left branch's clauses are snapshotted to that branch when the set op is added; clauses added after the set op still apply to the whole statement. Chain grouping: a compound branch (e.g. a.UnionAll(b.Union(c))) was flattened, silently regrouping it as (a UNION ALL b) UNION c. And because SQL binds INTERSECT tighter than UNION/EXCEPT, mixed-kind flat chains like a.Union(b).Intersect(c) rendered a UNION b INTERSECT c — read by SQL as a UNION (b INTERSECT c) — not the fluent-intended (a UNION b) INTERSECT c. Compound branches are now parenthesized, and the accumulated left segment of a flat chain is parenthesized at each precedence rise. Same-precedence chains (including all same-kind chains) are unchanged and stay flat. These parenthesized compound forms target PostgreSQL/MySQL; SQLite does not accept a parenthesized or per-branch-ordered compound select as a set-op term, so nested/scoped/mixed-kind set ops are not expressible there.
0551ef4 to
dbede9c
Compare
2 participants
Summary
Set operations (Union, UnionAll, Intersect, Except) concatenated each branch with no parentheses and let the left (receiver) branch's ORDER BY/LIMIT/OFFSET leak onto the enclosing statement. A union whose branches each carried their own ordering/limiting produced invalid SQL: two ORDER BYs and two LIMITs, with the right branch's clauses binding to the whole union instead of that branch.
Now any branch carrying ORDER BY/LIMIT/OFFSET is wrapped in parentheses, and the left branch's clauses are snapshotted to that branch when the set op is added. Ordering/limiting applied after the set op still applies to the whole statement. Plain branches, and same-operator nested branches with no ordering, are left unparenthesized (the latter avoids emitting a parenthesized compound select, which SQLite rejects).
Motivation
Fixes #
Changes
Checklist
make all)Summary by CodeRabbit