Fix #480: render Oracle LIMIT as OFFSET ... ROWS FETCH FIRST ... ROWS ONLY

[geoHeil]

Fixes #480.

Problem

GeneratorConfig::limit_fetch_style was never read by the generator, and Oracle's LIMIT → FETCH FIRST conversion existed only in the transpile normalization pass, and only for the top-level SELECT. As a result Oracle output kept LIMIT (rejected with ORA-03049) for:

• ASTs built with the builder or generated directly with Generator (the repro in the issue)
• subqueries, CTEs, IN (SELECT ...), INSERT ... SELECT
• UNION / INTERSECT / EXCEPT
• T-SQL TOP n sources

LIMIT ALL / LIMIT NULL were also turned into the invalid FETCH FIRST ALL ROWS ONLY.

Fix

• Oracle's generator config now sets limit_fetch_style: LimitFetchStyle::FetchFirst.

• The generator honors FetchFirst everywhere a limit is rendered: SELECT, set operations, subquery modifiers, and standalone Limit nodes. It emits [OFFSET n ROWS] FETCH FIRST m [PERCENT] ROWS ONLY, puts OFFSET first, and adds its mandatory ROWS keyword.

• No-op limits (ALL / NULL) are dropped.

• T-SQL TOP n [PERCENT] [WITH TIES] maps to the equivalent FETCH FIRST ... ROWS {ONLY | WITH TIES}.

• The top-level-only rewrite in normalization/statements.rs is removed, so the generator is the single source of truth.

• T-SQL's limit_fetch_style changes from FetchFirst to Top, which describes what it already does. T-SQL has dedicated TOP / OFFSET-FETCH handling, so its output is unchanged.

• Set-operation branches keep their own limits. A bare SELECT operand whose row limit renders as a trailing clause (e.g. T-SQL SELECT TOP 5 ... becoming LIMIT 5 / FETCH FIRST 5 ROWS ONLY) is parenthesized, so the limit no longer applies to the whole UNION / INTERSECT / EXCEPT. This also fixes the same bug for LIMIT targets such as DuckDB, Postgres and Trino. SQLite rejects parenthesized operands, so it gets SELECT * FROM (...). ClickHouse already binds branch LIMITs locally and is left unchanged. Operands wrapped in comments (Expression::Annotated, e.g. a comment before UNION ALL) are grouped the same way, with the comment written after the closing parenthesis.

• Config precedence. Setting limit_fetch_style: FetchFirst now overrides dialect-specific LIMIT/OFFSET rendering (for example, Presto/Trino OFFSET n LIMIT m). T-SQL/Fabric are the exception: they keep their own TOP / OFFSET ... FETCH NEXT output, since FETCH requires ORDER BY there. This removes the duplicated FETCH NEXT ... FETCH FIRST ... output.

• Standalone Limit(ALL) / Limit(NULL) nodes emit nothing in FetchFirst style.

Because the setting now does something, a custom GeneratorConfig { limit_fetch_style: FetchFirst, .. } also works for any target except T-SQL/Fabric.

Before / after (Oracle)

Input	Before	After
builder::from("t").select_cols(["a"]).limit(5)	SELECT a FROM t LIMIT 5	SELECT a FROM t FETCH FIRST 5 ROWS ONLY
SELECT * FROM (SELECT a FROM t LIMIT 5) AS s	... (SELECT a FROM t LIMIT 5) s	... (SELECT a FROM t FETCH FIRST 5 ROWS ONLY) s
SELECT a FROM t UNION ALL SELECT b FROM u ORDER BY 1 LIMIT 5 OFFSET 2	... LIMIT 5 OFFSET 2	... ORDER BY 1 OFFSET 2 ROWS FETCH FIRST 5 ROWS ONLY
SELECT a FROM t LIMIT ALL	SELECT a FROM t FETCH FIRST ALL ROWS ONLY	SELECT a FROM t
T-SQL SELECT TOP 10 PERCENT a FROM t	SELECT TOP 10 PERCENT a FROM t	SELECT a FROM t FETCH FIRST 10 PERCENT ROWS ONLY
T-SQL SELECT a FROM t UNION ALL SELECT TOP 5 a FROM u	... UNION ALL SELECT a FROM u LIMIT 5	... UNION ALL (SELECT a FROM u FETCH FIRST 5 ROWS ONLY)

Tests

• Direct-generator and config cases are generator unit tests in src/generator.rs (test_fetch_first_style_*). They cover the builder, pretty output, no-op limits, and precedence for T-SQL, Fabric, Presto, Trino and Postgres.
• Cross-dialect cases are in tests/dialect_matrix.rs (oracle_row_limit_regressions), which CI already runs. They cover nested queries; set operations with outer ORDER BY / LIMIT / OFFSET; branch-local TOP on either side of UNION, INTERSECT and EXCEPT for Oracle, DuckDB, T-SQL identity and SQLite; comment-annotated operands (block and line comments) for DuckDB, SQLite and Oracle, plus a generator unit test with annotated AST operands on either side; TOP → FETCH; Oracle FETCH identity; and unchanged output for LIMIT dialects. The separate oracle_limit_regression.rs has been removed.
• Passing locally: lib unit tests, deep_nesting_regression, dialect_matrix, all sqlglot_* fixture suites (identity, dialect identity, transpilation, transpile, parser, pretty), custom_dialect_tests, custom_clickhouse_parser, custom_clickhouse_coverage. cargo fmt --all -- --check is clean.

Not in scope, and unchanged from before this PR: PERCENT on set-operation limits is still dropped (Union.limit is a bare expression). A FETCH written after a compound query's ORDER BY is still attached to the last branch by the parser.

🤖 Generated with Claude Code

[tobilg]

Recommendation: request changes before merging.

Moving Oracle row-limit rendering into the shared generator is a good architectural direction. It addresses the builder/direct-generator problem and extends support to nested queries. A few remaining cases affect correctness and regression coverage.

1. Preserve branch-local TOP semantics in set operations.

   This T-SQL query:

   SELECT a FROM t
   UNION ALL
   SELECT TOP 5 a FROM u

   currently becomes:

   SELECT a FROM t
   UNION ALL
   SELECT a FROM u FETCH FIRST 5 ROWS ONLY

   The source limits only the second branch; the generated statement limits the entire union. With ten rows in each table, the intended result contains 15 rows, while the generated form returns five. I reproduced this difference using equivalent queries in DuckDB and checked the documented clause semantics.

   This path already emitted Oracle-incompatible LIMIT syntax before the PR. The new conversion therefore needs to preserve the branch boundary before it can safely produce executable Oracle SQL.

   Suggested adjustment: wrap individually limited operands in subqueries and cover both sides of UNION, INTERSECT, and EXCEPT, including outer ordering and limits. Relevant code: generator.rs.

2. Make configuration-driven rendering and T-SQL rendering mutually exclusive.

   With dialect = TSQL and limit_fetch_style = FetchFirst, generating an AST for:

   SELECT a FROM t ORDER BY a LIMIT 5 OFFSET 2

   produces:

   SELECT a FROM t ORDER BY a
   OFFSET 2 ROWS FETCH NEXT 5 ROWS ONLY FETCH FIRST 5 ROWS ONLY

   This configuration emitted a single valid FETCH clause on the base commit. Changing T-SQL’s default to Top avoids the overlap for default callers, but explicit configurations still encounter it. Presto and Trino also bypass FetchFirst when OFFSET is present.

   Suggested adjustment: establish consistent precedence between the configuration and dialect-specific handling, then test LIMIT/OFFSET and explicit FETCH combinations. Relevant code: generator.rs.

3. Include the new regression cases in CI’s selected test targets.

   The ten new tests pass when run explicitly, but oracle_limit_regression.rs is not included in the test targets selected by the workflow. The green CI result therefore does not cover these tests.

   Suggested adjustment: move direct-generator assertions into the existing generator unit tests and cross-dialect cases into tests/dialect_matrix.rs, which CI already executes. See the Makefile test selection.

Additional observations for scope tracking:

• Standalone Limit(NULL) and Limit(ALL) nodes still emit FETCH clauses instead of receiving the no-op handling used for SELECT.
• Percentage limits on set operations lose PERCENT; that metadata loss already exists on the base commit.
• Oracle compound-query regeneration can place FETCH before ORDER BY; that ordering defect also predates this PR.

Validation completed: all ten new tests, all 208 dialect-matrix tests, and Rust formatting passed locally. Additional probes reproduced the cases above against both the PR and its base. No Oracle server was used.

The implementation adds no obvious expensive traversal or duplicate generation pass; performance assessment here is based on code inspection, not optimized benchmarks. The local SQLGlot implementation shares some of these correctness gaps, so matching its output alone would not resolve them.

Addressing the three requested changes above would make the fix substantially safer to merge.

[tobilg]

The update resolves the configuration-precedence issue and moves the regression tests into CI-selected suites. Standalone LIMIT ALL / LIMIT NULL handling is also fixed. The shared-generator approach fits the existing architecture.

One variant of the earlier branch-limit finding remains: generate_set_operand only recognizes bare Expression::Select nodes. An Expression::Annotated wrapper bypasses the new grouping logic.

For example, this T-SQL input:

SELECT TOP 5 a FROM t
/* branch note */
UNION ALL SELECT a FROM u

generates the following for DuckDB and SQLite:

SELECT a FROM t LIMIT 5 /* branch note */
UNION ALL SELECT a FROM u

I reproduced execution failures in both databases: DuckDB reports a syntax error at UNION, and SQLite reports that LIMIT must appear after UNION ALL. Both block comments and line comments trigger this case.

The same omission can change results through direct AST generation. When an annotation wraps the limited right-hand operand, the generated SQL becomes:

SELECT a FROM t
UNION ALL
SELECT a FROM u LIMIT 5 /* branch note */

With ten rows in each table, DuckDB and SQLite return 5 rows instead of the intended 15.

Suggested adjustment: inspect through annotation wrappers when deciding whether an operand requires grouping, while preserving its comments during rendering. Add commented SQL cases to the existing dialect_matrix.rs tests and annotated AST cases to the generator unit tests, covering both operands and the set operators.

Validation on ded13a1b:

• make test-rust-verify passed with the existing exclusions, including 1,304 library tests, 214 dialect-matrix tests, fixture suites, ClickHouse checks and FFI tests.
• Formatting and diff whitespace checks passed.
• CI passed.
• Independent DuckDB and SQLite execution reproduced the remaining issue.

The local SQLGlot implementation also produces invalid SQL for the commented example, so its output does not provide a correct reference for this case. No Oracle server was used.

Code inspection found no additional whole-tree traversal or duplicate generation pass; comparative performance benchmarks were not run.

[geoHeil]

Thanks. Fixed the Annotated variant in d86263b.

• generate_set_operand now recurses through Expression::Annotated. It groups the inner operand and writes the comments after the closing parenthesis. This applies to both operands of UNION, INTERSECT and EXCEPT.
  • SELECT TOP 5 a FROM t\n/* branch note */\nUNION ALL SELECT a FROM u in DuckDB → (SELECT a FROM t LIMIT 5) /* branch note */ UNION ALL SELECT a FROM u
  • SQLite → SELECT * FROM (SELECT a FROM t LIMIT 5) /* branch note */ UNION ALL SELECT a FROM u
  • Oracle → (SELECT a FROM t FETCH FIRST 5 ROWS ONLY) /* branch note */ UNION ALL SELECT a FROM u
• Tests:
  • dialect_matrix.rs: the reviewer's commented T-SQL input, with block and line comments, for all three set operators, transpiled to DuckDB, SQLite and Oracle.
  • Generator unit test: annotated AST operands on the left and right side, for all three set operators.
• Validation:
  • cargo test -p polyglot-sql --lib (1305) and --test dialect_matrix (215) pass.
  • I executed the SQLite output for the right-operand case with 10 rows per table and got 15 rows.
  • DuckDB execution was not run locally.

🤖 Generated with Claude Code

[tobilg]

Merged, thanks! Would it be possible to instruct your agent not to add himself as a co-committer? This is possible with either global or local instructions via AGENT.md or CLAUDE.md

https://code.claude.com/docs/en/settings-reference#git-and-attribution

[geoHeil]

Merged, thanks! Would it be possible to instruct your agent not to add himself as a co-committer? This is possible with either global or local instructions via AGENT.md or CLAUDE.md

https://code.claude.com/docs/en/settings-reference#git-and-attribution

let me look into that - would it make sense to enforce this from this project harness? I guess this is relevant for any future contributions of a diverse set of people/agents?