Skip to content

fix: assign trailing LIMIT and OFFSET to set operations - #2751

Open
fudianchn wants to merge 1 commit into
JSQLParser:masterfrom
fudianchn:fix/set-operation-limit-ownership
Open

fudianchn wants to merge 1 commit into
JSQLParser:masterfrom
fudianchn:fix/set-operation-limit-ownership

Conversation

@fudianchn

Copy link
Copy Markdown
Contributor

AI disclosure: this change was prepared with AI coding agents, reviewed and revised line by line by me.

What

Trailing LIMIT and OFFSET clauses now belong to SetOperationList when its final SELECT is unparenthesized, including queries without ORDER BY.

Why

For SELECT a FROM t1 UNION SELECT b FROM t2 LIMIT 1, the parser stores LIMIT on the last PlainSelect and leaves the set-level limit null. PostgreSQL applies that clause to the complete UNION result. SQL round trips hide the incorrect AST ownership, so consumers inspecting or changing the set-level limit cannot control the intended result.

How

  1. Transfer a non-null terminal LIMIT and OFFSET independently of ORDER BY, following the existing suffix-transfer blocks.
  2. Keep the PlainSelect guard so parenthesized branches retain their local clauses.
  3. Correct two existing getter assertions and add AST, parse/deparse and SQL execution regressions.

Root cause

The final PlainSelect consumes the trailing LIMIT and OFFSET before SetOperationList is constructed. Their transfer to the set node is nested inside the ORDER BY guard, so an absent ORDER BY leaves them attached to the branch.

Testing

  • SetOperationLimitTest adds 31 cases. On upstream master bb55bb9d, 18 fail and 13 normal controls pass; the fix passes all 31. Cases include UNION/UNION ALL, INTERSECT, EXCEPT, the existing MINUS syntax, WITH, multiple branches, LIMIT expressions and parameters, LIMIT/OFFSET/FETCH combinations, and local/global parentheses. Removing the parenthesis guard is rejected by six tests; restoring the fixed grammar passes again.
  • JDK 17 targeted Gradle run: SetOperationLimitTest, SelectTest and SetOperationModifierTest report 794 cases, zero failures/errors and eight existing skipped cases, with 786 actually executed. SpotlessCheck passes.
  • PostgreSQL 18.4, existing container and read-only constant queries: global versus parenthesized local LIMIT 1 yields one versus two rows; OFFSET 2 yields zero versus one row. H2 consumer tests also change the parsed global limit to zero and verify zero total rows, while changing a local limit retains the other branch's row.
  • Full Gradle check passes with 9,433 cases and Maven clean verify plus Spotless check passes with 9,415 cases, both with zero failures/errors and 25 skipped. Gradle static checks, grammar ambiguity and applicable coverage pass. Restricted license checking scans both changed Java files successfully; the grammar header is manually verified unchanged. Maven's 33 existing SQL header warnings all refer to files byte-identical to the base commit. Project JMH parseSQLStatements, unchanged 54-statement corpus, version=latest: interleaved states, three forks per state, two one-second warmups and five one-second measurements per fork; 15 samples per state. Baseline 28.608 ms/op, 99.9% CI [21.591,35.624]; fixed 29.544 ms/op, CI [21.057,38.031]. The intervals overlap; no measurable regression in this benchmark. This does not measure a suffix-ownership-specific workload. No Windows/macOS local matrix was run.

Behavior notes

  • Consumers should read the global LIMIT/OFFSET from SetOperationList rather than the last unparenthesized PlainSelect. The branch fields become null. Parenthesized local clauses retain their existing ownership.
  • SQL output is unchanged. No tokens, public APIs or syntax acceptance are added. Existing ORDER BY and FETCH handling remains covered, including the original Incorrect association of LIMIT and OFFSET in a union query #903 and Union with order and limit #1116 examples.
  • This change does not address invalid duplicate clauses, ClickHouse LIMIT BY ownership or precedence between mixed set-operation types.

Verification of the original issue

No open issue is linked. The missing no-ORDER path was reproduced on upstream master bb55bb9de8377de9880b14e4fe3b3cc94bf63498; the locally verified fixed commit is 9fa49735e7be923321df3544debf78d56d85e894. This builds on Tomer Shay's merged #1132, which fixed the ORDER-plus-LIMIT cases from #1018 and #1116. Thanks to Tomer Shay for establishing that transfer; this change completes its no-ORDER branch and keeps the historical examples as passing AST controls. Those closed issues are not reopened or claimed fixed again.

SELECT a FROM t1 UNION SELECT b FROM t2 LIMIT 1;
SELECT a FROM t1 UNION SELECT b FROM t2 OFFSET 2;
SELECT a FROM t1 UNION (SELECT b FROM t2 LIMIT 1);

The first two store their suffix on the set node and clear it from the last branch. The third keeps LIMIT on the inner PlainSelect and leaves the set-level limit null.

Signed-off-by: 付典 <fudianchn@gmail.com>
@fudianchn
fudianchn marked this pull request as ready for review October 2, 2026 07:25

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant