Fix dropped SELECT clauses and lost FROM-list comma

[gmr]

Summary

Fixes four bugs in the SELECT formatter (src/formatter/select.rs) where whole clauses were silently dropped or a comma was lost, producing invalid SQL. A small reusable helper was extracted in src/formatter/expr.rs.

Problem

• FOR UPDATE / FOR SHARE locking clauses are silently dropped from SELECT #18 — FOR UPDATE / FOR SHARE locking clauses were dropped entirely. SELECT id FROM accounts WHERE active FOR UPDATE; lost FOR UPDATE.
• WINDOW clause is silently dropped, leaving dangling OVER references #19 — The WINDOW clause was dropped, leaving dangling OVER w references with no window definition (invalid output).
• FETCH FIRST ... ROWS ONLY is silently dropped from SELECT #20 — The SQL-standard FETCH FIRST n ROWS ONLY form was dropped: extract_limit_value could not read select_fetch_first_value, so no limit was emitted.
• Comma dropped between plain table and JOIN group in mixed FROM lists, changing semantics #21 — In mixed FROM lists, the comma between a plain table and a JOIN group was dropped (both orders), so FROM t1, t2 JOIN t3 ... rendered as t1 t2 (parsed as t1 AS t2). Affected both river and left-aligned (aweber) styles.

Solution

• FOR UPDATE / FOR SHARE locking clauses are silently dropped from SELECT #18 — Added a for_locking field to SelectClauses, collect for_locking_clause, and render each locking item (OF list, NOWAIT, SKIP LOCKED, NO KEY UPDATE, KEY SHARE) in both renderers.
• WINDOW clause is silently dropped, leaving dangling OVER references #19 — Added a window_clause field, collect and render the named window definitions. Extracted format_window_spec_body from format_over_clause (identical output) so the spec body is formatted consistently.
• FETCH FIRST ... ROWS ONLY is silently dropped from SELECT #20 — The FETCH FIRST form is preserved verbatim (e.g. FETCH FIRST 5 ROWS ONLY) rather than normalized to LIMIT. Rationale: it is what the user wrote, and the existing formatter has no LIMIT/OFFSET normalization, so preserving is the consistent choice. FETCH (5 chars) aligns identically to LIMIT in the river.
• Comma dropped between plain table and JOIN group in mixed FROM lists, changing semantics #21 — Replaced the has_multiple_non_join heuristic with logic that appends a comma after every FROM item except the last, in both river and left-aligned styles.

Tests

Added regression fixtures (auto-discovered by tests/all_fixtures.rs):

• river/for_locking_clause, river/window_named_clause, river/fetch_first_rows_only, river/from_comma_join_mix
• aweber/from_comma_join_mix (Comma dropped between plain table and JOIN group in mixed FROM lists, changing semantics #21 affects both styles)

All 72 fixtures pass; just check (fmt + clippy -D warnings + test) is green.

Fixes #18
Fixes #19
Fixes #20
Fixes #21

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Enhanced SQL formatting for window functions, including OVER with inline window specifications and named window references.
  • Added/improved rendering of WINDOW definitions and row-level locking clauses (FOR UPDATE / FOR SHARE), plus correct FETCH FIRST ... ROWS ONLY output.
  • Improved SELECT formatting consistency for FROM clauses when mixing comma-separated items with explicit JOIN ... ON.

• Tests

  • Added and updated fixtures covering named window usage, locking clauses, FETCH FIRST, and comma/join combinations.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f6afc55c-728d-46a8-8cc6-767131e3686c

📥 Commits

Reviewing files that changed from the base of the PR and between b7c1115 and 82aca22.

📒 Files selected for processing (2)

• src/formatter/expr.rs
• tests/fixtures/river/window_named_clause.expected

🚧 Files skipped from review as they are similar to previous changes (2)

• tests/fixtures/river/window_named_clause.expected
• src/formatter/expr.rs

---

📝 Walkthrough

Walkthrough

This PR preserves WINDOW, FOR UPDATE/FOR SHARE, and FETCH FIRST clauses during SELECT formatting, and fixes comma placement for mixed comma-and-JOIN FROM lists in both river and left-aligned styles. It also adds fixtures covering the updated output.

Changes

Missing SELECT clause preservation fixes

Layer / File(s)	Summary
Window spec body formatting refactor src/formatter/expr.rs, tests/fixtures/river/window_named_clause.*	format_over_clause now distinguishes inline window specs, named window references, and fallback window bodies, and format_window_spec_body returns only the inner contents used by the named-window fixture.
Clause collection and river formatting src/formatter/select.rs, tests/fixtures/river/fetch_first_rows_only.*, tests/fixtures/river/for_locking_clause.*, tests/fixtures/river/from_comma_join_mix.*, tests/fixtures/river/window_named_clause.*	SelectClauses collects WINDOW and locking clauses, river formatting emits them, preserves FETCH FIRST, updates river width keyword tracking, and formats mixed FROM items by position.
Left-aligned formatting and comma placement src/formatter/select.rs, tests/fixtures/aweber/from_comma_join_mix.*	Left-aligned formatting emits WINDOW and locking blocks, preserves FETCH FIRST, and formats mixed FROM lists with position-based commas.
Test fixtures tests/fixtures/river/*, tests/fixtures/aweber/*	New SQL and expected fixtures cover named windows, locking clauses, FETCH FIRST, and mixed comma-plus-JOIN FROM syntax.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• gmr/libpgfmt#4: Both PRs modify src/formatter/expr.rs window clause formatting and OVER rendering.

Poem

I hop through SQL, ears held high,
Where lost clauses now can’t slip by.
WINDOW, FOR, and FETCH first too,
Commas stay put in joins I chew. 🐰

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main fix: preserving dropped SELECT clauses and mixed FROM-list commas.
Linked Issues check	✅ Passed	The changes address #18-#21 by preserving locking clauses, WINDOW clauses, FETCH FIRST, and mixed FROM-list commas.
Out of Scope Changes check	✅ Passed	All code and fixture changes are directly tied to the SELECT formatter fixes described in the linked issues.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

---

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

[gmr]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@tests/fixtures/river/window_named_clause.expected`:
- Around line 1-4: The expected SQL output is rewriting a bare named-window
reference into an inline window specification; update the formatter/fixture for
the ROW_NUMBER() OVER form so the named window reference stays as OVER w instead
of being rendered as OVER (w). Focus on the window-clause handling in the SQL
formatting path that produces this expected file and preserve the bare
named-window syntax when a WINDOW alias like w is referenced.

In `@tests/fixtures/river/window_named_clause.sql`:
- Line 1: The window formatting in format_over_clause is rewriting named window
references like OVER w into OVER (...), which changes the emitted SQL form.
Update the formatter to preserve a bare named window reference when the OVER
clause points directly to a named window, and only parenthesize inline
specifications; use the existing format_over_clause logic to distinguish these
cases so this fixture stays as OVER w, or add a separate frame-clause case if
needed.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a5d42cda-c290-4a94-857f-bffe9c896966

📥 Commits

Reviewing files that changed from the base of the PR and between c949e2b and b7c1115.

📒 Files selected for processing (12)

• src/formatter/expr.rs
• src/formatter/select.rs
• tests/fixtures/aweber/from_comma_join_mix.expected
• tests/fixtures/aweber/from_comma_join_mix.sql
• tests/fixtures/river/fetch_first_rows_only.expected
• tests/fixtures/river/fetch_first_rows_only.sql
• tests/fixtures/river/for_locking_clause.expected
• tests/fixtures/river/for_locking_clause.sql
• tests/fixtures/river/from_comma_join_mix.expected
• tests/fixtures/river/from_comma_join_mix.sql
• tests/fixtures/river/window_named_clause.expected
• tests/fixtures/river/window_named_clause.sql